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)