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

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

[Michael Roth]

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>
---
 scripts/qapi-commands.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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))
 
-- 
1.9.1

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

[Markus Armbruster]

Michael Roth <mdroth@linux.vnet.ibm.com> writes:

> 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>
> ---
>  scripts/qapi-commands.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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))

Thanks for the clear commit message.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Peter Maydell]

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

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

[Michael Roth]

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

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

[Luiz Capitulino]

On Tue, 20 May 2014 12:20:39 -0500
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>
> ---
>  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))
>