Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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.