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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Peter Maydell (2014-05-20 13:21:15)
> On 20 May 2014 18:20, Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Tested-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Had I tested this before? In any case I have now :-)
> 
> It fixes the more recent clang compile warning as well as
> the more long standing sanitizer runtime complaints.

Thanks! You added your Tested-by: in the original thread, but it
was probably old enough to warrant another test run :)

> 
> thanks
> -- PMM