Paolo Bonzini <pbonz...@redhat.com> writes:
> On 20/05/20 10:11, Markus Armbruster wrote:
>>> On 19/05/20 16:54, Markus Armbruster wrote:
>>>> +
>>>> + object_ref(OBJECT(dev));
>>>> + object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>> + if (err) {
>>>> + error_propagate_prepend(errp, err,
>>>> + "Initialization of device %s failed: ",
>>>> + object_get_typename(OBJECT(dev)));
>>>> + }
>>>> + object_unref(OBJECT(dev));
>>> Why is the ref/unref pair needed? Should it be done in the realized
>>> setter instead?
>> Copied from qdev_init_nofail(), where it is necessary (I figured out why
>> the hard way). It doesn't seem to be necessary here, though. Thanks!
>
> Why is it necessary there? It seems a bit iffy.
My exact thoughts a few days back. One debugging session later, I
understood, and put them right back. Glad we have tests :)
When object_property_set_bool() fails in qdev_init_nofail(), the
reference count can drop to zero. Certainly surprised me. Have a look:
dev = qdev_create(bus, type_name);
// @dev is a weak reference, and @bus holds the only strong one
...
qdev_init_nofail(dev);
In qdev_init_nofail():
// object_ref(OBJECT(dev));
object_property_set_bool(OBJECT(dev), true, "realized", &err);
This is a fancy way to call device_set_realized(). If something goes
wrong there, we execute
fail:
error_propagate(errp, local_err);
if (unattached_parent) {
/*
* Beware, this doesn't just revert
* object_property_add_child(), it also runs bus_remove()!
*/
object_unparent(OBJECT(dev));
unattached_count--;
}
and bus_remove() drops the reference count to zero.
Back in qdev_init_nofail(), we then use after free:
if (err) {
error_reportf_err(err, "Initialization of device %s failed: ",
---> object_get_typename(OBJECT(dev)));
exit(1);
}
// object_unref(OBJECT(dev));
The ref/unref keeps around @dev long enough for adding @dev's type name
to the error message.
The equivalent new pattern doesn't have this issue:
dev = qdev_new(type_name);
// @dev is the only reference
...
qdev_realize_and_unref(dev, bus, errp);
In qdev_realize(), called via qdev_realize_and_unref():
qdev_set_parent_bus(dev, bus);
// @bus now holds the second reference
// object_ref(OBJECT(dev));
object_property_set_bool(OBJECT(dev), true, "realized", &err);
In device_set_realized(), the reference count drops to one, namely
@dev's reference. That one goes away only in qdev_realize_and_unref(),
after we added @dev's type name to the error message.
However, a boring drive to the supermarket gave me this scenario:
dev = qdev_new(type_name);
// @dev is the only reference
...
object_property_add_child(parent, name, OBJECT(dev));
// @parent holds the second reference
object_unref(dev);
// unusual, but not wrong; @parent holds the only reference now
...
qdev_realize(dev, bus, errp);
Here, the reference count can drop to zero when device_set_realized()
fails, and qdev_realize()'s object_get_typename() is a use after free.
Best to keep the ref/unref, I think.
