| Message ID | 20160922203927.28241-3-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Calling: > > { "execute": "qom-set", > "arguments": { "path": "/machine", "property": "rtc-time" } } > > Will crash with: > > qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj' > failed This is actually a recent regression. Let's add "Broken in commit 5c678ee." Can do on commit. > Clear the obj and return an error. > > The patch also fixes a similar potential crash in qmp_input_type_null() > by checking qmp_input_get_object() returned a valid qobj. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > qapi/qmp-input-visitor.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 64dd392..fc91e74 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -338,6 +338,12 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, > QmpInputVisitor *qiv = to_qiv(v); > QObject *qobj = qmp_input_get_object(qiv, name, true); > > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + *obj = NULL; > + return; > + } > + > qobject_incref(qobj); > *obj = qobj; > } > @@ -347,6 +353,11 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) > QmpInputVisitor *qiv = to_qiv(v); > QObject *qobj = qmp_input_get_object(qiv, name, true); > > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > + > if (qobject_type(qobj) != QTYPE_QNULL) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "null");
Markus Armbruster <armbru@redhat.com> writes: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> Calling: >> >> { "execute": "qom-set", >> "arguments": { "path": "/machine", "property": "rtc-time" } } >> >> Will crash with: >> >> qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj' >> failed > > This is actually a recent regression. Let's add "Broken in commit > 5c678ee." Can do on commit. > >> Clear the obj and return an error. >> >> The patch also fixes a similar potential crash in qmp_input_type_null() >> by checking qmp_input_get_object() returned a valid qobj. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> I'd like to rephrase like this, if it's all right with you: qapi: Fix crash when 'any' or 'null' parameter is missing Unlike the other visit methods, visit_type_any() and visit_type_null() neglect to check whether qmp_input_get_object() succeeded. They crash when it fails. Reproducer: { "execute": "qom-set", "arguments": { "path": "/machine", "property": "rtc-time" } } Will crash with: qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj' failed Broken in commit 5c678ee. Fix by adding the missing error checks. Also: Reviewed-by: Markus Armbruster <armbru@redhat.com>
Hi Markus ----- Original Message ----- > Markus Armbruster <armbru@redhat.com> writes: > > > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > >> Calling: > >> > >> { "execute": "qom-set", > >> "arguments": { "path": "/machine", "property": "rtc-time" } } > >> > >> Will crash with: > >> > >> qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj' > >> failed > > > > This is actually a recent regression. Let's add "Broken in commit > > 5c678ee." Can do on commit. > > > >> Clear the obj and return an error. > >> > >> The patch also fixes a similar potential crash in qmp_input_type_null() > >> by checking qmp_input_get_object() returned a valid qobj. > >> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > > I'd like to rephrase like this, if it's all right with you: > > qapi: Fix crash when 'any' or 'null' parameter is missing > > Unlike the other visit methods, visit_type_any() and visit_type_null() > neglect to check whether qmp_input_get_object() succeeded. They crash > when it fails. Reproducer: > > { "execute": "qom-set", > "arguments": { "path": "/machine", "property": "rtc-time" } } > > Will crash with: > > qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj' > failed > > Broken in commit 5c678ee. Fix by adding the missing error checks. > > Also: > Reviewed-by: Markus Armbruster <armbru@redhat.com> Looks good to me, thanks >
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 64dd392..fc91e74 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -338,6 +338,12 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, QmpInputVisitor *qiv = to_qiv(v); QObject *qobj = qmp_input_get_object(qiv, name, true); + if (!qobj) { + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + *obj = NULL; + return; + } + qobject_incref(qobj); *obj = qobj; } @@ -347,6 +353,11 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) QmpInputVisitor *qiv = to_qiv(v); QObject *qobj = qmp_input_get_object(qiv, name, true); + if (!qobj) { + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + return; + } + if (qobject_type(qobj) != QTYPE_QNULL) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "null");
Calling: { "execute": "qom-set", "arguments": { "path": "/machine", "property": "rtc-time" } } Will crash with: qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj' failed Clear the obj and return an error. The patch also fixes a similar potential crash in qmp_input_type_null() by checking qmp_input_get_object() returned a valid qobj. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- qapi/qmp-input-visitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)