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

[Paolo Bonzini]

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!

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