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

[Laszlo Ersek <lersek@redhat.com>]

On 03/20/14 20:21, Michael Roth wrote:
> Quoting Markus Armbruster (2014-03-18 04:32:08)
>> Peter Maydell <peter.maydell@linaro.org> 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