From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43119) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zajj9-0004DO-G3 for qemu-devel@nongnu.org; Sat, 12 Sep 2015 08:16:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zajj5-0007j4-DJ for qemu-devel@nongnu.org; Sat, 12 Sep 2015 08:16:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35189) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zajj5-0007ht-4h for qemu-devel@nongnu.org; Sat, 12 Sep 2015 08:16:15 -0400 References: <1441999089-28960-1-git-send-email-armbru@redhat.com> <1441999089-28960-21-git-send-email-armbru@redhat.com> <55F33D06.1090103@redhat.com> <55F3411B.5070905@redhat.com> <87a8ssf47y.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <55F41788.6030008@redhat.com> Date: Sat, 12 Sep 2015 06:16:08 -0600 MIME-Version: 1.0 In-Reply-To: <87a8ssf47y.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="435pubCxi1D6Iq8OITSFs1NxSfHMCVcuS" Subject: Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --435pubCxi1D6Iq8OITSFs1NxSfHMCVcuS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/12/2015 02:10 AM, Markus Armbruster wrote: > Relatively harmless, because the qnull_ singleton is static. Worth > fixing anyway, of course. >=20 >>> I'm still investigating, and may be able to find the patch >> >> Squash this in, and you can have: >> Reviewed-by: Eric Blake Well, your further questions are spot on; my squash proposal isn't quite right. > I put the qnull() in qmp_output_first() to avoid (QObject *)0 entirely.= > Also because that's where I found the FIXME :) >=20 > You lift it into one of two callers. Impact: >=20 > * qmp_output_get_qobject() >=20 > - master: return NULL when !e || !e->value >=20 > - my patch: return qnull() when !e > return NULL when !e->value >=20 > - your patch: return qnull() when !e || !e->value The leak in your patch was that qnull() increments the count, then qmp_output_get_object() also increments the count. I guess I'll have to investigate where e->value can be set to NULL to see if my idea was safe, or if we'd have to do something even different. If this were the only caller, then I guess we could always do something like this in qmp_output_first(), leaving the possibility of returning NULL for e->value: if (!e) { obj =3D qnull(); qobject_decref(obj); /* Caller will again increment refcount */ return obj; } But it's not the only caller. >=20 > * qmp_output_visitor_cleanup() >=20 > - master: root =3D NULL when when !e || !e->value >=20 > - my patch: root =3D qnull() when !e > root =3D NULL when !e->value >=20 > - your patch: root =3D NULL when when !e || !e->value And calls qobject_decref(root), but that is safe on NULL. Here, your patch ends up with a net 0 refcnt on qnull() (incremented in qmp_output_first(), decremented in the cleanup), but my idea above to pre-decrement would be wrong. Another option would be to keep your patch to qmp_output_first(), but then fix qmp_output_get_object() to special case it it has an instance of QNull (no refcnt change) vs. anything else (qobject_incref). But that feels gross. >=20 > where e =3D QTAILQ_LAST(&qov->stack, QStack) >=20 > Questions: >=20 > * How can e become NULL? >=20 > The only place that pushes onto &qov->stack appears to be > qmp_output_push_obj(). Can obviously push e with !e->value, but can'= t > push null e. My understanding is that qov->stack corresponds to nesting levels of {} or [] in the JSON code. The testsuite shows a case where the stack is empty as one case where e is NULL, but if e is non-NULL, I'm not sure whether e->value can ever be NULL. I'll have to read the code more close= ly. >=20 > * What about qmp_output_last()? >=20 > Why does qmp_output_first() check for !e, but not qmp_output_last()? >=20 > My patch makes them less symmetric (bad smell). Yours makes them mor= e > symmetric, but not quite. Which is awkward in its own right. >=20 > * How does this contraption work? Indeed. Without reading further, we're both shooting in the dark for something that makes tests pass, but without being a clean interface. How about this: go ahead with your series as proposed, with the squash hunk to tests/ to avoid the leak in the testsuite, but leaving the leak in qmp_output_get_object(), and we address the leak in a followup patch. refcnt is size_t, so on 32-bit platforms, it could roll over after 4G repeats of the leak and cause catastrophe, but I don't think we are outputting qnull() frequently enough for the leak to bite while we wait for a followup patch. The followup patch(es) will then have to include some contract documentation, so that we track what we learned while investigating the code, and so that the next reader has more than just code to start from. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --435pubCxi1D6Iq8OITSFs1NxSfHMCVcuS 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/ iQEcBAEBCAAGBQJV9BeNAAoJEKeha0olJ0NqqkcH/3nNkymN6xEtGJKdgvny5l2Z CPoweZE61+q0TBfIAQiqFldM+qMqqKBs7zlUNblyWuZ9X6bsxMpj55MNBIFPwmWe rcHb3J1hT4nnayTsZYchPwHKJ3tPP/0CTofkPfjaHjdd/p2g4flNwqfrC/Ct0wmN s+//fBHauKEFXHYaMid3j+MbgEVSfe6KKEBLCv6E9W/w1bSPeMQlfcwc7hpXylFB /IKJPqlLKq1QGDJy1t+GCCvawEMdzvqNxEjZ1lt4KA1mnMYFQEGCvl9ScCke4y87 bvV6FVxXEf9UnvQXFot0VhqCl76DuoxJBTe8Uy5jV+GXXKXBRFh7h2YoqaDAfKo= =kTaj -----END PGP SIGNATURE----- --435pubCxi1D6Iq8OITSFs1NxSfHMCVcuS--