Message ID | 20160921103629.6410-2-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
On Wed 21 Sep 2016 12:36:27 PM CEST, Marc-André Lureau wrote: > The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing > parameters, but the qapi qmp_dispatch() code uses > QERR_INVALID_PARAMETER_TYPE. > > Improve qapi code to return QERR_INVALID_PARAMETER_TYPE where > appropriate. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing > parameters, but the qapi qmp_dispatch() code uses > QERR_INVALID_PARAMETER_TYPE. > > Improve qapi code to return QERR_INVALID_PARAMETER_TYPE where > appropriate. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/qmp-input-visitor.c | 109 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 73 insertions(+), 36 deletions(-) > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 64dd392..ea9972d 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -56,7 +56,7 @@ static QmpInputVisitor *to_qiv(Visitor *v) > > static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > const char *name, > - bool consume) > + bool consume, Error **errp) > { > StackObject *tos; > QObject *qobj; > @@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > > if (QSLIST_EMPTY(&qiv->stack)) { > /* Starting at root, name is ignored. */ > - return qiv->root; > - } First case: not in a container. qiv->root cannot be null. The old code is relatively clear: it returns this non-null value. Callers rely on it being non-null. The new code muddies the waters: it handles the impossible null value by setting an error with a misleading message, then returns null. Please go back to the old code and simply return qiv->root. You may assert it's non-null. > - > - /* We are in a container; find the next element. */ > - tos = QSLIST_FIRST(&qiv->stack); > - qobj = tos->obj; > - assert(qobj); > - > - if (qobject_type(qobj) == QTYPE_QDICT) { > - assert(name); > - ret = qdict_get(qobject_to_qdict(qobj), name); > - if (tos->h && consume && ret) { > - bool removed = g_hash_table_remove(tos->h, name); > - assert(removed); > - } > + ret = qiv->root; > } else { > - assert(qobject_type(qobj) == QTYPE_QLIST); > - assert(!name); > - ret = qlist_entry_obj(tos->entry); > - if (consume) { > - tos->entry = qlist_next(tos->entry); > + /* We are in a container; find the next element. */ > + tos = QSLIST_FIRST(&qiv->stack); > + qobj = tos->obj; > + assert(qobj); > + > + if (qobject_type(qobj) == QTYPE_QDICT) { > + assert(name); > + ret = qdict_get(qobject_to_qdict(qobj), name); > + if (tos->h && consume && ret) { > + bool removed = g_hash_table_remove(tos->h, name); > + assert(removed); > + } > + } else { > + assert(qobject_type(qobj) == QTYPE_QLIST); > + assert(!name); > + ret = qlist_entry_obj(tos->entry); > + if (consume) { > + tos->entry = qlist_next(tos->entry); > + } > } > } > > + if (!ret) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + } > + > return ret; > } Clearer with whitespace differences ignored: @@ -64,9 +64,8 @@ if (QSLIST_EMPTY(&qiv->stack)) { /* Starting at root, name is ignored. */ - return qiv->root; - } - + ret = qiv->root; + } else { /* We are in a container; find the next element. */ tos = QSLIST_FIRST(&qiv->stack); qobj = tos->obj; assert(qobj); if (qobject_type(qobj) == QTYPE_QDICT) { assert(name); ret = qdict_get(qobject_to_qdict(qobj), name); if (tos->h && consume && ret) { bool removed = g_hash_table_remove(tos->h, name); assert(removed); } } else { assert(qobject_type(qobj) == QTYPE_QLIST); assert(!name); ret = qlist_entry_obj(tos->entry); if (consume) { tos->entry = qlist_next(tos->entry); } } + } + + if (!ret) { + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + } return ret; } Two more cases: * In a QTYPE_QDICT container: If ret = qdict_get(qobject_to_qdict(qobj, name) is null, parameter name is missing, and we want to error_setg(errp, QERR_MISSING_PARAMETER, name). No ternary, because name can't be null. * In a QTYPE_QLIST container: ret = qlist_entry_obj(tos->entry) is the list member, a QObject. It must not be null because null is not a valid QObject. If we want to catch this, we should assert, not set an error with a misleading message. Note for the rest of the review: we return null excactly when we set an error. > > @@ -163,13 +167,16 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, true); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > Error *err = NULL; > > if (obj) { > *obj = NULL; > } > - if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > + if (!qobj) { > + return; > + } > + if (qobject_type(qobj) != QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "QDict"); > return; Mechanical; the next hunk is the same pattern. > @@ -191,10 +198,13 @@ static void qmp_input_start_list(Visitor *v, const char *name, > GenericList **list, size_t size, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, true); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > const QListEntry *entry; > > - if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { > + if (!qobj) { > + return; > + } > + if (qobject_type(qobj) != QTYPE_QLIST) { > if (list) { > *list = NULL; > } > @@ -232,11 +242,12 @@ static void qmp_input_start_alternate(Visitor *v, const char *name, > bool promote_int, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, false); > + QObject *qobj = qmp_input_get_object(qiv, name, false, errp); > > - if (!qobj) { > + if (obj) { > *obj = NULL; > - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + } > + if (!qobj) { > return; > } > *obj = g_malloc0(size); Why are you deviating from the mechanical change here? Note that obj can't be null here, by function contract. If called via visit_start_alternate() as it should be, the contract is enforced there. > @@ -250,8 +261,12 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj, > Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > + QInt *qint = qobject_to_qint(qobj); > > + if (!qobj) { > + return; > + } I'd call qobject_to_qint() here, not least for consistency with qmp_input_type_number(). Of course, your code works, and if you feel strongly about it, we can do it your way here. > if (!qint) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "integer"); Mechanical; the next few hunks are the same pattern. > @@ -266,8 +281,12 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj, > { > /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ > QmpInputVisitor *qiv = to_qiv(v); > - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > + QInt *qint = qobject_to_qint(qobj); > > + if (!qobj) { > + return; > + } > if (!qint) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "integer"); > @@ -281,8 +300,12 @@ static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj, > Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true)); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > + QBool *qbool = qobject_to_qbool(qobj); > > + if (!qobj) { > + return; > + } > if (!qbool) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "boolean"); > @@ -296,8 +319,12 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, > Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > + QString *qstr = qobject_to_qstring(qobj); > > + if (!qobj) { > + return; > + } > if (!qstr) { > *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > @@ -312,10 +339,13 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj, > Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, true); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > QInt *qint; > QFloat *qfloat; > > + if (!qobj) { > + return; > + } > qint = qobject_to_qint(qobj); > if (qint) { > *obj = qint_get_int(qobject_to_qint(qobj)); > @@ -336,7 +366,11 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, > Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, true); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > + > + if (!qobj) { > + return; > + } > > qobject_incref(qobj); > *obj = qobj; Aha, we got a different bug fix! The old code fails to fail when the parameter doesn't exist. Instead, it sets *obj = NULL, which seems very likely to crash QEMU. Let me try... yup: { "execute": "object-add", "arguments": { "qom-type": "memory-backend-file", "id": "foo" } } Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type: Assertion `qdict' failed." Either fix this in a separate patch before this one, or cover it in this one's commit message. Your choice. A separate patch might be usable for qemu-stable. > @@ -345,8 +379,11 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, > 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); > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + if (!qobj) { > + return; > + } > if (qobject_type(qobj) != QTYPE_QNULL) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "null"); Same bug, I think, but I don't have a reproducer handy. > @@ -356,7 +393,7 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) > static void qmp_input_optional(Visitor *v, const char *name, bool *present) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, false); > + QObject *qobj = qmp_input_get_object(qiv, name, false, NULL); > > if (!qobj) { > *present = false; Thanks for following my suggestion to move the "Parameter FOO is missing" error into qmp_input_get_object()! You fixed two crash bugs that way :)
Hi ----- Original Message ----- > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing > > parameters, but the qapi qmp_dispatch() code uses > > QERR_INVALID_PARAMETER_TYPE. > > > > Improve qapi code to return QERR_INVALID_PARAMETER_TYPE where > > appropriate. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > qapi/qmp-input-visitor.c | 109 > > +++++++++++++++++++++++++++++++---------------- > > 1 file changed, 73 insertions(+), 36 deletions(-) > > > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > > index 64dd392..ea9972d 100644 > > --- a/qapi/qmp-input-visitor.c > > +++ b/qapi/qmp-input-visitor.c > > @@ -56,7 +56,7 @@ static QmpInputVisitor *to_qiv(Visitor *v) > > > > static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > > const char *name, > > - bool consume) > > + bool consume, Error **errp) > > { > > StackObject *tos; > > QObject *qobj; > > @@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor > > *qiv, > > > > if (QSLIST_EMPTY(&qiv->stack)) { > > /* Starting at root, name is ignored. */ > > - return qiv->root; > > - } > > First case: not in a container. > > qiv->root cannot be null. > > The old code is relatively clear: it returns this non-null value. > Callers rely on it being non-null. I didn't realize that, ok > > The new code muddies the waters: it handles the impossible null value by > setting an error with a misleading message, then returns null. > > Please go back to the old code and simply return qiv->root. You may > assert it's non-null. ok > Two more cases: > > * In a QTYPE_QDICT container: > > If ret = qdict_get(qobject_to_qdict(qobj, name) is null, parameter > name is missing, and we want to > error_setg(errp, QERR_MISSING_PARAMETER, name). No ternary, because > name can't be null. > > * In a QTYPE_QLIST container: > > ret = qlist_entry_obj(tos->entry) is the list member, a QObject. It > must not be null because null is not a valid QObject. If we want to > catch this, we should assert, not set an error with a misleading > message. > > Note for the rest of the review: we return null excactly when we set an > error. > ok > > > > @@ -163,13 +167,16 @@ static void qmp_input_start_struct(Visitor *v, const > > char *name, void **obj, > > size_t size, Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, true); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > Error *err = NULL; > > > > if (obj) { > > *obj = NULL; > > } > > - if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > > + if (!qobj) { > > + return; > > + } > > + if (qobject_type(qobj) != QTYPE_QDICT) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "QDict"); > > return; > > Mechanical; the next hunk is the same pattern. > > > @@ -191,10 +198,13 @@ static void qmp_input_start_list(Visitor *v, const > > char *name, > > GenericList **list, size_t size, Error > > **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, true); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > const QListEntry *entry; > > > > - if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { > > + if (!qobj) { > > + return; > > + } > > + if (qobject_type(qobj) != QTYPE_QLIST) { > > if (list) { > > *list = NULL; > > } > > @@ -232,11 +242,12 @@ static void qmp_input_start_alternate(Visitor *v, > > const char *name, > > bool promote_int, Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, false); > > + QObject *qobj = qmp_input_get_object(qiv, name, false, errp); > > > > - if (!qobj) { > > + if (obj) { > > *obj = NULL; > > - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > > + } > > + if (!qobj) { > > return; > > } > > *obj = g_malloc0(size); > > Why are you deviating from the mechanical change here? Because there is already a QERR_MISSING_PARAMETER return. > > Note that obj can't be null here, by function contract. If called via > visit_start_alternate() as it should be, the contract is enforced there. ok > > > @@ -250,8 +261,12 @@ static void qmp_input_type_int64(Visitor *v, const > > char *name, int64_t *obj, > > Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + QInt *qint = qobject_to_qint(qobj); > > > > + if (!qobj) { > > + return; > > + } > > I'd call qobject_to_qint() here, not least for consistency with > qmp_input_type_number(). Of course, your code works, and if you feel > strongly about it, we can do it your way here. ok > > > if (!qint) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "integer"); > > Mechanical; the next few hunks are the same pattern. > > > @@ -266,8 +281,12 @@ static void qmp_input_type_uint64(Visitor *v, const > > char *name, uint64_t *obj, > > { > > /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ > > QmpInputVisitor *qiv = to_qiv(v); > > - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + QInt *qint = qobject_to_qint(qobj); > > > > + if (!qobj) { > > + return; > > + } > > if (!qint) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "integer"); > > @@ -281,8 +300,12 @@ static void qmp_input_type_bool(Visitor *v, const char > > *name, bool *obj, > > Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, > > true)); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + QBool *qbool = qobject_to_qbool(qobj); > > > > + if (!qobj) { > > + return; > > + } > > if (!qbool) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "boolean"); > > @@ -296,8 +319,12 @@ static void qmp_input_type_str(Visitor *v, const char > > *name, char **obj, > > Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, > > true)); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + QString *qstr = qobject_to_qstring(qobj); > > > > + if (!qobj) { > > + return; > > + } > > if (!qstr) { > > *obj = NULL; > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > @@ -312,10 +339,13 @@ static void qmp_input_type_number(Visitor *v, const > > char *name, double *obj, > > Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, true); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > QInt *qint; > > QFloat *qfloat; > > > > + if (!qobj) { > > + return; > > + } > > qint = qobject_to_qint(qobj); > > if (qint) { > > *obj = qint_get_int(qobject_to_qint(qobj)); > > @@ -336,7 +366,11 @@ static void qmp_input_type_any(Visitor *v, const char > > *name, QObject **obj, > > Error **errp) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, true); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + > > + if (!qobj) { > > + return; > > + } > > > > qobject_incref(qobj); > > *obj = qobj; > > Aha, we got a different bug fix! The old code fails to fail when the > parameter doesn't exist. Instead, it sets *obj = NULL, which seems very > likely to crash QEMU. Let me try... yup: > > { "execute": "object-add", > "arguments": { "qom-type": "memory-backend-file", "id": "foo" } } > > Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type: > Assertion `qdict' failed." > > Either fix this in a separate patch before this one, or cover it in this > one's commit message. Your choice. ok, I'll make a seperate patch > > A separate patch might be usable for qemu-stable. > > > @@ -345,8 +379,11 @@ static void qmp_input_type_any(Visitor *v, const char > > *name, QObject **obj, > > 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); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > > > + if (!qobj) { > > + return; > > + } > > if (qobject_type(qobj) != QTYPE_QNULL) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "null"); > > Same bug, I think, but I don't have a reproducer handy. let's include it in the same patch > > > @@ -356,7 +393,7 @@ static void qmp_input_type_null(Visitor *v, const char > > *name, Error **errp) > > static void qmp_input_optional(Visitor *v, const char *name, bool > > *present) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, false); > > + QObject *qobj = qmp_input_get_object(qiv, name, false, NULL); > > > > if (!qobj) { > > *present = false; > > Thanks for following my suggestion to move the "Parameter FOO is > missing" error into qmp_input_get_object()! You fixed two crash bugs > that way :) >
Hi ----- Original Message ----- > Aha, we got a different bug fix! The old code fails to fail when the > parameter doesn't exist. Instead, it sets *obj = NULL, which seems very > likely to crash QEMU. Let me try... yup: > > { "execute": "object-add", > "arguments": { "qom-type": "memory-backend-file", "id": "foo" } } > > Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type: > Assertion `qdict' failed." > > Either fix this in a separate patch before this one, or cover it in this > one's commit message. Your choice. > > A separate patch might be usable for qemu-stable. It looks to me that this is a different bug. visit_type_q_obj_object_add_arg_members() doesn't call visit_type_any() if "props" is missing (it's optionnal). And arg is zero'ed in qmp-marshal, and the assert() was added in ad739706bbadee49. I am trying to fix that regression. > > > @@ -345,8 +379,11 @@ static void qmp_input_type_any(Visitor *v, const char > > *name, QObject **obj, > > 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); > > + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > > > + if (!qobj) { > > + return; > > + } > > if (qobject_type(qobj) != QTYPE_QNULL) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "null"); > > Same bug, I think, but I don't have a reproducer handy. > > > @@ -356,7 +393,7 @@ static void qmp_input_type_null(Visitor *v, const char > > *name, Error **errp) > > static void qmp_input_optional(Visitor *v, const char *name, bool > > *present) > > { > > QmpInputVisitor *qiv = to_qiv(v); > > - QObject *qobj = qmp_input_get_object(qiv, name, false); > > + QObject *qobj = qmp_input_get_object(qiv, name, false, NULL); > > > > if (!qobj) { > > *present = false; > > Thanks for following my suggestion to move the "Parameter FOO is > missing" error into qmp_input_get_object()! You fixed two crash bugs > that way :) >
Marc-André Lureau <mlureau@redhat.com> writes: > Hi > > ----- Original Message ----- >> Aha, we got a different bug fix! The old code fails to fail when the >> parameter doesn't exist. Instead, it sets *obj = NULL, which seems very >> likely to crash QEMU. Let me try... yup: >> >> { "execute": "object-add", >> "arguments": { "qom-type": "memory-backend-file", "id": "foo" } } >> >> Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_type: >> Assertion `qdict' failed." >> >> Either fix this in a separate patch before this one, or cover it in this >> one's commit message. Your choice. >> >> A separate patch might be usable for qemu-stable. > > It looks to me that this is a different bug. > > visit_type_q_obj_object_add_arg_members() doesn't call visit_type_any() if "props" is missing (it's optionnal). > > And arg is zero'ed in qmp-marshal, and the assert() was added in ad739706bbadee49. I am trying to fix that regression. Okay, that's *also* a bug. For the bug I spotted, try { "execute": "qom-set", "arguments": { "path": "/machine", "property": "rtc-time" } } Trips assert(!err != !*obj) in its caller visit_type_any(). [...]
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 64dd392..ea9972d 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -56,7 +56,7 @@ static QmpInputVisitor *to_qiv(Visitor *v) static QObject *qmp_input_get_object(QmpInputVisitor *qiv, const char *name, - bool consume) + bool consume, Error **errp) { StackObject *tos; QObject *qobj; @@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, if (QSLIST_EMPTY(&qiv->stack)) { /* Starting at root, name is ignored. */ - return qiv->root; - } - - /* We are in a container; find the next element. */ - tos = QSLIST_FIRST(&qiv->stack); - qobj = tos->obj; - assert(qobj); - - if (qobject_type(qobj) == QTYPE_QDICT) { - assert(name); - ret = qdict_get(qobject_to_qdict(qobj), name); - if (tos->h && consume && ret) { - bool removed = g_hash_table_remove(tos->h, name); - assert(removed); - } + ret = qiv->root; } else { - assert(qobject_type(qobj) == QTYPE_QLIST); - assert(!name); - ret = qlist_entry_obj(tos->entry); - if (consume) { - tos->entry = qlist_next(tos->entry); + /* We are in a container; find the next element. */ + tos = QSLIST_FIRST(&qiv->stack); + qobj = tos->obj; + assert(qobj); + + if (qobject_type(qobj) == QTYPE_QDICT) { + assert(name); + ret = qdict_get(qobject_to_qdict(qobj), name); + if (tos->h && consume && ret) { + bool removed = g_hash_table_remove(tos->h, name); + assert(removed); + } + } else { + assert(qobject_type(qobj) == QTYPE_QLIST); + assert(!name); + ret = qlist_entry_obj(tos->entry); + if (consume) { + tos->entry = qlist_next(tos->entry); + } } } + if (!ret) { + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + } + return ret; } @@ -163,13 +167,16 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, true); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); Error *err = NULL; if (obj) { *obj = NULL; } - if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { + if (!qobj) { + return; + } + if (qobject_type(qobj) != QTYPE_QDICT) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "QDict"); return; @@ -191,10 +198,13 @@ static void qmp_input_start_list(Visitor *v, const char *name, GenericList **list, size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, true); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); const QListEntry *entry; - if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { + if (!qobj) { + return; + } + if (qobject_type(qobj) != QTYPE_QLIST) { if (list) { *list = NULL; } @@ -232,11 +242,12 @@ static void qmp_input_start_alternate(Visitor *v, const char *name, bool promote_int, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, false); + QObject *qobj = qmp_input_get_object(qiv, name, false, errp); - if (!qobj) { + if (obj) { *obj = NULL; - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + } + if (!qobj) { return; } *obj = g_malloc0(size); @@ -250,8 +261,12 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + QInt *qint = qobject_to_qint(qobj); + if (!qobj) { + return; + } if (!qint) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "integer"); @@ -266,8 +281,12 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj, { /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ QmpInputVisitor *qiv = to_qiv(v); - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + QInt *qint = qobject_to_qint(qobj); + if (!qobj) { + return; + } if (!qint) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "integer"); @@ -281,8 +300,12 @@ static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + QBool *qbool = qobject_to_qbool(qobj); + if (!qobj) { + return; + } if (!qbool) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "boolean"); @@ -296,8 +319,12 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + QString *qstr = qobject_to_qstring(qobj); + if (!qobj) { + return; + } if (!qstr) { *obj = NULL; error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", @@ -312,10 +339,13 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, true); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); QInt *qint; QFloat *qfloat; + if (!qobj) { + return; + } qint = qobject_to_qint(qobj); if (qint) { *obj = qint_get_int(qobject_to_qint(qobj)); @@ -336,7 +366,11 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, true); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + + if (!qobj) { + return; + } qobject_incref(qobj); *obj = qobj; @@ -345,8 +379,11 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, 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); + QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + if (!qobj) { + return; + } if (qobject_type(qobj) != QTYPE_QNULL) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "null"); @@ -356,7 +393,7 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) static void qmp_input_optional(Visitor *v, const char *name, bool *present) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, false); + QObject *qobj = qmp_input_get_object(qiv, name, false, NULL); if (!qobj) { *present = false;
The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing parameters, but the qapi qmp_dispatch() code uses QERR_INVALID_PARAMETER_TYPE. Improve qapi code to return QERR_INVALID_PARAMETER_TYPE where appropriate. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- qapi/qmp-input-visitor.c | 109 +++++++++++++++++++++++++++++++---------------- 1 file changed, 73 insertions(+), 36 deletions(-)