Re: [Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Mon, Jul 16, 2018 at 09:09:13AM +0200, Thomas Huth wrote:
> On 13.07.2018 23:26, Eduardo Habkost wrote:
> > On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth wrote:
> >> When trying to "device_add bcm2837" on a machine that is not suitable for
> >> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":
> >>
> >> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
> >>  "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " 
> >> \
> >>  "'arguments': {'command-line': 'info qtree'}}" | \
> >>  aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp 
> >> stdio
> >>
> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
> >>  "package": "build-all"}, "capabilities": []}}
> >> {"return": {}}
> >> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
> >>  hotplugged on this machine"}}
> >> Segmentation fault (core dumped)
> >>
> >> The qdev_set_parent_bus() from instance_init adds a link to the child 
> >> devices
> >> which is not valid anymore after the bcm2837 instance has been destroyed.
> >> Unfortunately, the child devices do not get destroyed / unlinked correctly
> >> because both object_initialize() and object_property_add_child() increase
> >> the reference count of the child objects by one, but only one reference
> >> is dropped when the parent gets removed. So let's use the new functions
> >> object_initialize_child() and sysbus_init_child_obj() instead to create
> >> the objects, which will take care of creating the child objects with the
> >> correct reference count of one.
> >>
> >> Signed-off-by: Thomas Huth <address@hidden>
> > 
> > Reviewed-by: Eduardo Habkost <address@hidden>
> > 
> > The usage of &error_abort in code that can be triggered from
> > device-list-properties still makes me nervous, but that's a
> > separate issue.
> 
> I first had similar thoughts, but I think it's a clear coding issue if
> the abort triggers here (and not something that the user should normally
> be able to trigger somehow), so error_abort should be ok in this case.

Agreed.  We don't really have an option, as instance_init can't
report errors.

-- 
Eduardo