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

[Thomas Huth]

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.

 Thomas