From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34983) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu1nZ-0005F0-KI for qemu-devel@nongnu.org; Wed, 04 Nov 2015 12:24:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zu1nU-0005PX-QF for qemu-devel@nongnu.org; Wed, 04 Nov 2015 12:24:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu1nU-0005PB-HN for qemu-devel@nongnu.org; Wed, 04 Nov 2015 12:24:32 -0500 References: <1446618049-13596-1-git-send-email-eblake@redhat.com> <1446618049-13596-4-git-send-email-eblake@redhat.com> <87ziyutbwh.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <563A3F46.5010601@redhat.com> Date: Wed, 4 Nov 2015 10:24:22 -0700 MIME-Version: 1.0 In-Reply-To: <87ziyutbwh.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WGjaOr5tTa6PKB7Wx0Osb1r1e8kG6jcS4" Subject: Re: [Qemu-devel] [PATCH v9 03/27] qapi: Plug leaks in test-qmp-* List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --WGjaOr5tTa6PKB7Wx0Osb1r1e8kG6jcS4 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/04/2015 01:19 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> 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 >> >> --- >> 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(-) >=20 > No leaks to plug in test/-qmp-commands.c and test-qmp-event.c? Didn't check. I'll do that. >=20 >> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-stri= ct.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(TestInputVisi= torData *data, >> { >> Visitor *v; >> >> + validate_teardown(data, NULL); >> + >> data->obj =3D qobject_from_json(json_string); >> g_assert(data->obj !=3D NULL); >> >=20 > 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. >=20 > But why only validate_test_init_raw() and not validate_test_init()? >=20 Umm, because I didn't look, and just plugged holes? Will fix. > The two duplicate code, by the way. And the fix will probably be by having one call the other. >=20 >> @@ -193,6 +195,8 @@ static void test_validate_fail_struct(TestInputVis= itorData *data, >> >> visit_type_TestStruct(v, &p, NULL, &err); >> g_assert(err); >> + error_free(err); >> + /* FIXME: visitor should not allocate p when returning error */ >=20 > Indeed. >=20 > Recommend to always mention new FIXMEs in the commit message. This fixme is part of the answer to your question about 6/27 - we do have test coverage on non-arrays. >> +++ b/tests/test-qmp-input-visitor.c >> @@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorDat= a *data, >> Visitor *v; >> va_list ap; >> >> + visitor_input_teardown(data, NULL); >> + >> va_start(ap, json_string); >> data->obj =3D qobject_from_jsonv(json_string, &ap); >> va_end(ap); >=20 > Here, you add it to visitor_input_test_init(), but not > visitor_input_test_init_raw(). >=20 > These two duplicate code, too. Looks like I get to fix this too. >> +++ b/tests/test-qmp-output-visitor.c >> @@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitor= Data *data, >> qobj =3D QOBJECT(qdict); Here, qobj is an alias to qdict... >> visit_type_any(data->ov, &qobj, NULL, &err); >> g_assert(!err); >> + qobject_decref(qobj); >> obj =3D qmp_output_get_qobject(data->qov); >> g_assert(obj !=3D NULL); >> qdict =3D qobject_to_qdict(obj); >> @@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitor= Data *data, > qobj =3D qdict_get(qdict, "string"); =2E..but then we are reassigning it to instead be an alias within qdict. > g_assert(qobj); > qstring =3D qobject_to_qstring(qobj); >> g_assert(qstring); >> g_assert_cmpstr(qstring_get_str(qstring), =3D=3D, "foo"); >> qobject_decref(obj); >> - qobject_decref(qobj); Dereferencing a subset of the qdict leaks the overal qdict. >=20 > 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? Valgrind complained about a leak, not a use-after-free. But there indeed may be more than one issue that got solved by correctly dropping the reference at the right point in time, prior to reassigning qobj for use as pointing to a different portion of the qdict. >=20 > 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. I'll have to refresh my memory what else is going on, but I can indeed split this out if it is different than a simple memleak fix. >=20 > Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0). Sounds like a separate patch. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --WGjaOr5tTa6PKB7Wx0Osb1r1e8kG6jcS4 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWOj9GAAoJEKeha0olJ0NqDycIAIpVAWKPJJDVJuxnhz7o4x8F Z8hDKXg4x0enr5/HqcPvXpgK0MldxxrO8HPJAW3a+Kgq5hJnLozaeB1dziC5dg7W IVHJpTErPuSybOgYNy3QMLXdV8D+nXswk4H6PHtuukgvjui5qdbea1+ulyR2oTxB joVv2V+JHZf7wH9UusmGZj9wxUc8Aho3czvQoRLfXpKieZ7/55EV/bLBAe/O28tk KGqSTF30j77g/LlcEDvYDplHye3omK2AjhNBGyJIy/SW1etSSucBDj9lYu5VT0GA kxGgkaLawpvjJ0Koq1+f0QnJw7QXyZQInZlOskAngUy/WEHqKEpN5eO7afx2ypU= =Haj6 -----END PGP SIGNATURE----- --WGjaOr5tTa6PKB7Wx0Osb1r1e8kG6jcS4--