Re: [Qemu-devel] [PATCH v9 35/37] qapi: Change visit_type_FOO() to no longer return partial objects

[Eric Blake]

On 01/28/2016 08:24 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 <address@hidden>
>>

>> +/* 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 address@hidden 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 address@hidden, 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 address@hidden  As far as I 
> can
> see, we routinely store a newly allocated object to address@hidden on success,
> and store nothing on failure.  To be able to pass this to the dealloc
> visitor, address@hidden 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 address@hidden 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 address@hidden 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 address@hidden on success, doesn't store
> anything on failure.
> 
> To make cleanup possible, address@hidden must be null initially.  Then the 
> return
> value is true iff address@hidden 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