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

[Peter Maydell]

On 20 March 2014 19:21, Michael Roth <address@hidden> wrote:
> 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))
>


What's the status of this? I noticed that clang is now
complaining at compile time as well as runtime:

  CC    tests/test-qmp-marshal.o
tests/test-qmp-marshal.c:181:9: warning: variable 'b' is used
uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]
    if (has_b) {
        ^~~~~
tests/test-qmp-marshal.c:188:42: note: uninitialized use occurs here
    retval = qmp_user_def_cmd3(a, has_b, b, &local_err);
                                         ^
tests/test-qmp-marshal.c:181:5: note: remove the 'if' if its condition
is always true
    if (has_b) {
    ^~~~~~~~~~~
tests/test-qmp-marshal.c:170:14: note: initialize the variable 'b' to
silence this warning
    int64_t b;
             ^
              = 0
1 warning generated.


thanks
-- PMM