diff mbox series

hw/core/qdev-clock: add a reference on aliased clocks

Message ID 20201020074426.105878-1-luc@lmichel.fr
State New
Headers show
Series hw/core/qdev-clock: add a reference on aliased clocks | expand

Commit Message

Luc Michel Oct. 20, 2020, 7:44 a.m. UTC
When aliasing a clock with the qdev_alias_clock() function, a new link
property is created on the device aliasing the clock. The link points
to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This
property is read only since it does not provide a check callback for
modifications.

The object_property_add_link() documentation stats that with
OBJ_PROP_LINK_STRONG properties, the linked object reference count get
decremented when the property is deleted. But it is _not_ incremented on
creation (object_property_add_link() does not actually know the link).

This commit increments the reference count on the aliased clock to
ensure the aliased clock stays alive during the property lifetime, and
to avoid a double-free memory error when the property get deleted.

Signed-off-by: Luc Michel <luc@lmichel.fr>
---
 hw/core/qdev-clock.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Philippe Mathieu-Daudé Oct. 20, 2020, 7:58 a.m. UTC | #1
Cc'ing Markus/Marc-André too.

On 10/20/20 9:44 AM, Luc Michel wrote:
> When aliasing a clock with the qdev_alias_clock() function, a new link

> property is created on the device aliasing the clock. The link points

> to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This

> property is read only since it does not provide a check callback for

> modifications.

> 

> The object_property_add_link() documentation stats that with

> OBJ_PROP_LINK_STRONG properties, the linked object reference count get

> decremented when the property is deleted. But it is _not_ incremented on

> creation (object_property_add_link() does not actually know the link).

> 

> This commit increments the reference count on the aliased clock to

> ensure the aliased clock stays alive during the property lifetime, and

> to avoid a double-free memory error when the property get deleted.

> 

> Signed-off-by: Luc Michel <luc@lmichel.fr>

> ---

>   hw/core/qdev-clock.c | 1 +

>   1 file changed, 1 insertion(+)

> 

> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c

> index 6a9a340d0f..5f5e143702 100644

> --- a/hw/core/qdev-clock.c

> +++ b/hw/core/qdev-clock.c

> @@ -59,10 +59,11 @@ static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,

>       } else {

>           object_property_add_link(OBJECT(dev), name,

>                                    object_get_typename(OBJECT(clk)),

>                                    (Object **) &ncl->clock,

>                                    NULL, OBJ_PROP_LINK_STRONG);

> +        object_ref(OBJECT(clk));

>       }

>   

>       ncl->clock = clk;

>   

>       QLIST_INSERT_HEAD(&dev->clocks, ncl, node);

>
Philippe Mathieu-Daudé Oct. 20, 2020, 8:32 a.m. UTC | #2
On 10/20/20 9:58 AM, Philippe Mathieu-Daudé wrote:
> Cc'ing Markus/Marc-André too.

> 

> On 10/20/20 9:44 AM, Luc Michel wrote:

>> When aliasing a clock with the qdev_alias_clock() function, a new link

>> property is created on the device aliasing the clock. The link points

>> to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This

>> property is read only since it does not provide a check callback for

>> modifications.

>>

>> The object_property_add_link() documentation stats that with

>> OBJ_PROP_LINK_STRONG properties, the linked object reference count get

>> decremented when the property is deleted. But it is _not_ incremented on

>> creation (object_property_add_link() does not actually know the link).

>>

>> This commit increments the reference count on the aliased clock to

>> ensure the aliased clock stays alive during the property lifetime, and

>> to avoid a double-free memory error when the property get deleted.

>>

>> Signed-off-by: Luc Michel <luc@lmichel.fr>

>> ---

>>   hw/core/qdev-clock.c | 1 +

>>   1 file changed, 1 insertion(+)

>>

>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c

>> index 6a9a340d0f..5f5e143702 100644

>> --- a/hw/core/qdev-clock.c

>> +++ b/hw/core/qdev-clock.c

>> @@ -59,10 +59,11 @@ static NamedClockList 

>> *qdev_init_clocklist(DeviceState *dev, const char *name,

>>       } else {

>>           object_property_add_link(OBJECT(dev), name,

>>                                    object_get_typename(OBJECT(clk)),

>>                                    (Object **) &ncl->clock,

>>                                    NULL, OBJ_PROP_LINK_STRONG);

>> +        object_ref(OBJECT(clk));


OK, this is particular to this model because device_finalize()
garbage-collector and calls qdev_finalize_clocklist().

With a comment explaining why we need this call:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


>>       }

>>       ncl->clock = clk;

>>       QLIST_INSERT_HEAD(&dev->clocks, ncl, node);

>>

>
diff mbox series

Patch

diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index 6a9a340d0f..5f5e143702 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -59,10 +59,11 @@  static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
     } else {
         object_property_add_link(OBJECT(dev), name,
                                  object_get_typename(OBJECT(clk)),
                                  (Object **) &ncl->clock,
                                  NULL, OBJ_PROP_LINK_STRONG);
+        object_ref(OBJECT(clk));
     }
 
     ncl->clock = clk;
 
     QLIST_INSERT_HEAD(&dev->clocks, ncl, node);