* Re: [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters
2014-05-20 17:20 [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters Michael Roth
@ 2014-05-20 17:59 ` Markus Armbruster
2014-05-20 18:21 ` Peter Maydell
2014-05-21 13:27 ` Luiz Capitulino
2 siblings, 0 replies; 5+ messages in thread
From: Markus Armbruster @ 2014-05-20 17:59 UTC (permalink / raw)
To: Michael Roth; +Cc: peter.maydell, famz, qemu-devel, lcapitulino
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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters
2014-05-20 17:20 [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters Michael Roth
2014-05-20 17:59 ` Markus Armbruster
@ 2014-05-20 18:21 ` Peter Maydell
2014-05-20 18:41 ` Michael Roth
2014-05-21 13:27 ` Luiz Capitulino
2 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2014-05-20 18:21 UTC (permalink / raw)
To: Michael Roth
Cc: Fam Zheng, Markus Armbruster, QEMU Developers, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters
2014-05-20 18:21 ` Peter Maydell
@ 2014-05-20 18:41 ` Michael Roth
0 siblings, 0 replies; 5+ messages in thread
From: Michael Roth @ 2014-05-20 18:41 UTC (permalink / raw)
To: Peter Maydell
Cc: Fam Zheng, Markus Armbruster, QEMU Developers, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters
2014-05-20 17:20 [Qemu-devel] [PATCH] qapi: zero-initialize all QMP command parameters Michael Roth
2014-05-20 17:59 ` Markus Armbruster
2014-05-20 18:21 ` Peter Maydell
@ 2014-05-21 13:27 ` Luiz Capitulino
2 siblings, 0 replies; 5+ messages in thread
From: Luiz Capitulino @ 2014-05-21 13:27 UTC (permalink / raw)
To: Michael Roth; +Cc: famz, peter.maydell, qemu-devel, armbru
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))
>
^ permalink raw reply [flat|nested] 5+ messages in thread