From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33424) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxi1a-00077Q-23 for qemu-devel@nongnu.org; Fri, 21 Oct 2016 18:10:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxi1W-0003cd-FN for qemu-devel@nongnu.org; Fri, 21 Oct 2016 18:10:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54986) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxi1W-0003cO-4n for qemu-devel@nongnu.org; Fri, 21 Oct 2016 18:10:46 -0400 References: <1476787052-27333-1-git-send-email-ptoscano@redhat.com> <877f91ztpr.fsf@dusky.pond.sub.org> <57db07f2-daee-c494-5ca0-651b53bcc889@redhat.com> From: Eric Blake Message-ID: <32b6baf4-cd23-c277-4dfb-79922c710efe@redhat.com> Date: Fri, 21 Oct 2016 17:10:43 -0500 MIME-Version: 1.0 In-Reply-To: <57db07f2-daee-c494-5ca0-651b53bcc889@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="si9VTc9ajaT3sUts3h2LSJ6JkUaW6uOUJ" Subject: Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Pino Toscano Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --si9VTc9ajaT3sUts3h2LSJ6JkUaW6uOUJ From: Eric Blake To: Markus Armbruster , Pino Toscano Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Message-ID: <32b6baf4-cd23-c277-4dfb-79922c710efe@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor References: <1476787052-27333-1-git-send-email-ptoscano@redhat.com> <877f91ztpr.fsf@dusky.pond.sub.org> <57db07f2-daee-c494-5ca0-651b53bcc889@redhat.com> In-Reply-To: <57db07f2-daee-c494-5ca0-651b53bcc889@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/21/2016 04:32 PM, Eric Blake wrote: > In fact, applying this patch regresses to the very state that f24582d > tried to prevent. However, I'm unable to see a difference in valgrind > on tests/test-qmp-output-visitor either with or without this patch, > which sadly means our testsuite is not actually testing this scenario. >=20 >>> Should this go into -stable? >=20 > NACK. >=20 > As mentioned in the v1 thread, the leak that Pino was seeing is fixed b= y > http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html > I don't think we don't want this patch. And below is my hack to test-qmp-output-visitor, which shows (at least under valgrind) that we don't want the patch. I should probably turn that into a formal patch submission; but should I piggyback the existing test or add a new one? With the patch omitted, valgrind is happy; with the patch applied, I see: /visitor/output/struct-errors: =3D=3D8458=3D=3D Invalid read of size 8 =3D=3D8458=3D=3D at 0x175DC2: qobject_decref (qobject.h:81) =3D=3D8458=3D=3D by 0x1767A5: qentry_destroy (qdict.c:413) =3D=3D8458=3D=3D by 0x17690B: qdict_destroy_obj (qdict.c:451) =3D=3D8458=3D=3D by 0x1784AB: qobject_destroy (qobject.c:29) =3D=3D8458=3D=3D by 0x171927: qobject_decref (qobject.h:83) =3D=3D8458=3D=3D by 0x1721B4: qmp_output_free (qmp-output-visitor.c:22= 3) =3D=3D8458=3D=3D by 0x16ED8E: visit_free (qapi-visit-core.c:34) =3D=3D8458=3D=3D by 0x130F39: visitor_output_teardown (test-qmp-output-visitor.c:44) =3D=3D8458=3D=3D by 0x130FE1: visitor_reset (test-qmp-output-visitor.c= :59) =3D=3D8458=3D=3D by 0x1326EF: test_visitor_out_struct_errors (test-qmp-output-visitor.c:283) =3D=3D8458=3D=3D by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D Address 0x8259db8 is 8 bytes inside a block of size 4,1= 20 free'd =3D=3D8458=3D=3D at 0x4C2CD5A: free (vg_replace_malloc.c:530) =3D=3D8458=3D=3D by 0x5088F2D: g_free (in /usr/lib64/libglib-2.0.so.0.= 4800.2) =3D=3D8458=3D=3D by 0x176937: qdict_destroy_obj (qdict.c:456) =3D=3D8458=3D=3D by 0x1784AB: qobject_destroy (qobject.c:29) =3D=3D8458=3D=3D by 0x171927: qobject_decref (qobject.h:83) =3D=3D8458=3D=3D by 0x1721B4: qmp_output_free (qmp-output-visitor.c:22= 3) =3D=3D8458=3D=3D by 0x16ED8E: visit_free (qapi-visit-core.c:34) =3D=3D8458=3D=3D by 0x130F39: visitor_output_teardown (test-qmp-output-visitor.c:44) =3D=3D8458=3D=3D by 0x130FE1: visitor_reset (test-qmp-output-visitor.c= :59) =3D=3D8458=3D=3D by 0x1326EF: test_visitor_out_struct_errors (test-qmp-output-visitor.c:283) =3D=3D8458=3D=3D by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D Block was alloc'd at =3D=3D8458=3D=3D at 0x4C2DA60: calloc (vg_replace_malloc.c:711) =3D=3D8458=3D=3D by 0x5088E70: g_malloc0 (in /usr/lib64/libglib-2.0.so= =2E0.4800.2) =3D=3D8458=3D=3D by 0x175EAF: qdict_new (qdict.c:33) =3D=3D8458=3D=3D by 0x171CAB: qmp_output_start_struct (qmp-output-visi= tor.c:108) =3D=3D8458=3D=3D by 0x16EE4A: visit_start_struct (qapi-visit-core.c:47= ) =3D=3D8458=3D=3D by 0x1326E3: test_visitor_out_struct_errors (test-qmp-output-visitor.c:282) =3D=3D8458=3D=3D by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.480= 0.2) =3D=3D8458=3D=3D by 0x50A8D1D: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.4800.2) =3D=3D8458=3D=3D by 0x50A8D40: g_test_run (in /usr/lib64/libglib-2.0.s= o.0.4800.2) =3D=3D8458=3D=3D by 0x134F8D: main (test-qmp-output-visitor.c:899) diff --git i/tests/test-qmp-output-visitor.c w/tests/test-qmp-output-visitor.c index 5e926d3..f1b5591 100644 --- i/tests/test-qmp-output-visitor.c +++ w/tests/test-qmp-output-visitor.c @@ -265,18 +265,22 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, EnumOne bad_values[] =3D { ENUM_ONE__MAX, -1 }; UserDefOne u =3D {0}; UserDefOne *pu =3D &u; - Error *err; + Error *err =3D NULL; int i; + /* First check: error within struct is detected */ for (i =3D 0; i < ARRAY_SIZE(bad_values) ; i++) { - err =3D NULL; u.has_enum1 =3D true; u.enum1 =3D bad_values[i]; visit_type_UserDefOne(data->ov, "unused", &pu, &err); - g_assert(err); - error_free(err); + error_free_or_abort(&err); visitor_reset(data); } + + /* Second check: aborting visit early doesn't leak */ + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); + visitor_reset(data); } --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --si9VTc9ajaT3sUts3h2LSJ6JkUaW6uOUJ 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/ iQEcBAEBCAAGBQJYCpJjAAoJEKeha0olJ0Nq1SgH+wYHyT0iCgtphsj8h80G9jYT 0t41wINZcJ3bKczx4dzE5LFLtLo8mEuy3Lqth9Sj1lOYdMv5u5YhA10fPyhdhrIr BwLHcarQqZH2t8UFQFo/aq9tjwOfDBPa2EefHUEQdIRIPdKd4yecc0clicUhvllZ zRt6x8Wkk0cGzwVa2bKt6oMWAsYpd2UR6cAmreBpsIkOi+ui0L5Zl6XDEn2Y6//o 5WsSwO+nCZ8KE1GNh3MuQkpF/KvP3R5zxLOktHLSbtvZ8FGb0OYF9wwmphCKN/4+ wDsxSfkjHjengvTKtu8okZccNAQZOopw8XCpgDlzLeRsvTtjhXdDbxA/wBR/9r8= =a6MU -----END PGP SIGNATURE----- --si9VTc9ajaT3sUts3h2LSJ6JkUaW6uOUJ--