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

[Markus Armbruster <armbru@redhat.com>]

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.

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.

[...]