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

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 10/02/2014 14:29, Markus Armbruster ha scritto:
>> Markus Armbruster <address@hidden> writes:
>>
>>> Paolo Bonzini <address@hidden> 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!

Thanks!

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

Yes, we should, and we should write it down before we commit the code!
Not two years later, when the original author has forgotten everything,
or has been run over by a bus[*], so the poor sod who needs to mess with
it gets to figure it all out, and gets his chance to claim his place in
git history as fabricator of "not quite correct, but we appreciate the
effort" documentation.

[...]

>> 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.
>>
>> [...]
>>


[*] Or sucked into the guts of some corporation.  Same result for us,
but probably much preferred by the author.