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

[Markus Armbruster]

Michael Roth <address@hidden> writes:

> 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?:

Possibly :)

> 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.

A superficial reading of C99 suggests {0} should work as long as 0 can
be assigned to the left hand side when it's of scalar type, or its first
part when it's not.

Predicting what might trigger warnings from random compilers is an
exercise in futility.  For what it's worth, we already have a number of
'{0}' initializers.

If they don't work out here, we can make the conditional enumerate more
(sets of) types.  I wouldn't worry about that now.