Eric Blake <ebl...@redhat.com> writes:
> Make valgrind happy with the current state of the tests, so that
> it is easier to see if future patches introduce new memory problems
> without being drowned in noise. Many of the leaks were due to
> calling a second init without tearing down the data from an earlier
> visit. But since teardown is already idempotent, and we already
> register teardown as part of input_visitor_test_add(), it is nicer
> to just make init() safe to call multiple times than it is to have
> to make all tests call teardown.
>
> Another common leak was forgetting to clean up an error object,
> after testing that an error was raised.
>
> Another leak was in test_visitor_in_struct_nested(), failing to
> clean the base member of UserDefTwo. Cleaning that up left
> check_and_free_str() as dead code (since using the qapi_free_*
> takes care of recursion, and we don't want double frees).
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v9: move earlier in series (was 13/17)
> v8: no change
> v7: no change
> v6: make init repeatable rather than adding teardown everywhere,
> fix additional leak with UserDefTwo base, plug additional files
> ---
> tests/test-qmp-input-strict.c | 10 ++++++++++
> tests/test-qmp-input-visitor.c | 41
> +++++++----------------------------------
> tests/test-qmp-output-visitor.c | 3 ++-
> 3 files changed, 19 insertions(+), 35 deletions(-)
No leaks to plug in test/-qmp-commands.c and test-qmp-event.c?
> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index b44184f..910e2f9 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData
> *data,
> {
> Visitor *v;
>
> + validate_teardown(data, NULL);
> +
> data->obj = qobject_from_json(json_string);
> g_assert(data->obj != NULL);
>
A test added with validate_test_add() may call validate_test_init_raw()
any number of time. Since validate_test_add() passes
validate_teardown() as fixture teardown function, the last one will be
cleaned up on test finalization. The others will be cleaned up by the
next validate_test_init_raw(). Okay. Actually, the whole fixture
business starts to make sense only now.
But why only validate_test_init_raw() and not validate_test_init()?
The two duplicate code, by the way.
> @@ -193,6 +195,8 @@ static void
> test_validate_fail_struct(TestInputVisitorData *data,
>
> visit_type_TestStruct(v, &p, NULL, &err);
> g_assert(err);
> + error_free(err);
> + /* FIXME: visitor should not allocate p when returning error */
Indeed.
Recommend to always mention new FIXMEs in the commit message.
> if (p) {
> g_free(p->string);
> }
> @@ -210,6 +214,7 @@ static void
> test_validate_fail_struct_nested(TestInputVisitorData *data,
>
> visit_type_UserDefTwo(v, &udp, NULL, &err);
> g_assert(err);
> + error_free(err);
> qapi_free_UserDefTwo(udp);
> }
>
> @@ -224,6 +229,7 @@ static void test_validate_fail_list(TestInputVisitorData
> *data,
>
> visit_type_UserDefOneList(v, &head, NULL, &err);
> g_assert(err);
> + error_free(err);
> qapi_free_UserDefOneList(head);
> }
>
> @@ -239,6 +245,7 @@ static void
> test_validate_fail_union_native_list(TestInputVisitorData *data,
>
> visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
> g_assert(err);
> + error_free(err);
> qapi_free_UserDefNativeListUnion(tmp);
> }
>
> @@ -253,6 +260,7 @@ static void
> test_validate_fail_union_flat(TestInputVisitorData *data,
>
> visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
> g_assert(err);
> + error_free(err);
> qapi_free_UserDefFlatUnion(tmp);
> }
>
> @@ -268,6 +276,7 @@ static void
> test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
>
> visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
> g_assert(err);
> + error_free(err);
> qapi_free_UserDefFlatUnion2(tmp);
> }
>
> @@ -282,6 +291,7 @@ static void
> test_validate_fail_alternate(TestInputVisitorData *data,
>
> visit_type_UserDefAlternate(v, &tmp, NULL, &err);
> g_assert(err);
> + error_free(err);
> qapi_free_UserDefAlternate(tmp);
> }
>
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 3f6bc4d..03c3682 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
> Visitor *v;
> va_list ap;
>
> + visitor_input_teardown(data, NULL);
> +
> va_start(ap, json_string);
> data->obj = qobject_from_jsonv(json_string, &ap);
> va_end(ap);
Here, you add it to visitor_input_test_init(), but not
visitor_input_test_init_raw().
These two duplicate code, too.
> @@ -177,12 +179,7 @@ static void test_visitor_in_enum(TestInputVisitorData
> *data,
> visit_type_EnumOne(v, &res, NULL, &err);
> g_assert(!err);
> g_assert_cmpint(i, ==, res);
> -
> - visitor_input_teardown(data, NULL);
> }
> -
> - data->obj = NULL;
> - data->qiv = NULL;
> }
>
>
> @@ -205,12 +202,6 @@ static void test_visitor_in_struct(TestInputVisitorData
> *data,
> g_free(p);
> }
>
> -static void check_and_free_str(char *str, const char *cmp)
> -{
> - g_assert_cmpstr(str, ==, cmp);
> - g_free(str);
> -}
> -
> static void test_visitor_in_struct_nested(TestInputVisitorData *data,
> const void *unused)
> {
> @@ -226,17 +217,14 @@ static void
> test_visitor_in_struct_nested(TestInputVisitorData *data,
> visit_type_UserDefTwo(v, &udp, NULL, &err);
> g_assert(!err);
>
> - check_and_free_str(udp->string0, "string0");
> - check_and_free_str(udp->dict1->string1, "string1");
> + g_assert_cmpstr(udp->string0, ==, "string0");
> + g_assert_cmpstr(udp->dict1->string1, ==, "string1");
> g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42);
> - check_and_free_str(udp->dict1->dict2->userdef->string, "string");
> - check_and_free_str(udp->dict1->dict2->string, "string2");
> + g_assert_cmpstr(udp->dict1->dict2->userdef->string, ==, "string");
> + g_assert_cmpstr(udp->dict1->dict2->string, ==, "string2");
> g_assert(udp->dict1->has_dict3 == false);
>
> - g_free(udp->dict1->dict2->userdef);
> - g_free(udp->dict1->dict2);
> - g_free(udp->dict1);
> - g_free(udp);
> + qapi_free_UserDefTwo(udp);
> }
>
> static void test_visitor_in_list(TestInputVisitorData *data,
> @@ -346,14 +334,12 @@ static void
> test_visitor_in_alternate(TestInputVisitorData *data,
> g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
> g_assert_cmpint(tmp->u.i, ==, 42);
> qapi_free_UserDefAlternate(tmp);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "'string'");
> visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
> g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
> g_assert_cmpstr(tmp->u.s, ==, "string");
> qapi_free_UserDefAlternate(tmp);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "false");
> visit_type_UserDefAlternate(v, &tmp, NULL, &err);
> @@ -361,7 +347,6 @@ static void
> test_visitor_in_alternate(TestInputVisitorData *data,
> error_free(err);
> err = NULL;
> qapi_free_UserDefAlternate(tmp);
> - visitor_input_teardown(data, NULL);
> }
>
> static void test_visitor_in_alternate_number(TestInputVisitorData *data,
> @@ -384,7 +369,6 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
> error_free(err);
> err = NULL;
> qapi_free_AltStrBool(asb);
> - visitor_input_teardown(data, NULL);
>
> /* FIXME: Order of alternate should not affect semantics; asn should
> * parse the same as ans */
> @@ -396,35 +380,30 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
> error_free(err);
> err = NULL;
> qapi_free_AltStrNum(asn);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltNumStr(v, &ans, NULL, &error_abort);
> g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
> g_assert_cmpfloat(ans->u.n, ==, 42);
> qapi_free_AltNumStr(ans);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltStrInt(v, &asi, NULL, &error_abort);
> g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
> g_assert_cmpint(asi->u.i, ==, 42);
> qapi_free_AltStrInt(asi);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltIntNum(v, &ain, NULL, &error_abort);
> g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
> g_assert_cmpint(ain->u.i, ==, 42);
> qapi_free_AltIntNum(ain);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltNumInt(v, &ani, NULL, &error_abort);
> g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
> g_assert_cmpint(ani->u.i, ==, 42);
> qapi_free_AltNumInt(ani);
> - visitor_input_teardown(data, NULL);
>
> /* Parsing a double */
>
> @@ -434,21 +413,18 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
> error_free(err);
> err = NULL;
> qapi_free_AltStrBool(asb);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltStrNum(v, &asn, NULL, &error_abort);
> g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N);
> g_assert_cmpfloat(asn->u.n, ==, 42.5);
> qapi_free_AltStrNum(asn);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltNumStr(v, &ans, NULL, &error_abort);
> g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
> g_assert_cmpfloat(ans->u.n, ==, 42.5);
> qapi_free_AltNumStr(ans);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltStrInt(v, &asi, NULL, &err);
> @@ -456,21 +432,18 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
> error_free(err);
> err = NULL;
> qapi_free_AltStrInt(asi);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltIntNum(v, &ain, NULL, &error_abort);
> g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N);
> g_assert_cmpfloat(ain->u.n, ==, 42.5);
> qapi_free_AltIntNum(ain);
> - visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltNumInt(v, &ani, NULL, &error_abort);
> g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N);
> g_assert_cmpfloat(ani->u.n, ==, 42.5);
> qapi_free_AltNumInt(ani);
> - visitor_input_teardown(data, NULL);
> }
>
> static void test_native_list_integer_helper(TestInputVisitorData *data,
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 9364843..8606111 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitorData
> *data,
> qobj = QOBJECT(qdict);
> visit_type_any(data->ov, &qobj, NULL, &err);
> g_assert(!err);
> + qobject_decref(qobj);
> obj = qmp_output_get_qobject(data->qov);
> g_assert(obj != NULL);
> qdict = qobject_to_qdict(obj);
> @@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitorData
> *data,
qobj = qdict_get(qdict, "string");
g_assert(qobj);
qstring = qobject_to_qstring(qobj);
> g_assert(qstring);
> g_assert_cmpstr(qstring_get_str(qstring), ==, "foo");
> qobject_decref(obj);
> - qobject_decref(qobj);
Hmm... obj is an alias for qdict, qobj is a member of qdict, freeing
obj frees qobj (unless there's another reference to qobj I can't see).
The line you delete then is a use-after-free bug that underflows the
reference counter. Correct?
If yes, commit message should mention it briefly, because this isn't
just a leak. Actually, I'd make it a separate commit, to keep commit
messages simple, particularly the headlines.
Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0).
> }
>
> static void test_visitor_out_union_flat(TestOutputVisitorData *data,
> @@ -463,6 +463,7 @@ static void
> test_visitor_out_alternate(TestOutputVisitorData *data,
> g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42);
>
> qapi_free_UserDefAlternate(tmp);
> + qobject_decref(arg);
> }
>
> static void test_visitor_out_empty(TestOutputVisitorData *data,
