Re: [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters

[Luiz Capitulino]

On Tue, 20 May 2014 12:20:39 -0500
Michael Roth <address@hidden> wrote:

> In general QMP command parameter values are specified by consumers of the
> QMP/HMP interface, but in the case of optional parameters these values may
> be left uninitialized.
> 
> It is considered a bug for code to make use of optional parameters that have
> not been flagged as being present by the marshalling code (via corresponding
> has_<parameter> parameter), however our marshalling code will still pass
> these uninitialized values on to the corresponding QMP function (to then
> be ignored). Some compilers (clang in particular) consider this unsafe
> however, and generate warnings as a result. As reported by Peter Maydell:
> 
>   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
>   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).
> 
> Fix this by initializing all QMP command parameters to {0} in the
> marshalling code prior to passing them on to the QMP functions.
> 
> Signed-off-by: Michael Roth <address@hidden>
> Reported-by: Peter Maydell <address@hidden>
> Tested-by: Peter Maydell <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-commands.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to the qmp branch, thanks.

> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 386f17e..7d93d01 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -111,7 +111,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))
>