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

[Paolo Bonzini <pbonzini@redhat.com>]

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).

> 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:

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.

I think NULL pointer input can be used to *validate* input against QAPI 
types without building a throw-away object (which entails unbounded 
memory allocations for list types).  I don't know if the state of this 
is "broken", "it never worked", or "works but not tested and never 
used".  It's definitely not covered by the unit tests.

>  if (!err) {
> -    visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
> +    visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
>      error_propagate(errp, err);
>      err = NULL;
>      visit_end_implicit_struct(m, &err);
> @@ -61,8 +61,8 @@ if (!err) {
>      for argname, argentry, optional, structured in parse_args(members):
>          if optional:
>              ret += mcgen('''
> -visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
> -if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
> +visit_start_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
> +if ((*obj)->%(prefix)shas_%(c_name)s) {
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>                           c_name=c_var(argname), name=argname)
> @@ -72,7 +72,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
>              ret += generate_visit_struct_body(full_name, argname, argentry)
>          else:
>              ret += mcgen('''
> -visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
> +visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>                           type=type_name(argentry), c_name=c_var(argname),
> @@ -121,7 +121,7 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
>
>      ret += mcgen('''
>  if (!err) {
> -    if (!obj || *obj) {
> +    if (*obj) {
>          visit_type_%(name)s_fields(m, obj, &err);

This is a different problem, and I think a different Coverity error too, 
isn't it?

No objections to patch 1-9.

Paolo

>          error_propagate(errp, err);
>          err = NULL;
> @@ -273,7 +273,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>      if (!error_is_set(errp)) {
>          visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
>          if (!err) {
> -            if (obj && *obj) {
> +            if (*obj) {
>  ''',
>                   name=name)
>
>