On 01/28/2016 08:24 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> 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. > > 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 = ...' which I'm terming a blind assignment.
But I can try and come up with better wording.
>
>> 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).
>
> The assigning input visitor functions (core and generated) all assign
> either a pointer to a newly allocated object, or a non-pointer scalar
> value.
>
> Possible behaviors on error:
>
> (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
>
> CON: callers have to clean up
> CON: exposes careless callers to useless values
>
> (1) Don't assign anything
>
> 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.
>
> (2) Assign zero bits
>
> PRO: consistent
> CON: exposes careless callers to bogus zero values
Half-CON: Caller cannot pre-initialize a default
>
> (3) Assign null pointer, else don't assign anything
>
> CON: inconsistent
> CON: mix of (1)'s and (2)'s CON
Which I think is what I did in this patch.
>
> (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)
>
> 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.
>
>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>
>> +/* 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-defined
>> + * 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 failure).
>
> This specifies the safe value: NULL. Makes no sense for non-pointer
> scalars.
>
> 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.
>
>> + * Output visitors expect *@obj to be properly initialized on entry.
>
> What about the dealloc visitor?
Can be passed NULL, a partial object, or a complete object. But
spelling that out would indeed be useful.
>
>> + *
>> + * Additionally, all qapi structs have a generated function compatible
>> + * with this:
>> + *
>> + * void qapi_free_FOO(void *obj);
>> + *
>> + * which behaves like free(), even if @obj is NULL or was only partially
>> + * allocated before encountering an error.
>
> 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);
>
> Need to see how this is used before I can judge whether I like it :)
>
>> @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void
>> **obj,
>> ov->fake_id_opt->str = g_strdup(ov->opts_root->id);
>> opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
>> }
>> + return result;
>> }
>
> Stores the newly allocated object in *@obj on success, doesn't store
> anything on failure.
>
> To make cleanup possible, *@obj must be null initially. Then the return
> value is true iff *@obj is non-null on return.
If we want, I can change these to all store *obj = 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 = v->start_struct(v, name, obj, size, errp);
>> + if (result) {
>> + assert(obj && *obj);
>> + }
>
> Roundabout way to write assert(!result || (obj && *obj));
>
> Heh, you even assert one half of what I'm trying to prove.
>
> Can we make it assert(result == (visit_is_input(v) && obj && *obj) ?
Missing a ); guessing that you meant:
assert(result == (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 = NULL;
>
> Shouldn't storing null on error be left to v->type_str(), like we do for
> 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 char
>> *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 = NULL;
>> + }
>> out:
>> error_propagate(errp, err);
>> }
>> -''')
>> +''',
>> + c_name=c_name(name))
>>
>> return ret
>>
>
> Hmm.
>
> This grows qapi-visit.c by 14% for me.
>
> 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.
>
> Now I've seen the complete patch, two more remarks:
>
> * 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).
>
> * 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
