Paolo Bonzini <pbonz...@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?
[...]
