From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGs6c-0008NH-2w for qemu-devel@nongnu.org; Wed, 06 Jan 2016 12:42:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGs6Y-0005FK-Se for qemu-devel@nongnu.org; Wed, 06 Jan 2016 12:42:42 -0500 References: <1450717720-9627-1-git-send-email-eblake@redhat.com> <1450717720-9627-20-git-send-email-eblake@redhat.com> From: Eric Blake Message-ID: <568D5207.5050306@redhat.com> Date: Wed, 6 Jan 2016 10:42:31 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DrNnACIxESIQUW8SSudTQaXsKH4o1V0ub" Subject: Re: [Qemu-devel] [PATCH v8 19/35] qmp: Fix reference-counting of qnull on empty output visit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: Markus Armbruster , qemu-stable , QEMU , Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --DrNnACIxESIQUW8SSudTQaXsKH4o1V0ub Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/05/2016 07:05 AM, Marc-Andr=C3=A9 Lureau wrote: > Hi >=20 > On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake wrote: >> Commit 6c2f9a15 ensured that we would not return NULL when the >> caller used an output visitor but had nothing to visit. But >> in doing so, it added a FIXME about a reference count leak >> that could abort qemu in the (unlikely) case of SIZE_MAX such >> visits (more plausible on 32-bit). (Although that commit >> suggested we might fix it in time for 2.5, we ran out of time; >> fortunately, it is unlikely enough to bite that it was not >> worth worrying about during the 2.5 release.) >> >> This fixes things by documenting the internal contracts, and >> explaining why the internal function can return NULL and only >> the public facing interface needs to worry about qnull(), >> thus avoiding over-referencing the qnull_ global object. >> >> It does not, however, fix the stupidity of the stack mixing >> up two separate pieces of information; add a FIXME to explain >> that issue. >> >> Signed-off-by: Eric Blake >> Cc: qemu-stable@nongnu.org >> >> +++ b/qapi/qmp-output-visitor.c >> @@ -29,6 +29,15 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack; >> struct QmpOutputVisitor >> { >> Visitor visitor; >> + /* FIXME: we are abusing stack to hold two separate pieces of >> + * information: the current root object in slot 0, and the stack >> + * of N objects still being built in slots 1 through N (for N+1 >> + * slots in use). Worse, our behavior is inconsistent: >> + * qmp_output_add_obj() visiting two top-level scalars in a row >> + * discards the first in favor of the second, but visiting two >> + * top-level objects in a row tries to append the second object >> + * into the first (since the first object was placed in the stack= >> + * in both slot 0 and 1, but only popped from slot 1). */ >=20 > I skipped checking thoroughly this comment, since it's a bit > off-topic, although it looks ok. >=20 > Later, oh well, it's fixed in next commit. Imho it's not strictly > necessary in this commit. I added the comment based on Markus' request that I document how the stack is used; but yes, it does feel like a bit of churn since it changes in the next commit. If there's a reason to respin, I might change it to: Visitor visitor; /* Stack holds two pieces of information: the current root object in * slot 0, then a stack of N objects still being built in slots 1 * through N (for N+1 slots in use). * FIXME: The root object should be stored separately from the * stack, particularly since qmp_output_add_obj() behaves * differently when visiting two top-level scalars in a row than * it does for two objects (the second object is appended to the * first, since the first is placed in both slots 0 and 1 but only * popped from slot 1). */ >=20 > Reviewed-by: Marc-Andr=C3=A9 Lureau >=20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --DrNnACIxESIQUW8SSudTQaXsKH4o1V0ub 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/ iQEcBAEBCAAGBQJWjVIHAAoJEKeha0olJ0Nq7fMH/RUQ11adE3mt1Ys6cr8V5wSo 0jQvY1Yrp/cWPE6JoEtxJJOe7xRYfFa7sPrsacdxeB2sB4j47a11KzvJXrss5htN K+Zx+n1vfPS10W2SGB7pCeopn4tN/GktVu1CoiWY6z08kB1X8Zet9CpI7cpP8bpf Q8Ib5VLxkGXA9HActagU0mRIDHQKRIuyExqZ18f+4Nm5iyQ42ZcEUw3FXY3CoGW6 R4D99PB1HbFZaqP8dMVbU3vVzIYZFHgn2etWpWAra6+MqMn3XFSgDcYN9fv5yXcf 4Bzz1kwBBduhSrlkPdMT3di2a3GrjAemcOsMgdL6yCoF3IZAVzlV+sGv8RHB648= =rfbA -----END PGP SIGNATURE----- --DrNnACIxESIQUW8SSudTQaXsKH4o1V0ub--