| Message ID | 1397669978-26339-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/16/2014 01:39 PM, Eduardo Habkost wrote: > 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 <ehabkost@redhat.com> > --- > Changes v2: > * Change ordering: first check for TYPE_USER_CREATABLE and then check > if class is abstract. This makes the ordering of checks closer to > what's already done on device_add. Changes look fine to me, re-tested to verify. Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > --- > qmp.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/qmp.c b/qmp.c > index 87a28f7..9a93ab1 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_dynamic_cast(klass, TYPE_USER_CREATABLE)) { > + error_setg(errp, "object type '%s' isn't supported by object-add", > + type); > + return; > + } > + > + if (object_class_is_abstract(klass)) { > + error_setg(errp, "object type '%s' is abstract", type); > + return; > + } > + > obj = object_new(type); > if (qdict) { > for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { > @@ -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; >
On Wed, 16 Apr 2014 14:39:38 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > 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 <ehabkost@redhat.com> Applied to the qmp branch, thanks Eduardo. > --- > Changes v2: > * Change ordering: first check for TYPE_USER_CREATABLE and then check > if class is abstract. This makes the ordering of checks closer to > what's already done on device_add. > --- > qmp.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/qmp.c b/qmp.c > index 87a28f7..9a93ab1 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_dynamic_cast(klass, TYPE_USER_CREATABLE)) { > + error_setg(errp, "object type '%s' isn't supported by object-add", > + type); > + return; > + } > + > + if (object_class_is_abstract(klass)) { > + error_setg(errp, "object type '%s' is abstract", type); > + return; > + } > + > obj = object_new(type); > if (qdict) { > for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { > @@ -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;
Am 25.04.2014 17:12, schrieb Luiz Capitulino: > On Wed, 16 Apr 2014 14:39:38 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> 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 <ehabkost@redhat.com> > > Applied to the qmp branch, thanks Eduardo. > >> --- >> Changes v2: >> * Change ordering: first check for TYPE_USER_CREATABLE and then check >> if class is abstract. This makes the ordering of checks closer to >> what's already done on device_add. >> --- >> qmp.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/qmp.c b/qmp.c >> index 87a28f7..9a93ab1 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; Luiz, can you rename klass to oc please? Thanks, Andreas >> 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_dynamic_cast(klass, TYPE_USER_CREATABLE)) { >> + error_setg(errp, "object type '%s' isn't supported by object-add", >> + type); >> + return; >> + } >> + >> + if (object_class_is_abstract(klass)) { >> + error_setg(errp, "object type '%s' is abstract", type); >> + return; >> + } >> + >> obj = object_new(type); >> if (qdict) { >> for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { >> @@ -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; > >
On Fri, 25 Apr 2014 17:57:12 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 25.04.2014 17:12, schrieb Luiz Capitulino: > > On Wed, 16 Apr 2014 14:39:38 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > >> 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 <ehabkost@redhat.com> > > > > Applied to the qmp branch, thanks Eduardo. > > > >> --- > >> Changes v2: > >> * Change ordering: first check for TYPE_USER_CREATABLE and then check > >> if class is abstract. This makes the ordering of checks closer to > >> what's already done on device_add. > >> --- > >> qmp.c | 21 ++++++++++++++------- > >> 1 file changed, 14 insertions(+), 7 deletions(-) > >> > >> diff --git a/qmp.c b/qmp.c > >> index 87a28f7..9a93ab1 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; > > Luiz, can you rename klass to oc please? My pull request is almost ready (finishing last few tests), so I think it's a bit late for a style change. > > Thanks, > Andreas > > >> 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_dynamic_cast(klass, TYPE_USER_CREATABLE)) { > >> + error_setg(errp, "object type '%s' isn't supported by object-add", > >> + type); > >> + return; > >> + } > >> + > >> + if (object_class_is_abstract(klass)) { > >> + error_setg(errp, "object type '%s' is abstract", type); > >> + return; > >> + } > >> + > >> obj = object_new(type); > >> if (qdict) { > >> for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { > >> @@ -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; > > > > > >
Am 25.04.2014 20:02, schrieb Luiz Capitulino: > On Fri, 25 Apr 2014 17:57:12 +0200 > Andreas Färber <afaerber@suse.de> wrote: >> Am 25.04.2014 17:12, schrieb Luiz Capitulino: >>> On Wed, 16 Apr 2014 14:39:38 -0300 >>> Eduardo Habkost <ehabkost@redhat.com> wrote: >>> >>>> 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 <ehabkost@redhat.com> >>> >>> Applied to the qmp branch, thanks Eduardo. [...] >>>> diff --git a/qmp.c b/qmp.c >>>> index 87a28f7..9a93ab1 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; >> >> Luiz, can you rename klass to oc please? > > My pull request is almost ready (finishing last few tests), so I think > it's a bit late for a style change. Then either one of you please follow-up with a fix before you forget. I've been asked to change patches that way, so I expect others do, too. Andreas
On Fri, Apr 25, 2014 at 08:42:59PM +0200, Andreas Färber wrote: > Am 25.04.2014 20:02, schrieb Luiz Capitulino: > > On Fri, 25 Apr 2014 17:57:12 +0200 > > Andreas Färber <afaerber@suse.de> wrote: > >> Am 25.04.2014 17:12, schrieb Luiz Capitulino: > >>> On Wed, 16 Apr 2014 14:39:38 -0300 > >>> Eduardo Habkost <ehabkost@redhat.com> wrote: > >>> > >>>> 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 <ehabkost@redhat.com> > >>> > >>> Applied to the qmp branch, thanks Eduardo. > [...] > >>>> diff --git a/qmp.c b/qmp.c > >>>> index 87a28f7..9a93ab1 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; > >> > >> Luiz, can you rename klass to oc please? > > > > My pull request is almost ready (finishing last few tests), so I think > > it's a bit late for a style change. > > Then either one of you please follow-up with a fix before you forget. > I've been asked to change patches that way, so I expect others do, too. Could you explain why this is a bug? The patch matches the existing style in qmp.c, and if I grep the whole tree I see 482 matches for 'ObjectClass *klass' and 165 for 'ObjectClass *oc'.
Am 25.04.2014 21:16, schrieb Eduardo Habkost: > On Fri, Apr 25, 2014 at 08:42:59PM +0200, Andreas Färber wrote: >> Am 25.04.2014 20:02, schrieb Luiz Capitulino: >>> On Fri, 25 Apr 2014 17:57:12 +0200 >>> Andreas Färber <afaerber@suse.de> wrote: >>>> Am 25.04.2014 17:12, schrieb Luiz Capitulino: >>>>> On Wed, 16 Apr 2014 14:39:38 -0300 >>>>> Eduardo Habkost <ehabkost@redhat.com> wrote: >>>>> >>>>>> diff --git a/qmp.c b/qmp.c >>>>>> index 87a28f7..9a93ab1 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; >>>> >>>> Luiz, can you rename klass to oc please? >>> >>> My pull request is almost ready (finishing last few tests), so I think >>> it's a bit late for a style change. >> >> Then either one of you please follow-up with a fix before you forget. >> I've been asked to change patches that way, so I expect others do, too. > > Could you explain why this is a bug? The patch matches the existing > style in qmp.c, and if I grep the whole tree I see 482 matches for > 'ObjectClass *klass' and 165 for 'ObjectClass *oc'. Simple explanation for the numbers: "klass" was introduced through Anthony's Python-automated qdev -> QOM conversion, because C++ reserves "class". Then when I started making use of QOM for CPUs I was asked to change my copied "klass" to "oc", with the argument that "klass" is a deliberate misspelling and that we already used dc for DeviceClass, etc. Therefore I expect others to update their patches to the new style, too. ObjectClass *oc #define MY_CLASS(class) # or MY_CLASS(cls) -- not forbidden in macros Just like DO_UPCAST() and other pre-QOM macros, the only remedy to avoid bad copy&paste will be to pass over the source tree and fix it consistently. No time for that myself now, maybe a GSoC/OPW task? Regards, Andreas
diff --git a/qmp.c b/qmp.c index 87a28f7..9a93ab1 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_dynamic_cast(klass, TYPE_USER_CREATABLE)) { + error_setg(errp, "object type '%s' isn't supported by object-add", + type); + return; + } + + if (object_class_is_abstract(klass)) { + error_setg(errp, "object type '%s' is abstract", type); + return; + } + obj = object_new(type); if (qdict) { for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { @@ -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;
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 <ehabkost@redhat.com> --- Changes v2: * Change ordering: first check for TYPE_USER_CREATABLE and then check if class is abstract. This makes the ordering of checks closer to what's already done on device_add. --- qmp.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)