From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43333) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aW9Vj-0007CM-B8 for qemu-devel@nongnu.org; Wed, 17 Feb 2016 16:19:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aW9Vf-0008V9-Oz for qemu-devel@nongnu.org; Wed, 17 Feb 2016 16:19:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45587) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aW9Vf-0008V2-HP for qemu-devel@nongnu.org; Wed, 17 Feb 2016 16:19:43 -0500 References: <1455582057-27565-1-git-send-email-eblake@redhat.com> <1455582057-27565-13-git-send-email-eblake@redhat.com> <87vb5np5u7.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56C4E3EC.7040800@redhat.com> Date: Wed, 17 Feb 2016 14:19:40 -0700 MIME-Version: 1.0 In-Reply-To: <87vb5np5u7.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xmGGspr20RrEvsm9e3WQmBPjRdlIhsRkx" Subject: Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union() 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) --xmGGspr20RrEvsm9e3WQmBPjRdlIhsRkx Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/17/2016 11:08 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Commit cee2dedb noticed that if you have a partial flat union >> (such as if an input parse failed due to a missing >> discriminator), calling the dealloc visitor could result in >> trying to dereference a NULL pointer if we attempted to visit >> an object branch without an earlier successful call to >> visit_start_implicit_struct() allocating the pointer for that >> branch. But the "fix" it implemented requires the use of a >> '.data' member in the union, which may or may not be the same >> size as other branches of the union (consider a 32-bit platform >> where one of the branches is an int64), which feels fairly dirty. >=20 > Well, until the previous commit, it was the same, wasn't it? All > pointers. For simple unions, you could have (well, still can have, until my later patch gets rid of the simple_union_type() magic): struct SU { SUKind type; union { void *data; int8_t byte; } u; }; But you're right - for flat unions, ALL branches were represented as pointers (until this series unboxed them). >=20 >> Plus, as mentioned in that commit, it only works if you can >> assume that '.data' would be zero-initialized even if '.kind' was >> uninitialized, which is rather poor logic: our usage of >> visit_start_struct() happens to zero-initialize both fields, >> which means '.kind' is never truly uninitialized - but if we >> changed visit_start_struct() to use g_new() instead of g_new0(), >> then '.data' would not be any more reliable as a condition on >> whether to visit the branch matching '.kind', regardless of >> whether '.kind' was 0). >> >> Menawhile, now that we have just inlined the fields of all flat Meanwhile, >> unions, there is no longer the possibility of a null pointer to >> dereference in the first place. Where the branch structure used >> to be separately allocated by visit_start_implicit_struct(), it >> is now just pointing to a subset of the memory already >> zero-allocated by visit_start_struct(). I guess I may try and reword this slightly, and point to the fact that the NULL dereference was due to calling visit_start_implicit_FOO() (only done for flat unions; for simple unions the branches call visit_type_FOO(), and that call safely handled NULL); because we were using visit_start/end_implicit_struct() for its allocation effects. But the net result is the same - now that we no longer call visit_start_implicit_struct() for a union visit, the dealloc visitor no longer has to worry about a NULL dereference on a partially constructed object, so we no longer need to probe if the union contains any data. >> +++ b/scripts/qapi-visit.py >> @@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char = *name, %(c_name)s **obj, Error >> if variants: >> ret +=3D gen_err_check(label=3D'out_obj') >> ret +=3D mcgen(''' >> - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { >> - goto out_obj; >> - } >=20 > I'm afraid the previous commit broke this for flat unions. >=20 > Before the previous commit, all members of (*obj)->u were pointers to > the struct holding the variant members both for flat and simple unions.= > !!(*obj)->u.data tests whether the struct holding the variant members > has been allocated. This relies on uniform pointer format. >=20 > The dealloc visitor uses the "has been allocated" bit to suppress > visiting the struct when it hasn't been allocated. >=20 > The previous commit unboxes the struct for flat unions. Now ->u.data > reinterprets the first few bytes of that struct as pointer. If you're > "lucky", they're not all zero, and the struct gets visited. You're right - and I bet I could come up with a case where valgrind could call me on it. >=20 > Obvious fix: squash this hunk into the previous commit, then let this > commit drop the code that's no longer used. Yep, for bisectability, I think that's what I'll end up doing. >=20 > However, simple unions are still boxed. Why can't their pointer be nul= l > in the dealloc visitor? Simple unions still go through visit_type_FOO(), and _that_ function properly checks for NULL. It was only visit_type_implicit_FOO() that blindly dereferenced things. In fact, in the earlier incantation of this patch, my fix was to teach visit_type_implicit_FOO() how to check for NULL: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05442.html But now that visit_type_implicit_FOO() is gone, my earlier incantation got reduced in size. I guess it's all in how I document the commit messa= ge. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --xmGGspr20RrEvsm9e3WQmBPjRdlIhsRkx 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/ iQEcBAEBCAAGBQJWxOPsAAoJEKeha0olJ0NqisUH/jxf6i5L0CWmZxw+bnMW5bef mzvfescQPl8DZKycENs/Z6SNo+G0y7W2MHPU5qQk2ZPWNcS05Xmpah2A6y8r9l4/ gWDKgAp8TsowxNiUaf6CETmjKR+sKPvyWDyUoSV5npARN2e+af8Lxw881nW4DhiC QU0pl/wIAPRTHspmHWMEPxjrUj4TAjppoI2uZpLS0to0BazbU9jv0Gn1XxUlPkL6 MQXWef8qfkM4TeDcfW+m3fRBwTsShRUfR+6s7tmgtM+SPui4DALXZ0VyDnUclHlf YF9bX2ix8t0I8O0PjRpogHVKYi3TUEtHr+ZNv9/Fagze0NC81oCg0HuELxsdnBs= =JpGg -----END PGP SIGNATURE----- --xmGGspr20RrEvsm9e3WQmBPjRdlIhsRkx--