Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Paolo Bonzini <pbonzini@redhat.com>]

Il 10/02/2014 14:29, Markus Armbruster ha scritto:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 06/02/2014 15:30, Markus Armbruster ha scritto:
>>>> Visitors get passed a pointer to the visited object.  The generated
>>>> visitors try to cope with this pointer being null in some places, for
>>>> instance like this:
>>>>
>>>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>>>>
>>>> visit_start_optional() passes its second argument to Visitor method
>>>> start_optional.  Two out of two methods dereference it
>>>> unconditionally.
>>>
>>> Some visitor implementations however do not implement start_optional
>>> at all.  With these visitor implementations, you currently could pass
>>> a NULL object.  After your patch, you still can but you're passing a
>>> bad pointer which is also a problem (perhaps one that Coverity would
>>> also detect).
>>
>> We need to decide what the contract for the public visit_type_FOO() and
>> visit_type_FOOlist() is.
>>
>> No existing user wants to pass a null pointer, the semantics of passing
>> a null pointer are not obvious to me, and as we'll see below, the
>> generated code isn't entirely successful in avoiding to dereference a
>> null argument :)
>>
>>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>>> index ff4239c..3eb10c8 100644
>>>> --- a/scripts/qapi-visit.py
>>>> +++ b/scripts/qapi-visit.py
>>>> @@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
>>>>
>>>>      if base:
>>>>          ret += mcgen('''
>>>> -visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
>>>> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
>>>
>>> This is the implementation of start_implicit_struct:
>>
>> One of two implementations.
>>
>>> static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
>>>                                             size_t size, Error **errp)
>>> {
>>>     if (obj) {
>>>         *obj = g_malloc0(size);
>>>     }
>>> }
>>>
>>> Before your patch, if obj is NULL, *obj is not written.
>>>
>>> After your patch, if obj is NULL, and c_name is not the first field in
>>> the struct, *obj is written and you get a NULL pointer
>>> dereference. Same for end_implicit_struct in
>>> qapi/qapi-dealloc-visitor.c.
>>>
>>> So I think if you remove this checking, you need to do the same in the
>>> visitor implementations as well.
>>
>> Can do.
>
> I'd like to keep this null check.  Let me explain why.

Patch 10 is okay then!

We really should write down all of this.  Thanks for spelling it down 
for us! :(

Paolo

> The start_struct() callback gets called in two separate ways.
>
> 1. Boxed struct: argument is a struct **.
>
> 2. Unboxed struct: argument is null.
>
> Example: UserDefTwo from tests/qapi-schema/qapi-schema-test.json
>
> Schema:
>
>     { 'type': 'UserDefTwo',
>       'data': { 'string': 'str',
>                 'dict': { 'string': 'str',
>                           ... } } }
>
> Generated type:
>
>     struct UserDefTwo
>     {
>         char * string;
>         struct
>         {
>             char * string;
>             ...
>         } dict;
>     };
>
> The generated visit_type_UserDefTwo() takes a struct UserDefOne **
> argument, which can't sensibly be null, as discussed earlier in this
> thread.
>
> It passes that argument to visit_start_struct().  This is the boxed
> case.
>
> When it's time to visit UserDefTwo member dict, it calls
> visit_start_struct() again, but with a null argument.  This is the
> unboxed case.
>
> Therefore, implementations of start_struct() better be prepared for a
> null argument.  opts_start_struct() isn't, and I'm going to fix it.
>
> start_implicit_struct() is not currently called with a null argument as
> far as I can tell, but I'd prefer to keep it close to start_struct().
>
> The fact that we're still committing interfaces like
> include/qapi/visitor.h without spelling out at least the non-obvious
> parts of the callback contracts is depressing.
>
> [...]
>