From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYc1A-00020L-EF for qemu-devel@nongnu.org; Fri, 11 Apr 2014 10:01:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WYc15-0003hl-Do for qemu-devel@nongnu.org; Fri, 11 Apr 2014 10:01:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58325) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYc15-0003ha-6g for qemu-devel@nongnu.org; Fri, 11 Apr 2014 10:01:15 -0400 Message-ID: <5347F5A4.9040305@redhat.com> Date: Fri, 11 Apr 2014 16:01:08 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <87bnx3vomf.fsf@blackfin.pond.sub.org> <20140320192134.8983.86526@loki> In-Reply-To: <20140320192134.8983.86526@loki> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] qapi-commands.py generates code that uses uninitialized variables List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth , Markus Armbruster , Peter Maydell Cc: QEMU Developers , Anthony Liguori On 03/20/14 20:21, Michael Roth wrote: > Quoting Markus Armbruster (2014-03-18 04:32:08) >> Peter Maydell writes: >> >>> This is something clang's -fsanitize=undefined spotted. The >>> code generated by qapi-commands.py in qmp-marshal.c for >>> qmp_marshal_* functions where there are some optional >>> arguments looks like this: >>> >>> bool has_force = false; >>> bool force; >>> >>> mi = qmp_input_visitor_new_strict(QOBJECT(args)); >>> v = qmp_input_get_visitor(mi); >>> visit_type_str(v, &device, "device", errp); >>> visit_start_optional(v, &has_force, "force", errp); >>> if (has_force) { >>> visit_type_bool(v, &force, "force", errp); >>> } >>> visit_end_optional(v, errp); >>> qmp_input_visitor_cleanup(mi); >>> >>> if (error_is_set(errp)) { >>> goto out; >>> } >>> qmp_eject(device, has_force, force, errp); >>> >>> In the case where has_force is false, we never initialize >>> force, but then we use it by passing it to qmp_eject. >>> I imagine we don't then actually use the value, but clang >> >> Use of FOO when !has_FOO is a bug. >> >>> complains in particular for 'bool' variables because the value >>> that ends up being loaded from memory for 'force' is not either >>> 0 or 1 (being uninitialized stack contents). >>> >>> Anybody understand what the codegenerator is doing well enough >>> to suggest a fix? I'd guess that just initializing the variable either >>> at point of declaration or in an else {) clause of the 'if (has_force)' >>> conditional would suffice, but presumably you need to handle >>> all the possible data types... >> >> I can give it a try. Will probably take a while, though. > > Could it be as simple as this?: > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 9734ab0..a70482e 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -99,7 +99,7 @@ bool has_%(argname)s = false; > argname=c_var(argname), argtype=c_type(argtype)) > else: > ret += mcgen(''' > -%(argtype)s %(argname)s; > +%(argtype)s %(argname)s = {0}; > ''', > argname=c_var(argname), argtype=c_type(argtype)) > > Pointer-type are special-cased initialized to NULL, so that leaves these guys > in the current set of qapi-defined types that we use as direct arguments for > qmp commands: > > NON-POINTER TYPE: BlockdevOnError > NON-POINTER TYPE: bool > NON-POINTER TYPE: DataFormat > NON-POINTER TYPE: double > NON-POINTER TYPE: DumpGuestMemoryFormat > NON-POINTER TYPE: int64_t > NON-POINTER TYPE: MirrorSyncMode > NON-POINTER TYPE: NewImageMode > NON-POINTER TYPE: uint32_t > > I'm trying to make sense of whether {0} is a valid initializer in all these > cases, as I saw some references to GCC complaining about cases where you don't > use an initializer for each nested subtype (back in 2002 at least: > http://www.ex-parrot.com/~chris/random/initialise.html), but that doesn't seem > to be the case now. > > If that's not safe, we can memset based on sizeof() in the else clause, but > obviously that's sub-optimal. { 0 } is safe. { 0 } is a "universal initializer". If you tell me which C version we care about this week, I can look up and cite the language for you. The gist, as far as I remember, is that - 0 is a good initializer for any scalar type, - the outermost braces are ignored when initializing a scalar, - the outermost braces allow initialization of an aggregate (struct or array) or a union, - sub-aggregates don't require further braces. Thanks, Laszlo