Message ID | 1400606439-19899-1-git-send-email-mdroth@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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>
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
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
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)) >
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))