qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
Date: Fri, 07 Feb 2014 15:23:38 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

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.

Here's the other implementation:

static void qapi_dealloc_start_struct(Visitor *v, void **obj, const char *kind,
                                      const char *name, size_t unused,
                                      Error **errp)
{
    QapiDeallocVisitor *qov = to_qov(v);
    qapi_dealloc_push(qov, obj);
}

It happily passes null obj to qapi_dealloc_push().  There, null has a
special meaning, used by qapi_dealloc_start_list(): it's a list head
tracker.  I'd be surprised if this code could cope with null obj.

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

Perhaps it worked in theory for some early version, but it doesn't work
for the current version.  Code that has never been used or even tested
not working shouldn't surprise anybody :)

Consider visit_type_NameInfo(m, NULL, "whatever", &local_err).  Assuming
no errors anywhere, this calls

    visit_start_struct(m, NULL, "NameInfo", "whatever", ...)

then enters visit_type_NameInfo_fields(), which calls

    visit_start_optional(m, NULL, "whatever", ...)

and then crashes dereferencing null obj:

    if (obj && (*obj)->has_name) {
        visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err);
    }

It's not clear to me what visit_type_NameInfo_fields() should do for a
null obj.  Should it visit_type_str(m, NULL, ...)?  That would assume
the optional member is present.  Why?  Feels arbitrary to me.

Should a use for null obj materialize, we can put back support for null
obj.  It'll even work then, having a user.

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

It's the same, actually: I'm deleting code attempting to cope with null
obj.

The bulk of the Coverity defects is like this one:

310     static void visit_type_NameInfo_fields(Visitor *m, NameInfo ** obj, 
Error **errp)
311     {
312         Error *err = NULL;

(1) Event cond_notnull:         Condition "obj", taking true branch. Now the 
value of "obj" is not NULL.
Also see events:        [notnull][dead_error_condition][dead_error_line]

313         visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", 
&err);
314         if (obj && (*obj)->has_name) {

(2) Event notnull:      At condition "obj", the value of "obj" cannot be NULL.
(3) Event dead_error_condition:         The condition "obj" must be true.
(4) Event dead_error_line:      Execution cannot reach this expression "NULL" 
inside statement "visit_type_str(m, (obj ? &(...".
Also see events:        [cond_notnull]

315             visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err);
316         }
317         visit_end_optional(m, &err);
318     
319         error_propagate(errp, err);
320     }

In addition, we get a few bogus defects like this one:

2470    static void visit_type_PciBridgeInfo_bus_fields(Visitor *m, 
PciBridgeInfo ** obj, Error **errp)
2471    {
2472        Error *err = NULL;
2473        visit_type_int(m, obj ? &(*obj)->bus.number : NULL, "number", &err);
2474        visit_type_int(m, obj ? &(*obj)->bus.secondary : NULL, "secondary", 
&err);
2475        visit_type_int(m, obj ? &(*obj)->bus.subordinate : NULL, 
"subordinate", &err);
2476        visit_type_PciMemoryRange(m, obj ? &(*obj)->bus.io_range : NULL, 
"io_range", &err);
2477        visit_type_PciMemoryRange(m, obj ? &(*obj)->bus.memory_range : 
NULL, "memory_range", &err);
2478        visit_type_PciMemoryRange(m, obj ? &(*obj)->bus.prefetchable_range 
: NULL, "prefetchable_range", &err);
2479    
2480        error_propagate(errp, err);
2481    }
2482    
2483    static void visit_type_PciBridgeInfo_fields(Visitor *m, PciBridgeInfo 
** obj, Error **errp)
2484    {
2485        Error *err = NULL;

(1) Event cond_true:    Condition "!error_is_set(errp)", taking true branch

2486        if (!error_is_set(errp)) {
2487            Error **errp = &err; /* from outer scope */
2488            Error *err = NULL;
2489            visit_start_struct(m, NULL, "", "bus", 0, &err);

(2) Event cond_true:    Condition "!err", taking true branch

2490            if (!err) {

(3) Event cond_false:   Condition "!obj", taking false branch
(4) Event cond_false:   Condition "*obj", taking false branch
(6) Event var_compare_op:       Comparing "*obj" to null implies that "*obj" 
might be null.
Also see events:        [var_deref_op]

2491                if (!obj || *obj) {
2492                    visit_type_PciBridgeInfo_bus_fields(m, obj, &err);
2493                    error_propagate(errp, err);
2494                    err = NULL;

(5) Event if_end:       End of if statement

2495                }
2496                /* Always call end_struct if start_struct succeeded.  */
2497                visit_end_struct(m, &err);
2498            }
2499            error_propagate(errp, err);
2500        }

(7) Event cond_true:    Condition "obj", taking true branch

2501        visit_start_optional(m, obj ? &(*obj)->has_devices : NULL, 
"devices", &err);

(8) Event cond_true:    Condition "obj", taking true branch
(9) Event var_deref_op:         Dereferencing null pointer "*obj".
Also see events:        [var_compare_op]

2502        if (obj && (*obj)->has_devices) {
2503            visit_type_PciDeviceInfoList(m, obj ? &(*obj)->devices : NULL, 
"devices", &err);
2504        }
2505        visit_end_optional(m, &err);
2506    
2507        error_propagate(errp, err);
2508    }

>
> No objections to patch 1-9.

What does it take make you accept PATCH 10?

[...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]