| Message ID | 1340800884-4571-1-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 27.06.2012 14:41, schrieb Anthony Liguori: > From Markus: > > Before: > > $ qemu-system-x86_64 -display none -drive if=ide > qemu-system-x86_64: Device needs media, but drive is empty > qemu-system-x86_64: Initialization of device ide-hd failed > [Exit 1 ] > > After: > > $ qemu-system-x86_64 -display none -drive if=ide > qemu-system-x86_64: Device needs media, but drive is empty > Segmentation fault (core dumped) > [Exit 139 (SIGSEGV)] > > This error always existed as qdev_init() frees the object. But QOM > goes a bit further and purposefully sets the class pointer to NULL to > help find use-after-free. It worked :-) > > Cc: Andreas Faerber <afaerber@suse.de> > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> Reviewed-by: Andreas Färber <afaerber@suse.de> This together with the semantics discussions we're having makes me think we should attack "QOM'ifying" qdev sooner than later. I.e., reviewing what naming, chaining, etc. we can already change to align the TYPE_DEVICE-derived types with the generic QOM infrastructure. Andreas
On 06/27/2012 08:14 AM, Andreas Färber wrote: > Am 27.06.2012 14:41, schrieb Anthony Liguori: >> From Markus: >> >> Before: >> >> $ qemu-system-x86_64 -display none -drive if=ide >> qemu-system-x86_64: Device needs media, but drive is empty >> qemu-system-x86_64: Initialization of device ide-hd failed >> [Exit 1 ] >> >> After: >> >> $ qemu-system-x86_64 -display none -drive if=ide >> qemu-system-x86_64: Device needs media, but drive is empty >> Segmentation fault (core dumped) >> [Exit 139 (SIGSEGV)] >> >> This error always existed as qdev_init() frees the object. But QOM >> goes a bit further and purposefully sets the class pointer to NULL to >> help find use-after-free. It worked :-) >> >> Cc: Andreas Faerber<afaerber@suse.de> >> Reported-by: Markus Armbruster<armbru@redhat.com> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > > Reviewed-by: Andreas Färber<afaerber@suse.de> > > This together with the semantics discussions we're having makes me think > we should attack "QOM'ifying" qdev sooner than later. I.e., reviewing > what naming, chaining, etc. we can already change to align the > TYPE_DEVICE-derived types with the generic QOM infrastructure. We really ought to find all uses of qdev_init_nofail() or qdev_try_init() (including the sysbus et al derivatives) and add explicit qdev_free() calls in the error handling case such that we can remove the qdev_free() calls that are done automatically in the init function. Destructing an object automagically in a virtual method is pretty darn evil and really promotes these sort of problems. Regards, Anthony Liguori > > Andreas >
Anthony Liguori <aliguori@us.ibm.com> writes: >>From Markus: > > Before: > > $ qemu-system-x86_64 -display none -drive if=ide > qemu-system-x86_64: Device needs media, but drive is empty > qemu-system-x86_64: Initialization of device ide-hd failed > [Exit 1 ] > > After: > > $ qemu-system-x86_64 -display none -drive if=ide > qemu-system-x86_64: Device needs media, but drive is empty > Segmentation fault (core dumped) > [Exit 139 (SIGSEGV)] > > This error always existed as qdev_init() frees the object. But QOM > goes a bit further and purposefully sets the class pointer to NULL to > help find use-after-free. It worked :-) > > Cc: Andreas Faerber <afaerber@suse.de> > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> Tested-by: Markus Armbruster <armbru@redhat.com>
Am 27.06.2012 15:44, schrieb Anthony Liguori: > On 06/27/2012 08:14 AM, Andreas Färber wrote: >> This together with the semantics discussions we're having makes me think >> we should attack "QOM'ifying" qdev sooner than later. I.e., reviewing >> what naming, chaining, etc. we can already change to align the >> TYPE_DEVICE-derived types with the generic QOM infrastructure. > > We really ought to find all uses of qdev_init_nofail() or > qdev_try_init() (including the sysbus et al derivatives) and add > explicit qdev_free() calls in the error handling case such that we can > remove the qdev_free() calls that are done automatically in the init > function. I strongly disagree: We should instead rip out qdev_free() and use object_delete(). "free" vs. "delete" is still a qdev'ism, no need to make things worse. No disagreement on adding explicit QOM-style calls. Regards, Andreas > > Destructing an object automagically in a virtual method is pretty darn > evil and really promotes these sort of problems. > > Regards, > > Anthony Liguori
diff --git a/hw/qdev.c b/hw/qdev.c index a6c4c02..af54467 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -258,9 +258,10 @@ int qdev_simple_unplug_cb(DeviceState *dev) way is somewhat unclean, and best avoided. */ void qdev_init_nofail(DeviceState *dev) { + const char *typename = object_get_typename(OBJECT(dev)); + if (qdev_init(dev) < 0) { - error_report("Initialization of device %s failed", - object_get_typename(OBJECT(dev))); + error_report("Initialization of device %s failed", typename); exit(1); } }
From Markus: Before: $ qemu-system-x86_64 -display none -drive if=ide qemu-system-x86_64: Device needs media, but drive is empty qemu-system-x86_64: Initialization of device ide-hd failed [Exit 1 ] After: $ qemu-system-x86_64 -display none -drive if=ide qemu-system-x86_64: Device needs media, but drive is empty Segmentation fault (core dumped) [Exit 139 (SIGSEGV)] This error always existed as qdev_init() frees the object. But QOM goes a bit further and purposefully sets the class pointer to NULL to help find use-after-free. It worked :-) Cc: Andreas Faerber <afaerber@suse.de> Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/qdev.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)