From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOq0X-0008U5-Az for qemu-devel@nongnu.org; Thu, 28 Jan 2016 12:05:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOq0S-0007YM-D0 for qemu-devel@nongnu.org; Thu, 28 Jan 2016 12:05:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46238) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOq0S-0007Xx-2J for qemu-devel@nongnu.org; Thu, 28 Jan 2016 12:05:16 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-36-git-send-email-eblake@redhat.com> <87h9hxiv66.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56AA4A4A.60703@redhat.com> Date: Thu, 28 Jan 2016 10:05:14 -0700 MIME-Version: 1.0 In-Reply-To: <87h9hxiv66.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Lw1PX02CXGsDexmc3TvgcXWeSkU7CT4Ob" Subject: Re: [Qemu-devel] [PATCH v9 35/37] qapi: Change visit_type_FOO() to no longer return partial objects List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Lw1PX02CXGsDexmc3TvgcXWeSkU7CT4Ob Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/28/2016 08:24 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Returning a partial object on error is an invitation for a careless >> caller to leak memory. As no one outside the testsuite was actually >> relying on these semantics, it is cleaner to just document and >> guarantee that ALL pointer-based visit_type_FOO() functions always >> leave a safe value in *obj during an input visitor (either the new >> object on success, or NULL if an error is encountered). >> >> Since input visitors have blind assignment semantics, we have to >> track the result of whether an assignment is made all the way down >> to each visitor callback implementation, to avoid making decisions >> based on potentially uninitialized storage. >=20 > I'm not sure I get this paragraph. What's "blind assignment semantics"= ? The caller does: { Foo *bar; /* uninit */ visit_type_Foo(&bar); if (no error) { /* use bar */ } } Which means the visitor core can't do 'if (*obj)', because obj may be uninitialized on entry; if it dereferences obj at all, it must be via '*obj =3D ...' which I'm terming a blind assignment. But I can try and come up with better wording. >=20 >> Note that we still leave *obj unchanged after a scalar-based >> visit_type_FOO(); I did not feel like auditing all uses of >> visit_type_Enum() to see if the callers would tolerate a specific >> sentinel value (not to mention having to decide whether it would >> be better to use 0 or ENUM__MAX as that sentinel). >=20 > The assigning input visitor functions (core and generated) all assign > either a pointer to a newly allocated object, or a non-pointer scalar > value. >=20 > Possible behaviors on error: >=20 > (0) What we have now: assign something that must be cleaned up with the= > dealloc visitor if it's a pointer, but is otherwise useless >=20 > CON: callers have to clean up > CON: exposes careless callers to useless values >=20 > (1) Don't assign anything >=20 > PRO: consistent > CON: exposes careless callers to uninitialized values Half-PRO: Caller can pre-initialize a default, and rely on that value on error. In fact, I think we have callers doing that when visiting an enum, and I didn't feel up to auditing them all when first writing the patch. But a small audit right now shows: qom/object.c:object_property_get_enum() starts with uninitialized 'int ret;', hardcodes 'return 0;' on some failures, but otherwise passes it to visit_type_enum() then blindly returns that value even if errp is set. Yuck. Callers HAVE to check errp rather than relying on the return value to flag errors; although it looks like the lone caller is in numa.c and passes &error_abort. Maybe I should just bite the bullet, and audit ALL uses of visitor for their behavior of what to expect in *obj on error. >=20 > (2) Assign zero bits >=20 > PRO: consistent > CON: exposes careless callers to bogus zero values Half-CON: Caller cannot pre-initialize a default >=20 > (3) Assign null pointer, else don't assign anything >=20 > CON: inconsistent > CON: mix of (1)'s and (2)'s CON Which I think is what I did in this patch. >=20 > (4) Other ideas? Store the caller's initial value (often all zero, but not necessarily), and restore THAT value on error (preserves a pre-initialized default, but leaves uninitialized garbage in place to bite careless callers) >=20 > Note that *obj is almost always null on entry, because we allocate > objects zero-initialized. Only root visits can expose their caller to > uninitialized values. >=20 >> Signed-off-by: Eric Blake >> >> +/* All qapi types have a corresponding function with a signature >> + * roughly compatible with this: >> + * >> + * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error= **errp); >> + * >> + * where *@obj is itself a pointer or a scalar. The visit functions = for >> + * built-in types are declared here, while the functions for qapi-def= ined >> + * struct, union, enum, and list types are generated (see qapi-visit.= h). >> + * Input visitors may receive an uninitialized *@obj, and guarantee a= >> + * safe value is assigned (a new object on success, or NULL on failur= e). >=20 > This specifies the safe value: NULL. Makes no sense for non-pointer > scalars. >=20 > May input visitors really receive uninitialized *@obj? As far as I can= > see, we routinely store a newly allocated object to *@obj on success, > and store nothing on failure. To be able to pass this to the dealloc > visitor, *@obj must have been null initially, mustn't it? Correct. Pre-patch: either the caller pre-initialized obj to NULL (and can then blindly pass it to the dealloc visitor regardless of whether visit_start_struct() succeeded), or it did not (in which case the dealloc visitor must not be called if *obj remains uninitialized because visit_start_struct() failed, but MUST be called if visit_start_struct() succeeded to clean up the partial object). Post-patch: calling visit_start_struct() always assigns to *obj, and the dealloc visitor can be safely called. >=20 >> + * Output visitors expect *@obj to be properly initialized on entry. >=20 > What about the dealloc visitor? Can be passed NULL, a partial object, or a complete object. But spelling that out would indeed be useful. >=20 >> + * >> + * Additionally, all qapi structs have a generated function compatibl= e >> + * with this: >> + * >> + * void qapi_free_FOO(void *obj); >> + * >> + * which behaves like free(), even if @obj is NULL or was only partia= lly >> + * allocated before encountering an error. >=20 > Will partially allocated QAPI objects remain visible outside input > visitors? A user can still hand-initialize a QAPI struct into partial state, although you are correct that this patch is what is changing things to no longer leak a partial object outside of the visit_type_FOO() calls. >> + * Returns true if *@obj was allocated; if that happens, and an error= >> + * occurs any time before the matching visit_end_struct(), then the >> + * caller (usually a visit_type_FOO() function) knows to undo the >> + * allocation before returning control further. >> */ >> -void visit_start_struct(Visitor *v, const char *name, void **obj, >> +bool visit_start_struct(Visitor *v, const char *name, void **obj, >> size_t size, Error **errp); >=20 > Need to see how this is used before I can judge whether I like it :) >=20 >> @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, vo= id **obj, >> ov->fake_id_opt->str =3D g_strdup(ov->opts_root->id); >> opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); >> } >> + return result; >> } >=20 > Stores the newly allocated object in *@obj on success, doesn't store > anything on failure. >=20 > To make cleanup possible, *@obj must be null initially. Then the retur= n > value is true iff *@obj is non-null on return. If we want, I can change these to all store *obj =3D NULL on failure. Thinking about it more: for any given visit_type_FOO(), if visit_start_struct() succeeds but something else later fails, *obj will be NULL; so it also makes sense that if visit_start_struct() fails than *obj should be NULL. >> -void visit_start_struct(Visitor *v, const char *name, void **obj, >> +bool visit_start_struct(Visitor *v, const char *name, void **obj, >> size_t size, Error **errp) >> { >> + bool result; >> + >> assert(obj ? size : !size); >> if (obj && visit_is_output(v)) { >> assert(*obj); >> } >> - v->start_struct(v, name, obj, size, errp); >> + result =3D v->start_struct(v, name, obj, size, errp); >> + if (result) { >> + assert(obj && *obj); >> + } >=20 > Roundabout way to write assert(!result || (obj && *obj)); >=20 > Heh, you even assert one half of what I'm trying to prove. >=20 > Can we make it assert(result =3D=3D (visit_is_input(v) && obj && *obj) = ? Missing a ); guessing that you meant: assert(result =3D=3D (visit_is_input(v) && obj && *obj)); Yes, I think we can, but we'd need a visit_is_input() helper. >> @@ -236,7 +253,11 @@ void visit_type_str(Visitor *v, const char *name,= char **obj, Error **errp) >> assert(*obj); >> } >> */ >> - v->type_str(v, name, obj, errp); >> + v->type_str(v, name, obj, &err); >> + if (!visit_is_output(v) && err) { >> + *obj =3D NULL; >=20 > Shouldn't storing null on error be left to v->type_str(), like we do fo= r > structs and lists? Not least because it begs the question whether this= > leaks a string stored by it. May be worthwhile to mandate that tighter semantics on each visitor. >> +++ b/scripts/qapi-visit.py >> @@ -301,10 +316,15 @@ void visit_type_%(c_name)s(Visitor *v, const cha= r *name, %(c_name)s **obj, Error >> visit_check_struct(v, &err); >> out_obj: >> visit_end_struct(v); >> + if (allocated && err) { >> + qapi_free_%(c_name)s(*obj); >> + *obj =3D NULL; >> + } >> out: >> error_propagate(errp, err); >> } >> -''') >> +''', >> + c_name=3Dc_name(name)) >> >> return ret >> >=20 > Hmm. >=20 > This grows qapi-visit.c by 14% for me. >=20 > Can we do the deallocation in the visitor core somehow? We'd have to > pass "how to deallocate" information: the appropriate qapi_free_FOO() > function. We already pass in "how to allocate" information: size. I thought about that; do we really want to be type-punning functions like that? But it does sound smaller; I can play with the idea. >=20 > Now I've seen the complete patch, two more remarks: >=20 > * I think all the allocating methods should behave the same. Right now= , > some store null on failure, and some don't. For the latter to work, > the value must be null initially (or else the dealloc visitor runs > amok). Agree; I'm leaning towards ALL allocating methods must store into *obj (either NULL on failure, or object on success). >=20 > * Do we really need the new return value? It looks like it's always > equal to visit_is_input(v) && obj && *obj. Except we don't (yet) have a visit_is_input() function, let alone one visible from within the generated qapi-visit.c code. Maybe doing that first would help. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Lw1PX02CXGsDexmc3TvgcXWeSkU7CT4Ob 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/ iQEcBAEBCAAGBQJWqkpKAAoJEKeha0olJ0Nq8p8H/A+odOdqOekKpziW+W01sTPx XY/Dd9TCJUuAiQxMmuzUMwEaLXifWchQNmHkcFVaXLaV9+mrYiy90eknHao+pkbV 7ihGbxls49yGug/ZecbtTfVF0oZRmoINv7eykLgwvyOakagoVCOZixC7jpvG14h6 ggNvzJYngaKOLXyVoDMwOSoWbHkQkw+zhwUWslatPDXtYpMCdItQIXofulpdksnd Fmopk4ByX4MW3gYkQCuVI8J3V3i9m222AcbGB4O14JoenDCwr/9Uam93khHWqpWr ZdVP2690sDqVNpIxU3Bw+LE71UuKNzbubJRLmNzC9CpFnKM5oQ0Cr155+d/DdOQ= =Mcpm -----END PGP SIGNATURE----- --Lw1PX02CXGsDexmc3TvgcXWeSkU7CT4Ob--