hw: fix some leaks in xlnx devices

[PATCH 2/2] hw/display/xlnx_dp: Don't leak dpcd and edid objects

Posted by Peter Maydell 1 month ago

In the xnlx_dp_init() function we create the s->dpcd and
s->edid objects with qdev_new(); then in xlnx_dp_realize()
we realize the dpcd with qdev_realize() and the edid with
qdev_realize_and_unref().

This is inconsistent, and both ways result in a memory
leak for the instance_init -> deinit lifecycle tested
by device-introspect-test:

Indirect leak of 1968 byte(s) in 1 object(s) allocated from:
    #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c5
3fecd904ba5fc1f521d7da080a0e4103b)
    #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
    #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
    #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
    #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
    #5 0x5aded54458be in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1272:20

Direct leak of 344 byte(s) in 1 object(s) allocated from:
    #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c53fecd904ba5fc1f521d7da080a0e4103b)
    #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
    #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
    #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
    #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
    #5 0x5aded5445a56 in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1275:22

Instead, explicitly object_unref() after we have added the objects as
child properties of the device.  This means they will automatically
be freed when this device is deinited.  When we do this,
qdev_realize() is the correct way to realize them in
xlnx_dp_realize().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/xlnx_dp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 7c980ee6423..ef73e1815fc 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1267,14 +1267,18 @@ static void xlnx_dp_init(Object *obj)
     s->aux_bus = aux_bus_init(DEVICE(obj), "aux");
 
     /*
-     * Initialize DPCD and EDID..
+     * Initialize DPCD and EDID. Once we have added the objects as
+     * child properties of this device, we can drop the reference we
+     * hold to them, leaving the child-property as the only reference.
      */
     s->dpcd = DPCD(qdev_new("dpcd"));
     object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd));
+    object_unref(s->dpcd);
 
     s->edid = I2CDDC(qdev_new("i2c-ddc"));
     i2c_slave_set_address(I2C_SLAVE(s->edid), 0x50);
     object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid));
+    object_unref(s->edid);
 
     fifo8_create(&s->rx_fifo, 16);
     fifo8_create(&s->tx_fifo, 16);
@@ -1311,8 +1315,8 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
     qdev_realize(DEVICE(s->dpcd), BUS(s->aux_bus), &error_fatal);
     aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
 
-    qdev_realize_and_unref(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
-                           &error_fatal);
+    qdev_realize(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
+                 &error_fatal);
 
     s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
     surface = qemu_console_surface(s->console);
-- 
2.43.0

Re: [PATCH 2/2] hw/display/xlnx_dp: Don't leak dpcd and edid objects

Posted by Francisco Iglesias 1 month ago

On Tue, Aug 26, 2025 at 06:49:56PM +0100, Peter Maydell wrote:
> In the xnlx_dp_init() function we create the s->dpcd and
> s->edid objects with qdev_new(); then in xlnx_dp_realize()
> we realize the dpcd with qdev_realize() and the edid with
> qdev_realize_and_unref().
> 
> This is inconsistent, and both ways result in a memory
> leak for the instance_init -> deinit lifecycle tested
> by device-introspect-test:
> 
> Indirect leak of 1968 byte(s) in 1 object(s) allocated from:
>     #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c5
> 3fecd904ba5fc1f521d7da080a0e4103b)
>     #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>     #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
>     #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
>     #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
>     #5 0x5aded54458be in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1272:20
> 
> Direct leak of 344 byte(s) in 1 object(s) allocated from:
>     #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c53fecd904ba5fc1f521d7da080a0e4103b)
>     #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>     #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
>     #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
>     #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
>     #5 0x5aded5445a56 in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1275:22
> 
> Instead, explicitly object_unref() after we have added the objects as
> child properties of the device.  This means they will automatically
> be freed when this device is deinited.  When we do this,
> qdev_realize() is the correct way to realize them in
> xlnx_dp_realize().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

> ---
>  hw/display/xlnx_dp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 7c980ee6423..ef73e1815fc 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1267,14 +1267,18 @@ static void xlnx_dp_init(Object *obj)
>      s->aux_bus = aux_bus_init(DEVICE(obj), "aux");
>  
>      /*
> -     * Initialize DPCD and EDID..
> +     * Initialize DPCD and EDID. Once we have added the objects as
> +     * child properties of this device, we can drop the reference we
> +     * hold to them, leaving the child-property as the only reference.
>       */
>      s->dpcd = DPCD(qdev_new("dpcd"));
>      object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd));
> +    object_unref(s->dpcd);
>  
>      s->edid = I2CDDC(qdev_new("i2c-ddc"));
>      i2c_slave_set_address(I2C_SLAVE(s->edid), 0x50);
>      object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid));
> +    object_unref(s->edid);
>  
>      fifo8_create(&s->rx_fifo, 16);
>      fifo8_create(&s->tx_fifo, 16);
> @@ -1311,8 +1315,8 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>      qdev_realize(DEVICE(s->dpcd), BUS(s->aux_bus), &error_fatal);
>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>  
> -    qdev_realize_and_unref(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
> -                           &error_fatal);
> +    qdev_realize(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
> +                 &error_fatal);
>  
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
> -- 
> 2.43.0
>

Re: [PATCH 2/2] hw/display/xlnx_dp: Don't leak dpcd and edid objects

Posted by Manos Pitsidianakis 1 month ago

On Tue, Aug 26, 2025 at 8:51 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the xnlx_dp_init() function we create the s->dpcd and
> s->edid objects with qdev_new(); then in xlnx_dp_realize()
> we realize the dpcd with qdev_realize() and the edid with
> qdev_realize_and_unref().
>
> This is inconsistent, and both ways result in a memory
> leak for the instance_init -> deinit lifecycle tested
> by device-introspect-test:
>
> Indirect leak of 1968 byte(s) in 1 object(s) allocated from:
>     #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c5
> 3fecd904ba5fc1f521d7da080a0e4103b)
>     #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>     #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
>     #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
>     #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
>     #5 0x5aded54458be in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1272:20
>
> Direct leak of 344 byte(s) in 1 object(s) allocated from:
>     #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c53fecd904ba5fc1f521d7da080a0e4103b)
>     #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>     #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
>     #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
>     #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
>     #5 0x5aded5445a56 in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1275:22
>
> Instead, explicitly object_unref() after we have added the objects as
> child properties of the device.  This means they will automatically
> be freed when this device is deinited.  When we do this,
> qdev_realize() is the correct way to realize them in
> xlnx_dp_realize().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/xlnx_dp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 7c980ee6423..ef73e1815fc 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1267,14 +1267,18 @@ static void xlnx_dp_init(Object *obj)
>      s->aux_bus = aux_bus_init(DEVICE(obj), "aux");
>
>      /*
> -     * Initialize DPCD and EDID..
> +     * Initialize DPCD and EDID. Once we have added the objects as
> +     * child properties of this device, we can drop the reference we
> +     * hold to them, leaving the child-property as the only reference.
>       */
>      s->dpcd = DPCD(qdev_new("dpcd"));
>      object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd));
> +    object_unref(s->dpcd);
>
>      s->edid = I2CDDC(qdev_new("i2c-ddc"));
>      i2c_slave_set_address(I2C_SLAVE(s->edid), 0x50);
>      object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid));
> +    object_unref(s->edid);
>
>      fifo8_create(&s->rx_fifo, 16);
>      fifo8_create(&s->tx_fifo, 16);
> @@ -1311,8 +1315,8 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>      qdev_realize(DEVICE(s->dpcd), BUS(s->aux_bus), &error_fatal);
>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>
> -    qdev_realize_and_unref(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
> -                           &error_fatal);
> +    qdev_realize(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
> +                 &error_fatal);
>
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
> --

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

> 2.43.0
>
>

Re: [PATCH 2/2] hw/display/xlnx_dp: Don't leak dpcd and edid objects

Posted by Edgar E. Iglesias 1 month ago

On Tue, Aug 26, 2025 at 06:49:56PM +0100, Peter Maydell wrote:
> In the xnlx_dp_init() function we create the s->dpcd and
> s->edid objects with qdev_new(); then in xlnx_dp_realize()
> we realize the dpcd with qdev_realize() and the edid with
> qdev_realize_and_unref().
> 
> This is inconsistent, and both ways result in a memory
> leak for the instance_init -> deinit lifecycle tested
> by device-introspect-test:
> 
> Indirect leak of 1968 byte(s) in 1 object(s) allocated from:
>     #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c5
> 3fecd904ba5fc1f521d7da080a0e4103b)
>     #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>     #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
>     #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
>     #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
>     #5 0x5aded54458be in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1272:20
> 
> Direct leak of 344 byte(s) in 1 object(s) allocated from:
>     #0 0x5aded4d54e23 in malloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-aarch64+0x24ffe23) (BuildId: 9f1e6c53fecd904ba5fc1f521d7da080a0e4103b)
>     #1 0x71fbfac9bb09 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>     #2 0x5aded7b9211c in object_new_with_type /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:767:15
>     #3 0x5aded7b92240 in object_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:789:12
>     #4 0x5aded7b773e4 in qdev_new /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/core/qdev.c:149:19
>     #5 0x5aded5445a56 in xlnx_dp_init /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/display/xlnx_dp.c:1275:22
> 
> Instead, explicitly object_unref() after we have added the objects as
> child properties of the device.  This means they will automatically
> be freed when this device is deinited.  When we do this,
> qdev_realize() is the correct way to realize them in
> xlnx_dp_realize().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> ---
>  hw/display/xlnx_dp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 7c980ee6423..ef73e1815fc 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1267,14 +1267,18 @@ static void xlnx_dp_init(Object *obj)
>      s->aux_bus = aux_bus_init(DEVICE(obj), "aux");
>  
>      /*
> -     * Initialize DPCD and EDID..
> +     * Initialize DPCD and EDID. Once we have added the objects as
> +     * child properties of this device, we can drop the reference we
> +     * hold to them, leaving the child-property as the only reference.
>       */
>      s->dpcd = DPCD(qdev_new("dpcd"));
>      object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd));
> +    object_unref(s->dpcd);
>  
>      s->edid = I2CDDC(qdev_new("i2c-ddc"));
>      i2c_slave_set_address(I2C_SLAVE(s->edid), 0x50);
>      object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid));
> +    object_unref(s->edid);
>  
>      fifo8_create(&s->rx_fifo, 16);
>      fifo8_create(&s->tx_fifo, 16);
> @@ -1311,8 +1315,8 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>      qdev_realize(DEVICE(s->dpcd), BUS(s->aux_bus), &error_fatal);
>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>  
> -    qdev_realize_and_unref(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
> -                           &error_fatal);
> +    qdev_realize(DEVICE(s->edid), BUS(aux_get_i2c_bus(s->aux_bus)),
> +                 &error_fatal);
>  
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
> -- 
> 2.43.0
>

[PATCH 1/2] hw/misc/xlnx-versal-cframe-reg: Free FIFO, g_tree on deinit
[PATCH 2/2] hw/display/xlnx_dp: Don't leak dpcd and edid objects