qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v9 03/27] qapi: Plug leaks in test-qmp-*


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v9 03/27] qapi: Plug leaks in test-qmp-*
Date: Wed, 04 Nov 2015 09:19:10 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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,



reply via email to

[Prev in Thread] Current Thread [Next in Thread]