Re: [Qemu-devel] qapi-commands.py generates code that uses uninitialized variables

[Michael Roth]

Quoting Markus Armbruster (2014-03-18 04:32:08)
> Peter Maydell <address@hidden> 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.