On Wed, Apr 16, 2014 at 08:53:23AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
>
> > Currently it is very easy to crash QEMU by issuing an object-add command
> > using an abstract class or a class that doesn't support
> > TYPE_USER_CREATABLE as parameter.
> >
> > Example: with the following QMP command:
> >
> > (QEMU) object-add qom-type=cpu id=foo
> >
> > QEMU aborts at:
> >
> > ERROR:qom/object.c:335:object_initialize_with_type: assertion failed:
> > (type->abstract == false)
> >
> > This patch moves the check for TYPE_USER_CREATABLE before object_new(),
> > and adds a check to prevent the code from trying to instantiate abstract
> > classes.
> >
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
>
> Related device_add fixes:
>
> 061e84f qdev-monitor: Avoid device_add crashing on non-device driver name
> 2fa4e56 qdev-monitor: Fix crash when device_add is called with abstract driver
> 7ea5e78 qdev: Do not let the user try to device_add when it cannot work
>
> > ---
> > qmp.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/qmp.c b/qmp.c
> > index 87a28f7..e7ecbb7 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id,
> > const QDict *qdict,
> > Visitor *v, Error **errp)
> > {
> > Object *obj;
> > + ObjectClass *klass;
> > const QDictEntry *e;
> > Error *local_err = NULL;
> >
> > - if (!object_class_by_name(type)) {
> > + klass = object_class_by_name(type);
> > + if (!klass) {
> > error_setg(errp, "invalid class name");
> > return;
> > }
> >
> > + if (object_class_is_abstract(klass)) {
> > + error_setg(errp, "object type '%s' is abstract", type);
> > + return;
> > + }
> > +
> > + if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
> > + error_setg(errp, "object type '%s' isn't supported by object-add",
> > + type);
> > + return;
> > + }
> > +
> > obj = object_new(type);
> > if (qdict) {
> > for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
>
> For comparison, this is qdev_device_add():
>
> oc = object_class_by_name(driver);
> if (!oc) {
> const char *typename = find_typename_by_alias(driver);
>
> if (typename) {
> driver = typename;
> oc = object_class_by_name(driver);
> }
> }
>
> if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> qerror_report(ERROR_CLASS_GENERIC_ERROR,
> "'%s' is not a valid device model name", driver);
> return NULL;
> }
>
> if (object_class_is_abstract(oc)) {
> qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> "non-abstract device type");
> return NULL;
> }
>
> dc = DEVICE_CLASS(oc);
> if (dc->cannot_instantiate_with_device_add_yet) {
> qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> "pluggable device type");
> return NULL;
> }
>
> Got the "subtype of" and the "not abstract" check in the opposite order,
> and the additional cannot_instantiate_with_device_add_yet check.
>
> Does the different order matter?
The order probably doesn't matter very much. But it makes sense to keep
it similar to device_add for consistency. I will send v2.
> > @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id,
> > const QDict *qdict,
> > }
> > }
> >
> > - if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > - error_setg(&local_err, "object type '%s' isn't supported by
> > object-add",
> > - type);
> > - goto out;
> > - }
> > -
> > user_creatable_complete(obj, &local_err);
> > if (local_err) {
> > goto out;
--
Eduardo
