From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WBklP-0007Td-LY for qemu-devel@nongnu.org; Fri, 07 Feb 2014 07:42:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WBklN-0006oB-RD for qemu-devel@nongnu.org; Fri, 07 Feb 2014 07:42:35 -0500 Received: from mail-qc0-x22d.google.com ([2607:f8b0:400d:c01::22d]:42893) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WBklN-0006nI-LA for qemu-devel@nongnu.org; Fri, 07 Feb 2014 07:42:33 -0500 Received: by mail-qc0-f173.google.com with SMTP id i8so5774639qcq.32 for ; Fri, 07 Feb 2014 04:42:33 -0800 (PST) Sender: Paolo Bonzini Message-ID: <52F4D4B0.4010307@redhat.com> Date: Fri, 07 Feb 2014 13:42:24 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1391697000-5855-1-git-send-email-armbru@redhat.com> <1391697000-5855-11-git-send-email-armbru@redhat.com> In-Reply-To: <1391697000-5855-11-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, mdroth@linux.vnet.ibm.com, aliguori@amazon.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) > >