| Message ID | 1391697000-5855-11-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/06/2014 07:30 AM, Markus Armbruster wrote: > 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. Let's see if I can find them without Coverity's help... opts-visitor.c:opts_start_optional qmp-input-visitor.c:qmp_input_start_optional string-input-visitor.c:parse_start_optional Your counting is off :) All three unconditionally dereference. [While at it, this code style from parse_start_optional, and similar in other locations, is rather verbose: if (!siv->string) { *present = false; return; } *present = true; } Shorter would be: *present = !!siv->string; } but that doesn't affect this patch] > > I fail to see how hits pointer could legitimately be null. > > All this useless null checking is highly redundant, which Coverity > duly reports. About 200 times. > > Remove the useless null checks. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi-visit.py | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 02/06/2014 07:30 AM, Markus Armbruster wrote: >> 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. > > Let's see if I can find them without Coverity's help... > > opts-visitor.c:opts_start_optional > qmp-input-visitor.c:qmp_input_start_optional > string-input-visitor.c:parse_start_optional > > Your counting is off :) All three unconditionally dereference. Right. I can't count :) Most Coverity's defects are about silliness like this: visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err); if (obj && (*obj)->has_name) { visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err); } The second "obj ? ... : ..." is guarded by if (obj), thus cannot take the false branch. > [While at it, this code style from parse_start_optional, and similar in > other locations, is rather verbose: > > if (!siv->string) { > *present = false; > return; > } > > *present = true; > } > > Shorter would be: > > *present = !!siv->string; > } > > but that doesn't affect this patch] Coccinelle job, if I can teach it our macros. Without that, it frequently chokes on some macro usage, leaving too much code unexamined. >> I fail to see how hits pointer could legitimately be null. >> >> All this useless null checking is highly redundant, which Coverity >> duly reports. About 200 times. >> >> Remove the useless null checks. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi-visit.py | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) > > Reviewed-by: Eric Blake <eblake@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) > >
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. 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? [...]
Il 07/02/2014 15:23, Markus Armbruster ha scritto: >> > No objections to patch 1-9. > What does it take make you accept PATCH 10? The email you just wrote, basically, :) and removing the NULL pointer checks in the visitor implementations. Paolo
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. [...]
Il 10/02/2014 14:29, Markus Armbruster ha scritto: > 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. 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. > > [...] >
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 10/02/2014 14:29, Markus Armbruster ha scritto: >> 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. > > Patch 10 is okay then! Thanks! > We really should write down all of this. Thanks for spelling it down > for us! :( Yes, we should, and we should write it down before we commit the code! Not two years later, when the original author has forgotten everything, or has been run over by a bus[*], so the poor sod who needs to mess with it gets to figure it all out, and gets his chance to claim his place in git history as fabricator of "not quite correct, but we appreciate the effort" documentation. [...] >> 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. >> >> [...] >> [*] Or sucked into the guts of some corporation. Same result for us, but probably much preferred by the author.
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); 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); 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)
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. I fail to see how hits pointer could legitimately be null. All this useless null checking is highly redundant, which Coverity duly reports. About 200 times. Remove the useless null checks. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi-visit.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)