Markus Armbruster <arm...@redhat.com> writes:
> Paolo Bonzini <pbonz...@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.
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.
[...]
