From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WD9Ij-0002rl-7R for qemu-devel@nongnu.org; Tue, 11 Feb 2014 04:06:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WD9Ic-0000bq-JS for qemu-devel@nongnu.org; Tue, 11 Feb 2014 04:06:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17112) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WD9Ic-0000bH-Bs for qemu-devel@nongnu.org; Tue, 11 Feb 2014 04:06:38 -0500 Message-ID: <52F9E813.3070402@redhat.com> Date: Tue, 11 Feb 2014 10:06:27 +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> <52F4D4B0.4010307@redhat.com> <878utnvw5x.fsf@blackfin.pond.sub.org> <87ob2f6qqd.fsf@blackfin.pond.sub.org> In-Reply-To: <87ob2f6qqd.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15; 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 Cc: kwolf@redhat.com, qemu-devel@nongnu.org, aliguori@amazon.com, mdroth@linux.vnet.ibm.com Il 10/02/2014 14:29, Markus Armbruster ha scritto: > Markus Armbruster writes: > >> Paolo Bonzini 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. > > [...] >