From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55986) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxMKT-0004i1-GR for qemu-devel@nongnu.org; Thu, 11 Jul 2013 15:15:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UxMKR-0006dY-8b for qemu-devel@nongnu.org; Thu, 11 Jul 2013 15:15:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7418) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxMKR-0006dL-0i for qemu-devel@nongnu.org; Thu, 11 Jul 2013 15:14:59 -0400 Message-ID: <51DF040D.3070504@redhat.com> Date: Thu, 11 Jul 2013 13:14:21 -0600 From: Eric Blake MIME-Version: 1.0 References: <20130711145009.74852147@redhat.com> In-Reply-To: <20130711145009.74852147@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2NFXBCLCBGDQTKPJLKFSG" Subject: Re: [Qemu-devel] [RFC] qapi: qapi-commands: fix possible leaks on visitor dealloc List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, aliguori@us.ibm.com, lersek@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2NFXBCLCBGDQTKPJLKFSG Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/11/2013 12:50 PM, Luiz Capitulino wrote: > I'm sending this as an RFC because this is untested, and also because > I'm wondering if I'm seeing things after a long patch review session. I can't say that I tested it either, but... >=20 > The problem is: in qmp-marshal.c, the dealloc visitor calls use the > same errp pointer of the input visitor calls. This means that if > any of the input visitor calls fails, then the dealloc visitor will > return early, beforing freeing the object's memory. s/beforing/before/ >=20 > Here's an example, consider this code: >=20 > int qmp_marshal_input_block_passwd(Monitor *mon, const QDict *qdict, QO= bject **ret) > { > [...] >=20 > char * device =3D NULL; > char * password =3D NULL; >=20 > mi =3D qmp_input_visitor_new_strict(QOBJECT(args)); > v =3D qmp_input_get_visitor(mi); > visit_type_str(v, &device, "device", errp); > visit_type_str(v, &password, "password", errp); > qmp_input_visitor_cleanup(mi); >=20 > if (error_is_set(errp)) { > goto out; > } > qmp_block_passwd(device, password, errp); >=20 > out: > md =3D qapi_dealloc_visitor_new(); > v =3D qapi_dealloc_get_visitor(md); > visit_type_str(v, &device, "device", errp); I definitely agree that the current generated code passes in a non-null errp, and that visit_type_str is a no-op when started in an existing erro= r. > visit_type_str(v, &password, "password", errp); > qapi_dealloc_visitor_cleanup(md); >=20 > [...] >=20 > return 0; > } >=20 > Consider errp !=3D NULL when the out label is reached, we're going > to leak device and password. >=20 > This patch fixes this by always passing errp=3DNULL for dealloc > visitors, meaning that we always try to free them regardless of > any previous failure. The above example would then be: >=20 > out: > md =3D qapi_dealloc_visitor_new(); > v =3D qapi_dealloc_get_visitor(md); > visit_type_str(v, &device, "device", NULL); > visit_type_str(v, &password, "password", NULL); > qapi_dealloc_visitor_cleanup(md); Is that safe even if the failure was after device was parsed, meaning the initial visitor to password was a no-op and there is nothing to deallocate for password? I _think_ this is a correct fix (it means that errors encountered only while doing a dealloc pass are lost, but what errors are you going to encounter in that direction?); but I'd feel more comfortable is someone else more familiar with visitors chimes in. >=20 > Signed-off-by: Luiz Capitulino > --- > scripts/qapi-commands.py | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) >=20 > +visit_start_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s); > if (has_%(c_name)s) { > ''', > - c_name=3Dc_var(argname), name=3Dargname) > + c_name=3Dc_var(argname), name=3Dargname,errp=3D= errparg) Any reason you don't use space after ',' (several instances)? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2NFXBCLCBGDQTKPJLKFSG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJR3wQNAAoJEKeha0olJ0NqpX4IAK1hYKfFk0NxggefOW4oVWzr BPLS8AkUsGlIkFur6iw1/0/UdvFN5PJFq7MxBnASZRbuMevn4Ha2W15UB8BWU+j5 Hcj8qiAPB83wumh1eBlYcdJgBe1aYSqtyl1MDPQDuKx80YGnHzv7+14yLSoD0ROU /NT1ftuia/rq33WAyjT3JvD/7bnPyCInK7F2UWD7qut6ROzaXkP0zAadj/TvSTYm ZKI/WM69nBwaSgUOlfZ79mSGUqecY5bcnJnEMKmcsEMmM1asK5jHnjW2boQbvJYf QoBqaK2DU8fqAByyFXnyi6tr8TT60pimwelFFAgzwyJ5Hmd8S4sdmnA9D/Y3c9k= =ESYs -----END PGP SIGNATURE----- ------enig2NFXBCLCBGDQTKPJLKFSG--