[PATCH 00/41] Fix various QOM object life-cycle issues

[PATCH 00/41] Fix various QOM object life-cycle issues

[Marc-André Lureau]

Hi,

After Markus's "Several QOM objects crash on introspection" report, I
started writing some unit test.

This series adds a new "qom-tests" qtest command that exercises basic QOM
object life-cycle: it instantiates all non-abstract object types, gets/sets
their properties, and unrefs them. This quickly surfaces leaks and crashes
that could otherwise be triggered at runtime via QMP qom commands.

The bulk of the series fixes the issues found by this test and ASan
help. Some of the patches are redundant with patches sent earlier on the
ML and marked as RFC, they should naturally be dropped during rebases,
but are added for completeness and to make sure CI pass after this
series in the meantime.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
Marc-André Lureau (41):
      hw/pci: handle missing bus in prop_pci_busnr_get
      chardev/char-socket: handle NULL addr in char_socket_get_addr
      hw/pci-bridge: handle missing parent in prop_pxb_uid_get
      hw/pci-host/i440fx: handle NULL bus in pci-hole64 getters
      hw/pci-host/q35: handle NULL bus in pci-hole64 getters
      hw/remote: check visit return in vfu_object_set_socket
      qom: skip link property check callback when clearing link
      hw/intc/apic: guard against NULL cpu in set_id
      RFC hw/remote: guard listener unregister in finalize
      backends/cryptodev-lkcf: skip cleanup when not initialized
      RFC io/net-listener: move mutex init to instance_init
      RFC net/colo-compare: guard finalize against uninitialized state
      RFC system/ioport: move embedded memory region cleanup to portio_list_destroy
      ui/console: remove console from global list on finalization
      hw/i386/x86: free oem_id and oem_table_id on finalization
      hw/core/machine: free shim_filename on finalization
      hw/core/resetcontainer: free children array on finalization
      net/filter: free old values in property setters
      ui/console-vc: destroy fifo on text console finalization
      target/i386/sev: add finalize functions and fix leaking setters
      target/i386/kvm/tdx: free strings in tdx_guest_finalize
      hw/i386/nitro_enclave: add instance finalize
      net/can: free ifname on socketcan finalization
      backends/igvm-cfg: free filename on finalization
      scsi/pr-manager-helper: free path on finalization
      accel/kvm: free device path on finalization
      system/qtest: free log path on finalization
      hw/i386/pc: free pcspk on finalization
      hw/fsi: move OPBus address space init to realize
      hw/gpio/pca9552: fix state_str leak in pca955x_set_led
      hw/arm/aspeed: free fmc_model and spi_model on finalization
      hw/tpm: free PPI buffer on finalization
      hw/arm/sbsa-ref: free unrealized flash devices on finalization
      hw/arm/virt: free flash devices and OEM strings on finalization
      hw/loongarch/virt: free flash devices and OEM strings on finalization
      hw/ppc/spapr: free host_model and host_serial on finalization
      hw/ppc/pnv: drop extra ref on PHB after adding as child
      target/riscv: fix general_user_opts hash table leak
      hw/riscv/virt: free flash devices and OEM strings on finalization
      hw/i2c/pmbus: fix undefined behavior in pmbus_direct_mode2data
      qtest: add "qom-tests" command

 tests/qtest/libqtest.h              |  8 ++++++
 accel/kvm/kvm-all.c                 |  8 ++++++
 backends/cryptodev-lkcf.c           |  4 +++
 backends/igvm-cfg.c                 |  1 +
 chardev/char-socket.c               |  4 +++
 hw/arm/aspeed.c                     |  9 +++++++
 hw/arm/sbsa-ref.c                   | 12 +++++++++
 hw/arm/virt.c                       | 14 ++++++++++
 hw/core/machine.c                   |  1 +
 hw/core/resetcontainer.c            |  3 +++
 hw/fsi/aspeed_apb2opb.c             | 31 +++++++++++++++------
 hw/gpio/pca9552.c                   |  2 +-
 hw/i2c/pmbus_device.c               |  4 +--
 hw/i386/nitro_enclave.c             | 11 ++++++++
 hw/i386/pc.c                        | 10 +++++++
 hw/i386/x86.c                       |  9 +++++++
 hw/intc/apic_common.c               |  2 +-
 hw/loongarch/virt.c                 | 14 ++++++++++
 hw/pci-bridge/pci_expander_bridge.c |  8 +++++-
 hw/pci-host/i440fx.c                | 15 +++++++++--
 hw/pci-host/q35.c                   | 15 +++++++++--
 hw/pci/pci.c                        | 11 ++++++--
 hw/ppc/pnv.c                        |  1 +
 hw/ppc/spapr.c                      |  2 ++
 hw/remote/remote-obj.c              |  4 ++-
 hw/remote/vfio-user-obj.c           |  4 ++-
 hw/riscv/virt.c                     | 14 ++++++++++
 hw/tpm/tpm_tis_sysbus.c             |  9 +++++++
 io/net-listener.c                   |  9 ++++++-
 net/can/can_socketcan.c             |  8 ++++++
 net/colo-compare.c                  | 31 +++++++++++----------
 net/filter.c                        |  2 ++
 qom/object.c                        | 10 ++++---
 scsi/pr-manager-helper.c            |  1 +
 system/ioport.c                     |  4 ++-
 system/qtest.c                      | 54 +++++++++++++++++++++++++++++++++++++
 target/i386/kvm/tdx.c               |  5 ++++
 target/i386/sev.c                   | 37 +++++++++++++++++++++++++
 target/riscv/cpu.c                  |  7 +++++
 tests/qtest/libqtest.c              |  6 +++++
 tests/qtest/qom-test.c              | 12 +++++++++
 ui/console-vc.c                     |  1 +
 ui/console.c                        |  6 ++++-
 43 files changed, 379 insertions(+), 44 deletions(-)
---
base-commit: aa15257174da180c6a8a9d58f87319cfe61c5520
change-id: 20260427-qom-tests-9dcf3b969411

Best regards,
--  
Marc-André Lureau <marcandre.lureau@redhat.com>

[PATCH 01/41] hw/pci: handle missing bus in prop_pci_busnr_get

[Marc-André Lureau]

When called on an unrealized device (e.g. from
qmp_qom_list_properties), pci_get_bus() returns NULL since the device
has no parent bus. Check for this to avoid a NULL dereference in
pci_bus_num().

Fixes: df9ac7254fd9 ("hw/pci: Add a busnr property to pci_props and use for acpi/gi")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/pci/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a0dbb81fd90..bf13cabd174 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -64,10 +64,17 @@ static void pcibus_reset_hold(Object *obj, ResetType type);
 static bool pcie_has_upstream_port(PCIDevice *dev);
 
 static void prop_pci_busnr_get(Object *obj, Visitor *v, const char *name,
-                             void *opaque, Error **errp)
+                               void *opaque, Error **errp)
 {
-    uint8_t busnr = pci_dev_bus_num(PCI_DEVICE(obj));
+    PCIDevice *dev = PCI_DEVICE(obj);
+    PCIBus *bus = pci_get_bus(dev);
+    uint8_t busnr;
 
+    if (!bus) {
+        error_setg(errp, "device not attached to a PCI bus");
+        return;
+    }
+    busnr = pci_bus_num(bus);
     visit_type_uint8(v, name, &busnr, errp);
 }
 

-- 
2.53.0

[PATCH 02/41] chardev/char-socket: handle NULL addr in char_socket_get_addr

[Marc-André Lureau]

When called on an unrealized chardev (e.g. from
qmp_qom_list_properties), s->addr is NULL. Return an error instead of
assert().

Fixes: 123676e9894f ("char-socket: add 'addr' property")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index e064b105c50..b629575fcf8 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1529,6 +1529,10 @@ char_socket_get_addr(Object *obj, Visitor *v, const char *name,
 {
     SocketChardev *s = SOCKET_CHARDEV(obj);
 
+    if (!s->addr) {
+        error_setg(errp, "socket not connected");
+        return;
+    }
     visit_type_SocketAddress(v, name, &s->addr, errp);
 }
 

-- 
2.53.0

[PATCH 03/41] hw/pci-bridge: handle missing parent in prop_pxb_uid_get

[Marc-André Lureau]

When called on an unrealized pxb bus (e.g. from
qmp_qom_list_properties), bus->parent_dev is NULL. The pxb_bus_num()
callback dereferences it unconditionally. Check for this to avoid a
NULL dereference.

Fixes: 97b9cb066e5f ("hw/pci-bridge: Add acpi_uid property to TYPE_PXB_BUS")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 11623a5666f..40ffbc4e082 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -85,8 +85,14 @@ static uint16_t pxb_bus_numa_node(PCIBus *bus)
 static void prop_pxb_uid_get(Object *obj, Visitor *v, const char *name,
                              void *opaque, Error **errp)
 {
-    uint32_t uid = pci_bus_num(PCI_BUS(obj));
+    PCIBus *bus = PCI_BUS(obj);
+    uint32_t uid;
 
+    if (!bus->parent_dev) {
+        error_setg(errp, "bus not attached to a device");
+        return;
+    }
+    uid = pci_bus_num(bus);
     visit_type_uint32(v, name, &uid, errp);
 }
 

-- 
2.53.0

[PATCH 04/41] hw/pci-host/i440fx: handle NULL bus in pci-hole64 getters

[Marc-André Lureau]

When called on an unrealized i440FX host bridge (e.g. from
qmp_qom_list_properties), h->bus is NULL since the root bus is only
created during realize. Guard against this in both
pci_hole64_start and pci_hole64_end getters, reporting an error.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/pci-host/i440fx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index e7d638b296c..6952094510d 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -189,8 +189,14 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
                                                 const char *name,
                                                 void *opaque, Error **errp)
 {
-    uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    uint64_t hole64_start;
 
+    if (!h->bus) {
+        error_setg(errp, "bus not realized");
+        return;
+    }
+    hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
     visit_type_uint64(v, name, &hole64_start, errp);
 }
 
@@ -206,10 +212,15 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
-    uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
+    uint64_t hole64_start;
     Range w64;
     uint64_t value, hole64_end;
 
+    if (!h->bus) {
+        error_setg(errp, "bus not realized");
+        return;
+    }
+    hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
     hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);

-- 
2.53.0

Re: [PATCH 04/41] hw/pci-host/i440fx: handle NULL bus in pci-hole64 getters

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> When called on an unrealized i440FX host bridge (e.g. from
> qmp_qom_list_properties), h->bus is NULL since the root bus is only
> created during realize. Guard against this in both
> pci_hole64_start and pci_hole64_end getters, reporting an error.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/pci-host/i440fx.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index e7d638b296c..6952094510d 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -189,8 +189,14 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
>                                                   const char *name,
>                                                   void *opaque, Error **errp)
>   {
> -    uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
> +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    uint64_t hole64_start;
>   
> +    if (!h->bus) {
> +        error_setg(errp, "bus not realized");

"PCI host bridge not realized"?

But calling a getter at this point is a programming error IMHO.

> +        return;
> +    }
> +    hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
>       visit_type_uint64(v, name, &hole64_start, errp);
>   }

Re: [PATCH 04/41] hw/pci-host/i440fx: handle NULL bus in pci-hole64 getters

[Marc-André Lureau]

Hi

On Tue, Apr 28, 2026 at 1:01 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 27/4/26 21:42, Marc-André Lureau wrote:
> > When called on an unrealized i440FX host bridge (e.g. from
> > qmp_qom_list_properties), h->bus is NULL since the root bus is only
> > created during realize. Guard against this in both
> > pci_hole64_start and pci_hole64_end getters, reporting an error.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/pci-host/i440fx.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> > index e7d638b296c..6952094510d 100644
> > --- a/hw/pci-host/i440fx.c
> > +++ b/hw/pci-host/i440fx.c
> > @@ -189,8 +189,14 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
> >                                                   const char *name,
> >                                                   void *opaque, Error **errp)
> >   {
> > -    uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
> > +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> > +    uint64_t hole64_start;
> >
> > +    if (!h->bus) {
> > +        error_setg(errp, "bus not realized");
>
> "PCI host bridge not realized"?

Ok

>
> But calling a getter at this point is a programming error IMHO.
>

We don't have a way to prevent calling getters when objects are just
instantiated but not yet in some "clean" state. Since this can be
triggered at run-time by QMP, we better return an error than doing
assert() or crash.

Re: [PATCH 04/41] hw/pci-host/i440fx: handle NULL bus in pci-hole64 getters

[Philippe Mathieu-Daudé]

On 28/4/26 08:10, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Apr 28, 2026 at 1:01 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 27/4/26 21:42, Marc-André Lureau wrote:
>>> When called on an unrealized i440FX host bridge (e.g. from
>>> qmp_qom_list_properties), h->bus is NULL since the root bus is only
>>> created during realize. Guard against this in both
>>> pci_hole64_start and pci_hole64_end getters, reporting an error.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    hw/pci-host/i440fx.c | 15 +++++++++++++--
>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>>> index e7d638b296c..6952094510d 100644
>>> --- a/hw/pci-host/i440fx.c
>>> +++ b/hw/pci-host/i440fx.c
>>> @@ -189,8 +189,14 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
>>>                                                    const char *name,
>>>                                                    void *opaque, Error **errp)
>>>    {
>>> -    uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
>>> +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>> +    uint64_t hole64_start;
>>>
>>> +    if (!h->bus) {
>>> +        error_setg(errp, "bus not realized");
>>
>> "PCI host bridge not realized"?
> 
> Ok
> 
>>
>> But calling a getter at this point is a programming error IMHO.
>>
> 
> We don't have a way to prevent calling getters when objects are just
> instantiated but not yet in some "clean" state. Since this can be
> triggered at run-time by QMP, we better return an error than doing
> assert() or crash.

Yeah I know, thus no objection, but still I'm wondering if this is
a good design. Maybe running introspection tests should be restricted
to a particular QEMU mode, like x-pre-config?

[PATCH 05/41] hw/pci-host/q35: handle NULL bus in pci-hole64 getters

[Marc-André Lureau]

When called on an unrealized Q35 host bridge (e.g. from
qmp_qom_list_properties), h->bus is NULL since the root bus is only
created during realize. Guard against this in both the
pci_hole64_start and pci_hole64_end getters.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/pci-host/q35.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index e85e4227b37..149c32b4b0e 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -132,8 +132,14 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
                                           const char *name, void *opaque,
                                           Error **errp)
 {
-    uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    uint64_t hole64_start;
 
+    if (!h->bus) {
+        error_setg(errp, "bus not realized");
+        return;
+    }
+    hole64_start = q35_host_get_pci_hole64_start_value(obj);
     visit_type_uint64(v, name, &hole64_start, errp);
 }
 
@@ -149,10 +155,15 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
+    uint64_t hole64_start;
     Range w64;
     uint64_t value, hole64_end;
 
+    if (!h->bus) {
+        error_setg(errp, "bus not realized");
+        return;
+    }
+    hole64_start = q35_host_get_pci_hole64_start_value(obj);
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
     hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);

-- 
2.53.0

[PATCH 06/41] hw/remote: check visit return in vfu_object_set_socket

[Marc-André Lureau]

vfu_object_set_socket() dereferences o->socket without checking if
visit_type_SocketAddress() succeeded. On failure, o->socket remains
NULL, leading to a NULL dereference. Check the return value.

Fixes: 8f9a9259d32c ("vfio-user: define vfio-user-server object")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/remote/vfio-user-obj.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 12ecdab6dea..49bf5ecae0c 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -161,7 +161,9 @@ static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
 
     o->socket = NULL;
 
-    visit_type_SocketAddress(v, name, &o->socket, errp);
+    if (!visit_type_SocketAddress(v, name, &o->socket, errp)) {
+        return;
+    }
 
     if (o->socket->type != SOCKET_ADDRESS_TYPE_UNIX) {
         error_setg(errp, "vfu: Unsupported socket type - %s",

-- 
2.53.0

Re: [PATCH 06/41] hw/remote: check visit return in vfu_object_set_socket

[Jagannathan Raman]

On 4/27/26 3:42 PM, Marc-André Lureau wrote:
> vfu_object_set_socket() dereferences o->socket without checking if
> visit_type_SocketAddress() succeeded. On failure, o->socket remains
> NULL, leading to a NULL dereference. Check the return value.
>
> Fixes: 8f9a9259d32c ("vfio-user: define vfio-user-server object")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

LGTM

Reviewed-by: Jagannathan Raman <jag.raman@oracle.com>

> ---
>   hw/remote/vfio-user-obj.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> index 12ecdab6dea..49bf5ecae0c 100644
> --- a/hw/remote/vfio-user-obj.c
> +++ b/hw/remote/vfio-user-obj.c
> @@ -161,7 +161,9 @@ static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
>   
>       o->socket = NULL;
>   
> -    visit_type_SocketAddress(v, name, &o->socket, errp);
> +    if (!visit_type_SocketAddress(v, name, &o->socket, errp)) {
> +        return;
> +    }
>   
>       if (o->socket->type != SOCKET_ADDRESS_TYPE_UNIX) {
>           error_setg(errp, "vfu: Unsupported socket type - %s",
>

[PATCH 07/41] qom: skip link property check callback when clearing link

[Marc-André Lureau]

When setting a link property to an empty string (clearing the link),
new_target is NULL. The check callback is still invoked with this NULL
value, but callbacks like isa_ipmi_bmc_check() cast it with
type-checking macros (IPMI_BMC(val)) which crash on NULL.

There is nothing to validate when the link is being cleared, so skip
the check callback entirely when new_target is NULL.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/object.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index f981e270440..c50334b3023 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1903,10 +1903,12 @@ static void object_set_link_property(Object *obj, Visitor *v,
 
     g_free(path);
 
-    prop->check(obj, name, new_target, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
+    if (new_target) {
+        prop->check(obj, name, new_target, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     *targetp = new_target;

-- 
2.53.0

Re: [PATCH 07/41] qom: skip link property check callback when clearing link

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:44, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> When setting a link property to an empty string (clearing the link),
> new_target is NULL. The check callback is still invoked with this NULL
> value, but callbacks like isa_ipmi_bmc_check() cast it with
> type-checking macros (IPMI_BMC(val)) which crash on NULL.
>
> There is nothing to validate when the link is being cleared, so skip
> the check callback entirely when new_target is NULL.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qom/object.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index f981e270440..c50334b3023 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1903,10 +1903,12 @@ static void object_set_link_property(Object *obj, Visitor *v,
>
>      g_free(path);
>
> -    prop->check(obj, name, new_target, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> +    if (new_target) {
> +        prop->check(obj, name, new_target, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }

I think that this would incorrectly allow setting a link
property to NULL on a property that uses
qdev_prop_allow_set_link_before_realize(). If the device
wants to forbid changing the link prop after realize, that
should include forbidding "clear existing link to NULL" as
well as "set it to some non-NULL thing".

Fortunately almost no users of object_property_add_link()
seem to use a function other than either
object_property_allow_set_link or
qdev_prop_allow_set_link_before_realize. The exceptions
I think are just xlnx_dp_set_dpdma and isa_ipmi_bmc_check.
So I think we need to fix those, and make sure the
docs point out that the check method can be passed a
NULL object.

thanks
-- PMM

[PATCH 08/41] hw/intc/apic: guard against NULL cpu in set_id

[Marc-André Lureau]

apic_common_set_id() dereferences s->cpu to check for x2APIC support
when the APIC ID is >= 255. On a standalone APIC object that has not
been attached to a CPU, s->cpu is NULL, causing a segfault.

Add a NULL check before dereferencing. When there is no CPU, the
x2APIC feature cannot be verified, so reject the large APIC ID.

Fixes: b5ee0468e9d2 ("apic: add support for x2APIC mode")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/intc/apic_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index bf4abc21d7b..874a05e529c 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -421,7 +421,7 @@ static void apic_common_set_id(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    if (value >= 255 && !cpu_has_x2apic_feature(&s->cpu->env)) {
+    if (value >= 255 && (!s->cpu || !cpu_has_x2apic_feature(&s->cpu->env))) {
         error_setg(&local_err,
                    "APIC ID %d requires x2APIC feature in CPU",
                    value);

-- 
2.53.0

Re: [PATCH 08/41] hw/intc/apic: guard against NULL cpu in set_id

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> apic_common_set_id() dereferences s->cpu to check for x2APIC support
> when the APIC ID is >= 255. On a standalone APIC object that has not
> been attached to a CPU, s->cpu is NULL, causing a segfault.
> 
> Add a NULL check before dereferencing. When there is no CPU, the
> x2APIC feature cannot be verified, so reject the large APIC ID.
> 
> Fixes: b5ee0468e9d2 ("apic: add support for x2APIC mode")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/intc/apic_common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index bf4abc21d7b..874a05e529c 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -421,7 +421,7 @@ static void apic_common_set_id(Object *obj, Visitor *v, const char *name,
>           return;
>       }

Could we check s->cpu earlier, i.e. on entry?

Or actually, why don't we use a link property checked once
in apic_common_realize()?

>   
> -    if (value >= 255 && !cpu_has_x2apic_feature(&s->cpu->env)) {
> +    if (value >= 255 && (!s->cpu || !cpu_has_x2apic_feature(&s->cpu->env))) {
>           error_setg(&local_err,
>                      "APIC ID %d requires x2APIC feature in CPU",
>                      value);
> 

Re: [PATCH 08/41] hw/intc/apic: guard against NULL cpu in set_id

[marcandre.lureau]

Hi,

On 2026-04-27 22:40:16+02:00, Philippe Mathieu-Daudé wrote:
> On 27/4/26 21:42, Marc-André Lureau wrote:
> 
> > apic_common_set_id() dereferences s->cpu to check for x2APIC support
> > when the APIC ID is >= 255. On a standalone APIC object that has not
> > been attached to a CPU, s->cpu is NULL, causing a segfault.
> > 
> > Add a NULL check before dereferencing. When there is no CPU, the
> > x2APIC feature cannot be verified, so reject the large APIC ID.
> > 
> > Fixes: b5ee0468e9d2 ("apic: add support for x2APIC mode")
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/intc/apic_common.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index bf4abc21d7b..874a05e529c 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -421,7 +421,7 @@ static void apic_common_set_id(Object *obj, Visitor *v, const char *name,
> >           return;
> >       }
> 
> Could we check s->cpu earlier, i.e. on entry?
>

On entry of the function? how would that help?

> 
> Or actually, why don't we use a link property checked once
> in apic_common_realize()?

That's also what you asked on commit 63073574e8d5551. But a link
property won't help (or how?).

I think to be more correct than this simple patch, we would need to move
the checks to realize(). There it should check ->cpu != NULL and ->id
validity. I'll update the fix.

[PATCH 09/41] RFC hw/remote: guard listener unregister in finalize

[Marc-André Lureau]

Guard the unregister by checking whether the listener callback was
set, which only happens right before registration.

This is probably better addressed by "[PATCH] qdev: test if DeviceListener
is in use". Just adding this patch for completeness.

Cc: jag.raman@oracle.com
Fixes: c7d80c7c1d9 ("multi-process: Associate fd of a PCIDevice with its object")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/remote/remote-obj.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
index 86192dc8dad..4e74ae76158 100644
--- a/hw/remote/remote-obj.c
+++ b/hw/remote/remote-obj.c
@@ -154,7 +154,9 @@ static void remote_object_finalize(Object *obj)
     RemoteObjectClass *k = REMOTE_OBJECT_GET_CLASS(obj);
     RemoteObject *o = REMOTE_OBJECT(obj);
 
-    device_listener_unregister(&o->listener);
+    if (o->listener.unrealize) {
+        device_listener_unregister(&o->listener);
+    }
 
     if (o->ioc) {
         qio_channel_shutdown(o->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);

-- 
2.53.0

Re: [PATCH 09/41] RFC hw/remote: guard listener unregister in finalize

[Jagannathan Raman]

On 4/27/26 3:42 PM, Marc-André Lureau wrote:
> Guard the unregister by checking whether the listener callback was
> set, which only happens right before registration.
>
> This is probably better addressed by "[PATCH] qdev: test if DeviceListener
> is in use". Just adding this patch for completeness.
>
> Cc: jag.raman@oracle.com
> Fixes: c7d80c7c1d9 ("multi-process: Associate fd of a PCIDevice with its object")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/remote/remote-obj.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
> index 86192dc8dad..4e74ae76158 100644
> --- a/hw/remote/remote-obj.c
> +++ b/hw/remote/remote-obj.c
> @@ -154,7 +154,9 @@ static void remote_object_finalize(Object *obj)
>       RemoteObjectClass *k = REMOTE_OBJECT_GET_CLASS(obj);
>       RemoteObject *o = REMOTE_OBJECT(obj);
>   
> -    device_listener_unregister(&o->listener);
> +    if (o->listener.unrealize) {
> +        device_listener_unregister(&o->listener);
> +    }
>   
>       if (o->ioc) {
>           qio_channel_shutdown(o->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>
Hi Marc-Andre,

I submitted a patch a short while back concerning this: 
https://patchew.org/QEMU/20260427162700.89750-1-jag.raman@oracle.com/

Do you want to stick to your patch, or want me to proceed with mine 
(while addressing feedback from Peter Maydell)? I'm OK with either approach.

Best regards,

Jag

Re: [PATCH 09/41] RFC hw/remote: guard listener unregister in finalize

[Marc-André Lureau]

Hi

On Tue, Apr 28, 2026 at 12:02 AM Jagannathan Raman <jag.raman@oracle.com> wrote:
>
>
> On 4/27/26 3:42 PM, Marc-André Lureau wrote:
> > Guard the unregister by checking whether the listener callback was
> > set, which only happens right before registration.
> >
> > This is probably better addressed by "[PATCH] qdev: test if DeviceListener
> > is in use". Just adding this patch for completeness.
> >
> > Cc: jag.raman@oracle.com
> > Fixes: c7d80c7c1d9 ("multi-process: Associate fd of a PCIDevice with its object")
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/remote/remote-obj.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
> > index 86192dc8dad..4e74ae76158 100644
> > --- a/hw/remote/remote-obj.c
> > +++ b/hw/remote/remote-obj.c
> > @@ -154,7 +154,9 @@ static void remote_object_finalize(Object *obj)
> >       RemoteObjectClass *k = REMOTE_OBJECT_GET_CLASS(obj);
> >       RemoteObject *o = REMOTE_OBJECT(obj);
> >
> > -    device_listener_unregister(&o->listener);
> > +    if (o->listener.unrealize) {
> > +        device_listener_unregister(&o->listener);
> > +    }
> >
> >       if (o->ioc) {
> >           qio_channel_shutdown(o->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> >
> Hi Marc-Andre,
>
> I submitted a patch a short while back concerning this:
> https://patchew.org/QEMU/20260427162700.89750-1-jag.raman@oracle.com/
>
> Do you want to stick to your patch, or want me to proceed with mine
> (while addressing feedback from Peter Maydell)? I'm OK with either approach.

I'd be happy to review/take your patch. Then I might include it in
future iterations of this series if you don't mind.
You may consider adding the Fixes: trailer.

[PATCH 10/41] backends/cryptodev-lkcf: skip cleanup when not initialized

[Marc-André Lureau]

cryptodev_lkcf_cleanup() locks a mutex that is only initialized
during the init vfunc (called at realize time). When the backend
is destroyed without ever being realized, the mutex is uninitialized
and the lock aborts.

Return early from cleanup when the backend was never started.

Note: it looks like cryptodev init/cleanup callbacks should rather be
regular complete/finalize overrides (calling the parent method).

Fixes: 39fff6f3e8b3 ("cryptodev: Add a lkcf-backend for cryptodev")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 backends/cryptodev-lkcf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
index 40c7bd3c5a0..74db03fe96b 100644
--- a/backends/cryptodev-lkcf.c
+++ b/backends/cryptodev-lkcf.c
@@ -255,6 +255,10 @@ static void cryptodev_lkcf_cleanup(CryptoDevBackend *backend, Error **errp)
     CryptoDevBackendClient *cc;
     CryptoDevLKCFTask *task, *next;
 
+    if (!lkcf->running) {
+        return;
+    }
+
     qemu_mutex_lock(&lkcf->mutex);
     lkcf->running = false;
     qemu_mutex_unlock(&lkcf->mutex);

-- 
2.53.0

Re: [PATCH 10/41] backends/cryptodev-lkcf: skip cleanup when not initialized

[zhenwei pi]

On 4/28/26 03:42, Marc-André Lureau wrote:
> cryptodev_lkcf_cleanup() locks a mutex that is only initialized
> during the init vfunc (called at realize time). When the backend
> is destroyed without ever being realized, the mutex is uninitialized
> and the lock aborts.
> 
> Return early from cleanup when the backend was never started.
> 
> Note: it looks like cryptodev init/cleanup callbacks should rather be
> regular complete/finalize overrides (calling the parent method).
> 
> Fixes: 39fff6f3e8b3 ("cryptodev: Add a lkcf-backend for cryptodev")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   backends/cryptodev-lkcf.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
> index 40c7bd3c5a0..74db03fe96b 100644
> --- a/backends/cryptodev-lkcf.c
> +++ b/backends/cryptodev-lkcf.c
> @@ -255,6 +255,10 @@ static void cryptodev_lkcf_cleanup(CryptoDevBackend *backend, Error **errp)
>       CryptoDevBackendClient *cc;
>       CryptoDevLKCFTask *task, *next;
>   
> +    if (!lkcf->running) {
> +        return;
> +    }
> +
>       qemu_mutex_lock(&lkcf->mutex);
>       lkcf->running = false;
>       qemu_mutex_unlock(&lkcf->mutex);
> 

'lkcf->running' is used for synchronization between main thread and LKCF 
works, 'backend->ready' indicates the status of cryptodev, so I prefer 
the following code instead:

if (!cryptodev_backend_is_ready(backend)) {
     return;
}

[PATCH 11/41] RFC io/net-listener: move mutex init to instance_init

[Marc-André Lureau]

The QIONetListener mutex is initialized in the convenience
constructor qio_net_listener_new() rather than in an instance_init.
This means a bare object_new(TYPE_QIO_NET_LISTENER) produces an
object with an uninitialized mutex, but instance_finalize
unconditionally calls qemu_mutex_destroy() on it, which aborts.

Move the mutex initialization to a proper instance_init so that init
and finalize are always paired regardless of how the object is
created.

This patch is just added for completeness, but "qio: Fix
qom-list-properties crash on net listener object" from Peter Xu is
basically the same.

Fixes: 9d86181874a ("qio: Protect NetListener callback with mutex")
Cc: peterx@redhat.com
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 io/net-listener.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/io/net-listener.c b/io/net-listener.c
index 9410d72da9c..1fd0f6cb5ab 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -38,7 +38,6 @@ QIONetListener *qio_net_listener_new(void)
     QIONetListener *listener;
 
     listener = QIO_NET_LISTENER(object_new(TYPE_QIO_NET_LISTENER));
-    qemu_mutex_init(&listener->lock);
     return listener;
 }
 
@@ -440,6 +439,13 @@ qio_net_listener_get_local_address(QIONetListener *listener, size_t n,
     return qio_channel_socket_get_local_address(sioc, errp);
 }
 
+static void qio_net_listener_instance_init(Object *obj)
+{
+    QIONetListener *listener = QIO_NET_LISTENER(obj);
+
+    qemu_mutex_init(&listener->lock);
+}
+
 static void qio_net_listener_finalize(Object *obj)
 {
     QIONetListener *listener = QIO_NET_LISTENER(obj);
@@ -463,6 +469,7 @@ static const TypeInfo qio_net_listener_info = {
     .parent = TYPE_OBJECT,
     .name = TYPE_QIO_NET_LISTENER,
     .instance_size = sizeof(QIONetListener),
+    .instance_init = qio_net_listener_instance_init,
     .instance_finalize = qio_net_listener_finalize,
 };
 

-- 
2.53.0

Re: [PATCH 11/41] RFC io/net-listener: move mutex init to instance_init

[Daniel P. Berrangé]

On Mon, Apr 27, 2026 at 11:42:10PM +0400, Marc-André Lureau wrote:
> The QIONetListener mutex is initialized in the convenience
> constructor qio_net_listener_new() rather than in an instance_init.
> This means a bare object_new(TYPE_QIO_NET_LISTENER) produces an
> object with an uninitialized mutex, but instance_finalize
> unconditionally calls qemu_mutex_destroy() on it, which aborts.
> 
> Move the mutex initialization to a proper instance_init so that init
> and finalize are always paired regardless of how the object is
> created.
> 
> This patch is just added for completeness, but "qio: Fix
> qom-list-properties crash on net listener object" from Peter Xu is
> basically the same.
> 
> Fixes: 9d86181874a ("qio: Protect NetListener callback with mutex")
> Cc: peterx@redhat.com
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  io/net-listener.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Re: [PATCH 11/41] RFC io/net-listener: move mutex init to instance_init

[Peter Xu]

On Mon, Apr 27, 2026 at 11:42:10PM +0400, Marc-André Lureau wrote:
> The QIONetListener mutex is initialized in the convenience
> constructor qio_net_listener_new() rather than in an instance_init.
> This means a bare object_new(TYPE_QIO_NET_LISTENER) produces an
> object with an uninitialized mutex, but instance_finalize
> unconditionally calls qemu_mutex_destroy() on it, which aborts.
> 
> Move the mutex initialization to a proper instance_init so that init
> and finalize are always paired regardless of how the object is
> created.
> 
> This patch is just added for completeness, but "qio: Fix
> qom-list-properties crash on net listener object" from Peter Xu is
> basically the same.
> 
> Fixes: 9d86181874a ("qio: Protect NetListener callback with mutex")
> Cc: peterx@redhat.com
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 12/41] RFC net/colo-compare: guard finalize against uninitialized state

[Marc-André Lureau]

colo_compare_finalize() assumes the object was fully set up by
colo_compare_complete(), but a bare object_new() followed by
object_unref() skips the complete callback entirely.

This causes two crashes:
- qemu_mutex_destroy on the static event_mtx which was never
  initialized (colo_compare_active is false)
- qemu_bh_delete(NULL) and iothread dereference when s->iothread
  is NULL

Guard the event_mtx teardown with colo_compare_active, and the
iothread-dependent cleanup with an s->iothread NULL check.

This is an alternative to patch "colo-compare: Fix QMP
qom-list-properties crashing", sent earlier, hence the RFC.

Fixes: 45942b79b9f8 ("net/colo-compare.c: Check that colo-compare is active")
Cc: peterx@redhat.com
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/colo-compare.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index c356419d6a8..be819db06c9 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -1416,7 +1416,7 @@ static void colo_compare_finalize(Object *obj)
             break;
         }
     }
-    if (QTAILQ_EMPTY(&net_compares)) {
+    if (colo_compare_active && QTAILQ_EMPTY(&net_compares)) {
         colo_compare_active = false;
         qemu_mutex_destroy(&event_mtx);
         qemu_cond_destroy(&event_complete_cond);
@@ -1431,30 +1431,29 @@ static void colo_compare_finalize(Object *obj)
     }
 
     colo_compare_timer_del(s);
+    g_clear_pointer(&s->event_bh, qemu_bh_delete);
 
-    qemu_bh_delete(s->event_bh);
+    if (s->iothread) {
+        AioContext *ctx = iothread_get_aio_context(s->iothread);
 
-    AioContext *ctx = iothread_get_aio_context(s->iothread);
-    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
-    if (s->notify_dev) {
-        AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
-    }
+        AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
+        if (s->notify_dev) {
+            AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
+        }
+
+        /* Release all unhandled packets after compare thread exited */
+        g_queue_foreach(&s->conn_list, colo_flush_packets, s);
+        AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
 
-    /* Release all unhandled packets after compare thead exited */
-    g_queue_foreach(&s->conn_list, colo_flush_packets, s);
-    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
+        object_unref(OBJECT(s->iothread));
+    }
 
     g_queue_clear(&s->conn_list);
     g_queue_clear(&s->out_sendco.send_list);
     if (s->notify_dev) {
         g_queue_clear(&s->notify_sendco.send_list);
     }
-
-    if (s->connection_track_table) {
-        g_hash_table_destroy(s->connection_track_table);
-    }
-
-    object_unref(OBJECT(s->iothread));
+    g_clear_pointer(&s->connection_track_table, g_hash_table_destroy);
 
     g_free(s->pri_indev);
     g_free(s->sec_indev);

-- 
2.53.0

Re: [PATCH 12/41] RFC net/colo-compare: guard finalize against uninitialized state

[Peter Xu]

On Mon, Apr 27, 2026 at 11:42:11PM +0400, Marc-André Lureau wrote:
> colo_compare_finalize() assumes the object was fully set up by
> colo_compare_complete(), but a bare object_new() followed by
> object_unref() skips the complete callback entirely.
> 
> This causes two crashes:
> - qemu_mutex_destroy on the static event_mtx which was never
>   initialized (colo_compare_active is false)
> - qemu_bh_delete(NULL) and iothread dereference when s->iothread
>   is NULL
> 
> Guard the event_mtx teardown with colo_compare_active, and the
> iothread-dependent cleanup with an s->iothread NULL check.
> 
> This is an alternative to patch "colo-compare: Fix QMP
> qom-list-properties crashing", sent earlier, hence the RFC.
> 
> Fixes: 45942b79b9f8 ("net/colo-compare.c: Check that colo-compare is active")
> Cc: peterx@redhat.com
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

For this one, wouldn't the early version be better?

https://lore.kernel.org/all/20260423183212.468047-2-peterx@redhat.com/

It's simpler and skip everything when complete() isn't properly invoked.
E.g. if there'll be new things added to complete() in the future, we also
don't need to worry on whether there will be one more null-deref we may
overlook.

> ---
>  net/colo-compare.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index c356419d6a8..be819db06c9 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -1416,7 +1416,7 @@ static void colo_compare_finalize(Object *obj)
>              break;
>          }
>      }
> -    if (QTAILQ_EMPTY(&net_compares)) {
> +    if (colo_compare_active && QTAILQ_EMPTY(&net_compares)) {
>          colo_compare_active = false;
>          qemu_mutex_destroy(&event_mtx);
>          qemu_cond_destroy(&event_complete_cond);
> @@ -1431,30 +1431,29 @@ static void colo_compare_finalize(Object *obj)
>      }
>  
>      colo_compare_timer_del(s);
> +    g_clear_pointer(&s->event_bh, qemu_bh_delete);
>  
> -    qemu_bh_delete(s->event_bh);
> +    if (s->iothread) {
> +        AioContext *ctx = iothread_get_aio_context(s->iothread);
>  
> -    AioContext *ctx = iothread_get_aio_context(s->iothread);
> -    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> -    if (s->notify_dev) {
> -        AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
> -    }
> +        AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> +        if (s->notify_dev) {
> +            AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
> +        }
> +
> +        /* Release all unhandled packets after compare thread exited */
> +        g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> +        AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
>  
> -    /* Release all unhandled packets after compare thead exited */
> -    g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> -    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> +        object_unref(OBJECT(s->iothread));
> +    }
>  
>      g_queue_clear(&s->conn_list);
>      g_queue_clear(&s->out_sendco.send_list);
>      if (s->notify_dev) {
>          g_queue_clear(&s->notify_sendco.send_list);
>      }
> -
> -    if (s->connection_track_table) {
> -        g_hash_table_destroy(s->connection_track_table);
> -    }
> -
> -    object_unref(OBJECT(s->iothread));
> +    g_clear_pointer(&s->connection_track_table, g_hash_table_destroy);
>  
>      g_free(s->pri_indev);
>      g_free(s->sec_indev);
> 
> -- 
> 2.53.0
> 

-- 
Peter Xu

Re: [PATCH 12/41] RFC net/colo-compare: guard finalize against uninitialized state

[Marc-André Lureau]

Hi

On Thu, Apr 30, 2026 at 12:59 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Apr 27, 2026 at 11:42:11PM +0400, Marc-André Lureau wrote:
> > colo_compare_finalize() assumes the object was fully set up by
> > colo_compare_complete(), but a bare object_new() followed by
> > object_unref() skips the complete callback entirely.
> >
> > This causes two crashes:
> > - qemu_mutex_destroy on the static event_mtx which was never
> >   initialized (colo_compare_active is false)
> > - qemu_bh_delete(NULL) and iothread dereference when s->iothread
> >   is NULL
> >
> > Guard the event_mtx teardown with colo_compare_active, and the
> > iothread-dependent cleanup with an s->iothread NULL check.
> >
> > This is an alternative to patch "colo-compare: Fix QMP
> > qom-list-properties crashing", sent earlier, hence the RFC.
> >
> > Fixes: 45942b79b9f8 ("net/colo-compare.c: Check that colo-compare is active")
> > Cc: peterx@redhat.com
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> For this one, wouldn't the early version be better?
>
> https://lore.kernel.org/all/20260423183212.468047-2-peterx@redhat.com/
>
> It's simpler and skip everything when complete() isn't properly invoked.
> E.g. if there'll be new things added to complete() in the future, we also
> don't need to worry on whether there will be one more null-deref we may
> overlook.
>

Yes, I have mixed feelings. On one hand, I'd rather have clean up code
for each field that doesn't depend on some extra state that can also
change meaning Otoh, I like the simplicity of your earlier patch too.
I'd say take what you prefer, you are the maintainer here :)

Re: [PATCH 12/41] RFC net/colo-compare: guard finalize against uninitialized state

[Peter Xu]

On Thu, Apr 30, 2026 at 01:09:09AM +0400, Marc-André Lureau wrote:
> Yes, I have mixed feelings. On one hand, I'd rather have clean up code
> for each field that doesn't depend on some extra state that can also
> change meaning Otoh, I like the simplicity of your earlier patch too.
> I'd say take what you prefer, you are the maintainer here :)

I'm not. :)

$ ./scripts/get_maintainer.pl -f net/colo-compare.c
Zhang Chen <zhangckid@gmail.com> (supporter:COLO Proxy)
Li Zhijian <lizhijian@fujitsu.com> (supporter:COLO Proxy)
Jason Wang <jasowang@redhat.com> (maintainer:Network device ba...)
qemu-devel@nongnu.org (open list:All patches CC here)

No strong feelings. I have a slight preference, but either way is fine by
me. In case you prefer the current one, feel free to still take:

Acked-by: Peter Xu <peterx@redhat.com>

Thanks for looking into all these.

-- 
Peter Xu

[PATCH 13/41] RFC system/ioport: move embedded memory region cleanup to portio_list_destroy

[Marc-André Lureau]

memory_region_portio_list_finalize() unconditionally calls
object_unref() on the embedded MemoryRegion, but when the object was
created via bare object_new() the MemoryRegion was never initialized
and has a refcount of 0, triggering an assertion failure.

The unref in finalize drops the keep-alive reference added during the
reparenting in portio_list_add_1(). Move it to portio_list_destroy()
where the cleanup belongs, and assert the refcount is zero in
finalize.

This is an alternative to "system/ioport: Fix qom-list-properties crash
on portio list obj" patch, hence the RFC.

Fixes: 690705ca0b0f ("softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions")
Cc: peterx@redhat.com
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 system/ioport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/system/ioport.c b/system/ioport.c
index 9209bff2eab..aa8f3229804 100644
--- a/system/ioport.c
+++ b/system/ioport.c
@@ -145,6 +145,7 @@ void portio_list_destroy(PortioList *piolist)
     for (i = 0; i < piolist->nr; ++i) {
         mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
         object_unparent(OBJECT(&mrpio->mr));
+        object_unref(OBJECT(&mrpio->mr));
         object_unref(mrpio);
     }
     g_free(piolist->regions);
@@ -346,7 +347,8 @@ static void memory_region_portio_list_finalize(Object *obj)
 {
     MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
 
-    object_unref(&mrpio->mr);
+    /* dropped by portio_list_destroy(), or never initialized */
+    assert(!mrpio->mr.parent_obj.ref);
     g_free(mrpio->ports);
 }
 

-- 
2.53.0

Re: [PATCH 13/41] RFC system/ioport: move embedded memory region cleanup to portio_list_destroy

[Peter Xu]

On Mon, Apr 27, 2026 at 11:42:12PM +0400, Marc-André Lureau wrote:
> memory_region_portio_list_finalize() unconditionally calls
> object_unref() on the embedded MemoryRegion, but when the object was
> created via bare object_new() the MemoryRegion was never initialized
> and has a refcount of 0, triggering an assertion failure.
> 
> The unref in finalize drops the keep-alive reference added during the
> reparenting in portio_list_add_1(). Move it to portio_list_destroy()
> where the cleanup belongs, and assert the refcount is zero in
> finalize.
> 
> This is an alternative to "system/ioport: Fix qom-list-properties crash
> on portio list obj" patch, hence the RFC.
> 
> Fixes: 690705ca0b0f ("softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions")
> Cc: peterx@redhat.com
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  system/ioport.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/system/ioport.c b/system/ioport.c
> index 9209bff2eab..aa8f3229804 100644
> --- a/system/ioport.c
> +++ b/system/ioport.c
> @@ -145,6 +145,7 @@ void portio_list_destroy(PortioList *piolist)
>      for (i = 0; i < piolist->nr; ++i) {
>          mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
>          object_unparent(OBJECT(&mrpio->mr));
> +        object_unref(OBJECT(&mrpio->mr));
>          object_unref(mrpio);
>      }
>      g_free(piolist->regions);
> @@ -346,7 +347,8 @@ static void memory_region_portio_list_finalize(Object *obj)
>  {
>      MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
>  
> -    object_unref(&mrpio->mr);
> +    /* dropped by portio_list_destroy(), or never initialized */
> +    assert(!mrpio->mr.parent_obj.ref);

I'm not sure if it's worthwhile to just add such assert and violate the
layering here, by accessing mr's refcount. Shall we just drop this check?

OTOH, if we will move the unref to destroy, shall we move the ports below
over and just go ahead removing the whole finalize()?

>      g_free(mrpio->ports);
>  }
>  
> 
> -- 
> 2.53.0
> 

-- 
Peter Xu

Re: [PATCH 13/41] RFC system/ioport: move embedded memory region cleanup to portio_list_destroy

[marcandre.lureau]

On 2026-04-29 17:07:56-04:00, Peter Xu wrote:
> On Mon, Apr 27, 2026 at 11:42:12PM +0400, Marc-André Lureau wrote:
> 
> > memory_region_portio_list_finalize() unconditionally calls
> > object_unref() on the embedded MemoryRegion, but when the object was
> > created via bare object_new() the MemoryRegion was never initialized
> > and has a refcount of 0, triggering an assertion failure.
> > 
> > The unref in finalize drops the keep-alive reference added during the
> > reparenting in portio_list_add_1(). Move it to portio_list_destroy()
> > where the cleanup belongs, and assert the refcount is zero in
> > finalize.
> > 
> > This is an alternative to "system/ioport: Fix qom-list-properties crash
> > on portio list obj" patch, hence the RFC.
> > 
> > Fixes: 690705ca0b0f ("softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions")
> > Cc: peterx@redhat.com
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  system/ioport.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/system/ioport.c b/system/ioport.c
> > index 9209bff2eab..aa8f3229804 100644
> > --- a/system/ioport.c
> > +++ b/system/ioport.c
> > @@ -145,6 +145,7 @@ void portio_list_destroy(PortioList *piolist)
> >      for (i = 0; i < piolist->nr; ++i) {
> >          mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
> >          object_unparent(OBJECT(&mrpio->mr));
> > +        object_unref(OBJECT(&mrpio->mr));
> >          object_unref(mrpio);
> >      }
> >      g_free(piolist->regions);
> > @@ -346,7 +347,8 @@ static void memory_region_portio_list_finalize(Object *obj)
> >  {
> >      MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
> >  
> > -    object_unref(&mrpio->mr);
> > +    /* dropped by portio_list_destroy(), or never initialized */
> > +    assert(!mrpio->mr.parent_obj.ref);
> 
> I'm not sure if it's worthwhile to just add such assert and violate the
> layering here, by accessing mr's refcount. Shall we just drop this check?
> 
> OTOH, if we will move the unref to destroy, shall we move the ports below
> over and just go ahead removing the whole finalize()?

Not fond of doing cleanups in non-conventional ways. I will rather take
your "system/ioport: Fix qom-list-properties crash on portio list obj"
patch in next iterations.

[PATCH 14/41] ui/console: remove console from global list on finalization

[Marc-André Lureau]

This commit removes the QemuConsole from the global "consoles" list when
it is finalized, fixing use-after-free on throw-away objects.

Reproducer: QMP command qom-list-properties with typename
"qemu-text-console", "qemu-fixed-text-console" or
"qemu-graphic-console".

The assertions added ensure that `dcls`, `gl_block`, and the
`dump_queue` are empty before removal, confirming the console is in a
clean state. This is left to handle correctly in a future series for
hot-unplug case.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index a7bd22515b9..fde8f50db21 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -394,10 +394,14 @@ qemu_console_finalize(Object *obj)
 {
     QemuConsole *c = QEMU_CONSOLE(obj);
 
-    /* TODO: check this code path, and unregister from consoles */
+    /* TODO: fix hot-unplug support of consoles */
+    assert(c->dcls == 0);
+    assert(c->gl_block == 0);
+    assert(qemu_co_queue_empty(&c->dump_queue));
     g_clear_pointer(&c->surface, qemu_free_displaysurface);
     g_clear_pointer(&c->gl_unblock_timer, timer_free);
     g_clear_pointer(&c->ui_timer, timer_free);
+    QTAILQ_REMOVE(&consoles, c, next);
 }
 
 static void

-- 
2.53.0

[PATCH 15/41] hw/i386/x86: free oem_id and oem_table_id on finalization

[Marc-André Lureau]

x86_machine_initfn allocates oem_id and oem_table_id via g_strndup,
but no instance_finalize existed for x86_machine_info, so these
strings were never freed when the object was destroyed.

Add x86_machine_finalize to release both fields.

Fixes: d07b22863b8e ("acpi: Move setters/getters of oem fields to X86MachineState")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i386/x86.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 01872cba073..dc7f0d56b01 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -372,6 +372,14 @@ static void x86_machine_initfn(Object *obj)
     x86ms->above_4g_mem_start = 4 * GiB;
 }
 
+static void x86_machine_finalize(Object *obj)
+{
+    X86MachineState *x86ms = X86_MACHINE(obj);
+
+    g_free(x86ms->oem_id);
+    g_free(x86ms->oem_table_id);
+}
+
 static void x86_machine_class_init(ObjectClass *oc, const void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -445,6 +453,7 @@ static const TypeInfo x86_machine_info = {
     .abstract = true,
     .instance_size = sizeof(X86MachineState),
     .instance_init = x86_machine_initfn,
+    .instance_finalize = x86_machine_finalize,
     .class_size = sizeof(X86MachineClass),
     .class_init = x86_machine_class_init,
     .interfaces = (const InterfaceInfo[]) {

-- 
2.53.0

Re: [PATCH 15/41] hw/i386/x86: free oem_id and oem_table_id on finalization

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> x86_machine_initfn allocates oem_id and oem_table_id via g_strndup,
> but no instance_finalize existed for x86_machine_info, so these
> strings were never freed when the object was destroyed.
> 
> Add x86_machine_finalize to release both fields.
> 
> Fixes: d07b22863b8e ("acpi: Move setters/getters of oem fields to X86MachineState")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/i386/x86.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 16/41] hw/core/machine: free shim_filename on finalization

[Marc-André Lureau]

machine_set_shim allocates shim_filename via g_strdup, but
machine_finalize did not free it.

Fixes: a5bd044b1579 ("x86/loader: add -shim option")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/core/machine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1b661fd36ae..6a7fb312eb6 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1282,6 +1282,7 @@ static void machine_finalize(Object *obj)
     MachineState *ms = MACHINE(obj);
 
     machine_free_boot_config(ms);
+    g_free(ms->shim_filename);
     g_free(ms->kernel_filename);
     g_free(ms->initrd_filename);
     g_free(ms->kernel_cmdline);

-- 
2.53.0

[PATCH 17/41] hw/core/resetcontainer: free children array on finalization

[Marc-André Lureau]

resettable_container_init allocates a GPtrArray for children, but
resettable_container_finalize was empty and never freed it.

Fixes: 4c046ce37af0 ("hw/core: Add ResetContainer which holds objects implementing Resettable")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/core/resetcontainer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/core/resetcontainer.c b/hw/core/resetcontainer.c
index ef84aa2374a..a4a6476a036 100644
--- a/hw/core/resetcontainer.c
+++ b/hw/core/resetcontainer.c
@@ -66,6 +66,9 @@ static void resettable_container_init(Object *obj)
 
 static void resettable_container_finalize(Object *obj)
 {
+    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
+
+    g_ptr_array_unref(rc->children);
 }
 
 static void resettable_container_class_init(ObjectClass *klass,

-- 
2.53.0

Re: [PATCH 17/41] hw/core/resetcontainer: free children array on finalization

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:44, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> resettable_container_init allocates a GPtrArray for children, but
> resettable_container_finalize was empty and never freed it.
>
> Fixes: 4c046ce37af0 ("hw/core: Add ResetContainer which holds objects implementing Resettable")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/core/resetcontainer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/core/resetcontainer.c b/hw/core/resetcontainer.c
> index ef84aa2374a..a4a6476a036 100644
> --- a/hw/core/resetcontainer.c
> +++ b/hw/core/resetcontainer.c
> @@ -66,6 +66,9 @@ static void resettable_container_init(Object *obj)
>
>  static void resettable_container_finalize(Object *obj)
>  {
> +    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
> +
> +    g_ptr_array_unref(rc->children);
>  }
>
>  static void resettable_container_class_init(ObjectClass *klass,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 18/41] net/filter: free old values in property setters

[Marc-André Lureau]

netfilter_set_position and netfilter_set_netdev_id overwrote their
respective fields with g_strdup without freeing the previous value,
leaking memory on each property write.

Fixes: fdccce459621 ("init/cleanup of netfilter object")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/filter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/filter.c b/net/filter.c
index c7cc6615dc9..389f3b0bfef 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -130,6 +130,7 @@ static void netfilter_set_netdev_id(Object *obj, const char *str, Error **errp)
 {
     NetFilterState *nf = NETFILTER(obj);
 
+    g_free(nf->netdev_id);
     nf->netdev_id = g_strdup(str);
 }
 
@@ -182,6 +183,7 @@ static void netfilter_set_position(Object *obj, const char *str, Error **errp)
 {
     NetFilterState *nf = NETFILTER(obj);
 
+    g_free(nf->position);
     nf->position = g_strdup(str);
 }
 

-- 
2.53.0

[PATCH 19/41] ui/console-vc: destroy fifo on text console finalization

[Marc-André Lureau]

qemu_text_console_init creates a fifo8 via fifo8_create, but
qemu_text_console_finalize never called fifo8_destroy, leaking the
underlying buffer.

Fixes: 0c9d0641ac ("ui/console: replace QEMUFIFO with Fifo8")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console-vc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/console-vc.c b/ui/console-vc.c
index 107294af1cc..89fa9a216e1 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -1081,6 +1081,7 @@ qemu_text_console_finalize(Object *obj)
 {
     QemuTextConsole *s = QEMU_TEXT_CONSOLE(obj);
 
+    fifo8_destroy(&s->out_fifo);
     QTAILQ_REMOVE(&vt100s, &s->vt, list);
 }
 

-- 
2.53.0

[PATCH 20/41] target/i386/sev: add finalize functions and fix leaking setters

[Marc-André Lureau]

None of the three SEV types (sev-common, sev-guest, sev-snp-guest)
had instance_finalize functions, so all string fields allocated
during init or by property setters were leaked on object destruction.

Add sev_common_finalize, sev_guest_finalize, and
sev_snp_guest_finalize to free the allocated fields.

Also fix sev_common_set_sev_device, sev_guest_set_dh_cert_file,
and sev_guest_set_session_file to free the old value before
replacing it.

Fixes: a9b4942f485b ("target/i386: add Secure Encrypted Virtualization (SEV) object")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 target/i386/sev.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 261b6aaa37b..3c2734ad83b 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -2527,6 +2527,7 @@ sev_common_get_sev_device(Object *obj, Error **errp)
 static void
 sev_common_set_sev_device(Object *obj, const char *value, Error **errp)
 {
+    g_free(SEV_COMMON(obj)->sev_device);
     SEV_COMMON(obj)->sev_device = g_strdup(value);
 }
 
@@ -2831,12 +2832,21 @@ sev_common_instance_init(Object *obj)
     QTAILQ_INIT(&sev_common->launch_vmsa);
 }
 
+static void
+sev_common_finalize(Object *obj)
+{
+    SevCommonState *sev_common = SEV_COMMON(obj);
+
+    g_free(sev_common->sev_device);
+}
+
 /* sev guest info common to sev/sev-es/sev-snp */
 static const TypeInfo sev_common_info = {
     .parent = TYPE_X86_CONFIDENTIAL_GUEST,
     .name = TYPE_SEV_COMMON,
     .instance_size = sizeof(SevCommonState),
     .instance_init = sev_common_instance_init,
+    .instance_finalize = sev_common_finalize,
     .class_size = sizeof(SevCommonStateClass),
     .class_init = sev_common_class_init,
     .abstract = true,
@@ -2856,6 +2866,7 @@ sev_guest_get_dh_cert_file(Object *obj, Error **errp)
 static void
 sev_guest_set_dh_cert_file(Object *obj, const char *value, Error **errp)
 {
+    g_free(SEV_GUEST(obj)->dh_cert_file);
     SEV_GUEST(obj)->dh_cert_file = g_strdup(value);
 }
 
@@ -2870,6 +2881,7 @@ sev_guest_get_session_file(Object *obj, Error **errp)
 static void
 sev_guest_set_session_file(Object *obj, const char *value, Error **errp)
 {
+    g_free(SEV_GUEST(obj)->session_file);
     SEV_GUEST(obj)->session_file = g_strdup(value);
 }
 
@@ -2937,12 +2949,23 @@ sev_guest_instance_init(Object *obj)
     sev_guest->legacy_vm_type = ON_OFF_AUTO_AUTO;
 }
 
+static void
+sev_guest_finalize(Object *obj)
+{
+    SevGuestState *sev_guest = SEV_GUEST(obj);
+
+    g_free(sev_guest->dh_cert_file);
+    g_free(sev_guest->session_file);
+    g_free(sev_guest->measurement);
+}
+
 /* guest info specific sev/sev-es */
 static const TypeInfo sev_guest_info = {
     .parent = TYPE_SEV_COMMON,
     .name = TYPE_SEV_GUEST,
     .instance_size = sizeof(SevGuestState),
     .instance_init = sev_guest_instance_init,
+    .instance_finalize = sev_guest_finalize,
     .class_init = sev_guest_class_init,
 };
 
@@ -3195,6 +3218,19 @@ sev_snp_guest_instance_init(Object *obj)
     sev_snp_guest->kvm_start_conf.policy = DEFAULT_SEV_SNP_POLICY;
 }
 
+static void
+sev_snp_guest_finalize(Object *obj)
+{
+    SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
+
+    g_free(sev_snp_guest->guest_visible_workarounds);
+    g_free(sev_snp_guest->id_block_base64);
+    g_free(sev_snp_guest->id_block);
+    g_free(sev_snp_guest->id_auth_base64);
+    g_free(sev_snp_guest->id_auth);
+    g_free(sev_snp_guest->host_data);
+}
+
 /* guest info specific to sev-snp */
 static const TypeInfo sev_snp_guest_info = {
     .parent = TYPE_SEV_COMMON,
@@ -3202,6 +3238,7 @@ static const TypeInfo sev_snp_guest_info = {
     .instance_size = sizeof(SevSnpGuestState),
     .class_init = sev_snp_guest_class_init,
     .instance_init = sev_snp_guest_instance_init,
+    .instance_finalize = sev_snp_guest_finalize,
 };
 
 static void

-- 
2.53.0

[PATCH 21/41] target/i386/kvm/tdx: free strings in tdx_guest_finalize

[Marc-André Lureau]

tdx_guest_finalize was empty, so mrconfigid, mrowner, and
mrownerconfig set by property setters were never freed.

Fixes: d05a0858cf87 ("i386/tdx: Support user configurable mrconfigid/mrowner/mrownerconfig")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 target/i386/kvm/tdx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 4714c9d514e..df46fce7693 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -1585,6 +1585,11 @@ static void tdx_guest_init(Object *obj)
 
 static void tdx_guest_finalize(Object *obj)
 {
+    TdxGuest *tdx = TDX_GUEST(obj);
+
+    g_free(tdx->mrconfigid);
+    g_free(tdx->mrowner);
+    g_free(tdx->mrownerconfig);
 }
 
 static ResettableState *tdx_reset_state(Object *obj)

-- 
2.53.0

[PATCH 22/41] hw/i386/nitro_enclave: add instance finalize

[Marc-André Lureau]

Property setter strings (vsock, id, parent_role, parent_id) were
never freed because nitro_enclave_machine_info had no
instance_finalize.

Fixes: f1826463d2e8 ("machine/nitro-enclave: New machine type for AWS Nitro Enclaves")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i386/nitro_enclave.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/i386/nitro_enclave.c b/hw/i386/nitro_enclave.c
index a29f0044d0e..1ce9ee3358a 100644
--- a/hw/i386/nitro_enclave.c
+++ b/hw/i386/nitro_enclave.c
@@ -337,11 +337,22 @@ static void nitro_enclave_class_init(ObjectClass *oc, const void *data)
                                           "Set parent instance identifier");
 }
 
+static void nitro_enclave_machine_finalize(Object *obj)
+{
+    NitroEnclaveMachineState *nems = NITRO_ENCLAVE_MACHINE(obj);
+
+    g_free(nems->vsock);
+    g_free(nems->id);
+    g_free(nems->parent_role);
+    g_free(nems->parent_id);
+}
+
 static const TypeInfo nitro_enclave_machine_info = {
     .name          = TYPE_NITRO_ENCLAVE_MACHINE,
     .parent        = TYPE_MICROVM_MACHINE,
     .instance_size = sizeof(NitroEnclaveMachineState),
     .instance_init = nitro_enclave_machine_initfn,
+    .instance_finalize = nitro_enclave_machine_finalize,
     .class_size    = sizeof(NitroEnclaveMachineClass),
     .class_init    = nitro_enclave_class_init,
 };

-- 
2.53.0

[PATCH 23/41] net/can: free ifname on socketcan finalization

[Marc-André Lureau]

can_host_socketcan_set_if allocates ifname via g_strdup, but no
instance_finalize existed to free it.

Fixes: ea15ea8a7c67 ("net/can: support for connecting to Linux host SocketCAN interface.")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/can/can_socketcan.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/can/can_socketcan.c b/net/can/can_socketcan.c
index 8a57ae07178..be67ed7f5a9 100644
--- a/net/can/can_socketcan.c
+++ b/net/can/can_socketcan.c
@@ -319,11 +319,19 @@ static void can_host_socketcan_class_init(ObjectClass *klass,
     chc->disconnect = can_host_socketcan_disconnect;
 }
 
+static void can_host_socketcan_finalize(Object *obj)
+{
+    CanHostSocketCAN *c = CAN_HOST_SOCKETCAN(obj);
+
+    g_free(c->ifname);
+}
+
 static const TypeInfo can_host_socketcan_info = {
     .parent = TYPE_CAN_HOST,
     .name = TYPE_CAN_HOST_SOCKETCAN,
     .instance_size = sizeof(CanHostSocketCAN),
     .instance_init = can_host_socketcan_instance_init,
+    .instance_finalize = can_host_socketcan_finalize,
     .class_init = can_host_socketcan_class_init,
 };
 

-- 
2.53.0

Re: [PATCH 23/41] net/can: free ifname on socketcan finalization

[Pavel Pisa]

Hello Marc-André,

thanks for the catch of the missing free

On Monday 27 of April 2026 21:42:22 Marc-André Lureau wrote:
> can_host_socketcan_set_if allocates ifname via g_strdup, but no
> instance_finalize existed to free it.
>
> Fixes: ea15ea8a7c67 ("net/can: support for connecting to Linux host
> SocketCAN interface.") Signed-off-by: Marc-André Lureau
> <marcandre.lureau@redhat.com>
> ---
>  net/can/can_socketcan.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/can/can_socketcan.c b/net/can/can_socketcan.c
> index 8a57ae07178..be67ed7f5a9 100644
> --- a/net/can/can_socketcan.c
> +++ b/net/can/can_socketcan.c
> @@ -319,11 +319,19 @@ static void can_host_socketcan_class_init(ObjectClass
> *klass, chc->disconnect = can_host_socketcan_disconnect;
>  }
>
> +static void can_host_socketcan_finalize(Object *obj)
> +{
> +    CanHostSocketCAN *c = CAN_HOST_SOCKETCAN(obj);
> +
> +    g_free(c->ifname);
> +}
> +
>  static const TypeInfo can_host_socketcan_info = {
>      .parent = TYPE_CAN_HOST,
>      .name = TYPE_CAN_HOST_SOCKETCAN,
>      .instance_size = sizeof(CanHostSocketCAN),
>      .instance_init = can_host_socketcan_instance_init,
> +    .instance_finalize = can_host_socketcan_finalize,
>      .class_init = can_host_socketcan_class_init,
>  };

Acked-by: Pavel Pisa <pisa@fel.cvut.cz>

Best wishes,

                Pavel

                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

[PATCH 24/41] backends/igvm-cfg: free filename on finalization

[Marc-André Lureau]

set_igvm allocates filename via g_strdup, but igvm_cfg_finalize
did not free it.

Fixes: c1d466d267cf ("backends/igvm: Add IGVM loader and configuration")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 backends/igvm-cfg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backends/igvm-cfg.c b/backends/igvm-cfg.c
index 64589ca34f2..e1f09855f66 100644
--- a/backends/igvm-cfg.c
+++ b/backends/igvm-cfg.c
@@ -108,4 +108,5 @@ static void igvm_cfg_finalize(Object *obj)
     if (igvm->file >= 0) {
         igvm_free(igvm->file);
     }
+    g_free(igvm->filename);
 }

-- 
2.53.0

Re: [PATCH 24/41] backends/igvm-cfg: free filename on finalization

[Gerd Hoffmann]

On Mon, Apr 27, 2026 at 11:42:23PM +0400, Marc-André Lureau wrote:
> set_igvm allocates filename via g_strdup, but igvm_cfg_finalize
> did not free it.
> 
> Fixes: c1d466d267cf ("backends/igvm: Add IGVM loader and configuration")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

[PATCH 25/41] scsi/pr-manager-helper: free path on finalization

[Marc-André Lureau]

set_path allocates path via g_strdup, but
pr_manager_helper_instance_finalize did not free it.

Fixes: 9bad2a6b9d0a ("scsi: add persistent reservation manager using qemu-pr-helper")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scsi/pr-manager-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index f6454cd80de..53432e6d606 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -284,6 +284,7 @@ static void pr_manager_helper_instance_finalize(Object *obj)
 {
     PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(obj);
 
+    g_free(pr_mgr->path);
     object_unref(OBJECT(pr_mgr->ioc));
     qemu_mutex_destroy(&pr_mgr->lock);
 }

-- 
2.53.0

Re: [PATCH 25/41] scsi/pr-manager-helper: free path on finalization

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> set_path allocates path via g_strdup, but
> pr_manager_helper_instance_finalize did not free it.
>
> Fixes: 9bad2a6b9d0a ("scsi: add persistent reservation manager using qemu-pr-helper")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scsi/pr-manager-helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index f6454cd80de..53432e6d606 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -284,6 +284,7 @@ static void pr_manager_helper_instance_finalize(Object *obj)
>  {
>      PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(obj);
>
> +    g_free(pr_mgr->path);
>      object_unref(OBJECT(pr_mgr->ioc));
>      qemu_mutex_destroy(&pr_mgr->lock);
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 26/41] accel/kvm: free device path on finalization

[Marc-André Lureau]

kvm_set_device allocates device via g_strdup, but no
instance_finalize existed for the KVM accelerator type.

Fixes: aef158b093b9 ("Add class property to configure KVM device node to use")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 accel/kvm/kvm-all.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 92af42503b1..96f90ebb240 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -4342,10 +4342,18 @@ static void kvm_accel_class_init(ObjectClass *oc, const void *data)
     kvm_arch_accel_class_init(oc);
 }
 
+static void kvm_accel_finalize(Object *obj)
+{
+    KVMState *s = KVM_STATE(obj);
+
+    g_free(s->device);
+}
+
 static const TypeInfo kvm_accel_type = {
     .name = TYPE_KVM_ACCEL,
     .parent = TYPE_ACCEL,
     .instance_init = kvm_accel_instance_init,
+    .instance_finalize = kvm_accel_finalize,
     .class_init = kvm_accel_class_init,
     .instance_size = sizeof(KVMState),
 };

-- 
2.53.0

Re: [PATCH 26/41] accel/kvm: free device path on finalization

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> kvm_set_device allocates device via g_strdup, but no
> instance_finalize existed for the KVM accelerator type.
>
> Fixes: aef158b093b9 ("Add class property to configure KVM device node to use")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 92af42503b1..96f90ebb240 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -4342,10 +4342,18 @@ static void kvm_accel_class_init(ObjectClass *oc, const void *data)
>      kvm_arch_accel_class_init(oc);
>  }
>
> +static void kvm_accel_finalize(Object *obj)
> +{
> +    KVMState *s = KVM_STATE(obj);
> +
> +    g_free(s->device);
> +}
> +
>  static const TypeInfo kvm_accel_type = {
>      .name = TYPE_KVM_ACCEL,
>      .parent = TYPE_ACCEL,
>      .instance_init = kvm_accel_instance_init,
> +    .instance_finalize = kvm_accel_finalize,
>      .class_init = kvm_accel_class_init,
>      .instance_size = sizeof(KVMState),
>  };
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 27/41] system/qtest: free log path on finalization

[Marc-André Lureau]

qtest_set_log allocates log via g_strdup, but no instance_finalize
existed for the qtest type.

Fixes: 6ba7ada3559e ("qtest: add a QOM object for qtest")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 system/qtest.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/system/qtest.c b/system/qtest.c
index cf90cd53adb..e9e5773341f 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -1020,10 +1020,18 @@ static void qtest_class_init(ObjectClass *oc, const void *data)
                                   qtest_get_log, qtest_set_log);
 }
 
+static void qtest_finalize(Object *obj)
+{
+    QTest *q = QTEST(obj);
+
+    g_free(q->log);
+}
+
 static const TypeInfo qtest_info = {
     .name = TYPE_QTEST,
     .parent = TYPE_OBJECT,
     .class_init = qtest_class_init,
+    .instance_finalize = qtest_finalize,
     .instance_size = sizeof(QTest),
     .interfaces = (const InterfaceInfo[]) {
         { TYPE_USER_CREATABLE },

-- 
2.53.0

Re: [PATCH 27/41] system/qtest: free log path on finalization

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:46, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> qtest_set_log allocates log via g_strdup, but no instance_finalize
> existed for the qtest type.
>
> Fixes: 6ba7ada3559e ("qtest: add a QOM object for qtest")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  system/qtest.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/system/qtest.c b/system/qtest.c
> index cf90cd53adb..e9e5773341f 100644
> --- a/system/qtest.c
> +++ b/system/qtest.c
> @@ -1020,10 +1020,18 @@ static void qtest_class_init(ObjectClass *oc, const void *data)
>                                    qtest_get_log, qtest_set_log);
>  }
>
> +static void qtest_finalize(Object *obj)
> +{
> +    QTest *q = QTEST(obj);
> +
> +    g_free(q->log);
> +}
> +
>  static const TypeInfo qtest_info = {
>      .name = TYPE_QTEST,
>      .parent = TYPE_OBJECT,
>      .class_init = qtest_class_init,
> +    .instance_finalize = qtest_finalize,
>      .instance_size = sizeof(QTest),
>      .interfaces = (const InterfaceInfo[]) {
>          { TYPE_USER_CREATABLE },

Doesn't the chr_name string have the same issue?

This pattern of "the object has a string property, and the
char *field that backs it is allocated if we give the property
a default value, and the setter has to free the old value
and strdup a copy of the new value, and you have to remember
to free the string in finalize" seems to be very common.
I wonder if there's some way we could make this less
error-prone and boilerplatey?

thanks
-- PMM

Re: [PATCH 27/41] system/qtest: free log path on finalization

[marcandre.lureau]

Hi

On 2026-04-30 17:01:14+01:00, Peter Maydell wrote:
> On Mon, 27 Apr 2026 at 20:46, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> 
> > qtest_set_log allocates log via g_strdup, but no instance_finalize
> > existed for the qtest type.
> >
> > Fixes: 6ba7ada3559e ("qtest: add a QOM object for qtest")
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  system/qtest.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/system/qtest.c b/system/qtest.c
> > index cf90cd53adb..e9e5773341f 100644
> > --- a/system/qtest.c
> > +++ b/system/qtest.c
> > @@ -1020,10 +1020,18 @@ static void qtest_class_init(ObjectClass *oc, const void *data)
> >                                    qtest_get_log, qtest_set_log);
> >  }
> >
> > +static void qtest_finalize(Object *obj)
> > +{
> > +    QTest *q = QTEST(obj);
> > +
> > +    g_free(q->log);
> > +}
> > +
> >  static const TypeInfo qtest_info = {
> >      .name = TYPE_QTEST,
> >      .parent = TYPE_OBJECT,
> >      .class_init = qtest_class_init,
> > +    .instance_finalize = qtest_finalize,
> >      .instance_size = sizeof(QTest),
> >      .interfaces = (const InterfaceInfo[]) {
> >          { TYPE_USER_CREATABLE },
> 
> Doesn't the chr_name string have the same issue?
> 

Indeed. It doesn't trigger in the test because a bare qtest object
has NULL chardev. But we should fix the leak nonetheless.

> This pattern of "the object has a string property, and the
> char *field that backs it is allocated if we give the property
> a default value, and the setter has to free the old value
> and strdup a copy of the new value, and you have to remember
> to free the string in finalize" seems to be very common.
> I wonder if there's some way we could make this less
> error-prone and boilerplatey?

I don't have a good solution to propose but to have a new optional
"free" callback + pointer and error-prone pointer arithmetics. I am not
sure this would make things simpler.

[PATCH 28/41] hw/i386/pc: free pcspk on finalization

[Marc-André Lureau]

Add pc_machine_finalize() to unref the pcspk device when it was never
realized. Once realized, the bus owns it and no action is needed.

Fixes: 6b8d1416482f ("audio: create pcspk device early")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i386/pc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2ecad3c503f..7b6ad97e5a9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1610,6 +1610,15 @@ static void pc_machine_initfn(Object *obj)
     }
 }
 
+static void pc_machine_finalize(Object *obj)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    if (pcms->pcspk && !qdev_is_realized(DEVICE(pcms->pcspk))) {
+        object_unref(OBJECT(pcms->pcspk));
+    }
+}
+
 static void pc_machine_reset(MachineState *machine, ResetType type)
 {
     CPUState *cs;
@@ -1748,6 +1757,7 @@ static const TypeInfo pc_machine_info = {
     .abstract = true,
     .instance_size = sizeof(PCMachineState),
     .instance_init = pc_machine_initfn,
+    .instance_finalize = pc_machine_finalize,
     .class_size = sizeof(PCMachineClass),
     .class_init = pc_machine_class_init,
     .interfaces = (const InterfaceInfo[]) {

-- 
2.53.0

[PATCH 29/41] hw/fsi: move OPBus address space init to realize

[Marc-André Lureau]

The OPBus instance_init initializes an AddressSpace, registering it in
the global address_spaces list. When a bare OPBus object is created
and destroyed (e.g. by qom-tests), there is no finalize to remove the
stale entry, leading to a heap-use-after-free when a subsequent
flatviews_reset iterates the list.

Move address_space_init to the bus realize callback and add the
corresponding address_space_destroy in unrealize, following the
NubusBus pattern. Also fix the memory_region_init owner from NULL to
the OPBus object, so the MR is properly parented instead of dangling
under the "unattached" container.

Fixes: eb04c35da2c0 ("hw/fsi: Aspeed APB2OPB & On-chip peripheral bus")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/fsi/aspeed_apb2opb.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/hw/fsi/aspeed_apb2opb.c b/hw/fsi/aspeed_apb2opb.c
index b9d72f3ecf6..5d1e471288b 100644
--- a/hw/fsi/aspeed_apb2opb.c
+++ b/hw/fsi/aspeed_apb2opb.c
@@ -282,13 +282,6 @@ static void fsi_aspeed_apb2opb_realize(DeviceState *dev, Error **errp)
     AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
     int i;
 
-    /*
-     * TODO: The OPBus model initializes the OPB address space in
-     * the .instance_init handler and this is problematic for test
-     * device-introspect-test. To avoid a memory corruption and a QEMU
-     * crash, qbus_init() should be called from realize(). Something to
-     * improve. Possibly, OPBus could also be removed.
-     */
     for (i = 0; i < ASPEED_FSI_NUM; i++) {
         qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, DEVICE(s),
                   NULL);
@@ -348,15 +341,37 @@ static void fsi_opb_init(Object *o)
 {
     OPBus *opb = OP_BUS(o);
 
-    memory_region_init(&opb->mr, 0, TYPE_FSI_OPB, UINT32_MAX);
+    memory_region_init(&opb->mr, o, TYPE_FSI_OPB, UINT32_MAX);
+}
+
+static void fsi_opb_realize(BusState *bus, Error **errp)
+{
+    OPBus *opb = OP_BUS(bus);
+
     address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
 }
 
+static void fsi_opb_unrealize(BusState *bus)
+{
+    OPBus *opb = OP_BUS(bus);
+
+    address_space_destroy(&opb->as);
+}
+
+static void fsi_opb_class_init(ObjectClass *klass, const void *data)
+{
+    BusClass *bc = BUS_CLASS(klass);
+
+    bc->realize = fsi_opb_realize;
+    bc->unrealize = fsi_opb_unrealize;
+}
+
 static const TypeInfo opb_info = {
     .name = TYPE_OP_BUS,
     .parent = TYPE_BUS,
     .instance_init = fsi_opb_init,
     .instance_size = sizeof(OPBus),
+    .class_init = fsi_opb_class_init,
 };
 
 static void fsi_opb_register_types(void)

-- 
2.53.0

Re: [PATCH 29/41] hw/fsi: move OPBus address space init to realize

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> The OPBus instance_init initializes an AddressSpace, registering it in
> the global address_spaces list. When a bare OPBus object is created
> and destroyed (e.g. by qom-tests), there is no finalize to remove the
> stale entry, leading to a heap-use-after-free when a subsequent
> flatviews_reset iterates the list.
>
> Move address_space_init to the bus realize callback and add the
> corresponding address_space_destroy in unrealize, following the
> NubusBus pattern. Also fix the memory_region_init owner from NULL to
> the OPBus object, so the MR is properly parented instead of dangling
> under the "unattached" container.
>
> Fixes: eb04c35da2c0 ("hw/fsi: Aspeed APB2OPB & On-chip peripheral bus")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/fsi/aspeed_apb2opb.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/hw/fsi/aspeed_apb2opb.c b/hw/fsi/aspeed_apb2opb.c
> index b9d72f3ecf6..5d1e471288b 100644
> --- a/hw/fsi/aspeed_apb2opb.c
> +++ b/hw/fsi/aspeed_apb2opb.c
> @@ -282,13 +282,6 @@ static void fsi_aspeed_apb2opb_realize(DeviceState *dev, Error **errp)
>      AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
>      int i;
>
> -    /*
> -     * TODO: The OPBus model initializes the OPB address space in
> -     * the .instance_init handler and this is problematic for test
> -     * device-introspect-test. To avoid a memory corruption and a QEMU
> -     * crash, qbus_init() should be called from realize(). Something to
> -     * improve. Possibly, OPBus could also be removed.
> -     */

I think that what this TODO comment is trying to note is that
we do the qbus_init() in realize because the implementation
of the bus does things in its instance_init that it ought
to be deferring to realize. If we fix the bus to do those
things in realize instead, we ought to be able to move
this qbus_init() to the fsi_aspeed_apb2opb_init function
where it more logically belongs.

Cédric, I think you wrote this comment; what do you think?

>      for (i = 0; i < ASPEED_FSI_NUM; i++) {
>          qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, DEVICE(s),
>                    NULL);
> @@ -348,15 +341,37 @@ static void fsi_opb_init(Object *o)
>  {
>      OPBus *opb = OP_BUS(o);
>
> -    memory_region_init(&opb->mr, 0, TYPE_FSI_OPB, UINT32_MAX);
> +    memory_region_init(&opb->mr, o, TYPE_FSI_OPB, UINT32_MAX);
> +}
> +
> +static void fsi_opb_realize(BusState *bus, Error **errp)
> +{
> +    OPBus *opb = OP_BUS(bus);
> +
>      address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
>  }
>
> +static void fsi_opb_unrealize(BusState *bus)
> +{
> +    OPBus *opb = OP_BUS(bus);
> +
> +    address_space_destroy(&opb->as);
> +}
> +
> +static void fsi_opb_class_init(ObjectClass *klass, const void *data)
> +{
> +    BusClass *bc = BUS_CLASS(klass);
> +
> +    bc->realize = fsi_opb_realize;
> +    bc->unrealize = fsi_opb_unrealize;
> +}
> +
>  static const TypeInfo opb_info = {
>      .name = TYPE_OP_BUS,
>      .parent = TYPE_BUS,
>      .instance_init = fsi_opb_init,
>      .instance_size = sizeof(OPBus),
> +    .class_init = fsi_opb_class_init,
>  };
>
>  static void fsi_opb_register_types(void)

thanks
-- PMM

Re: [PATCH 29/41] hw/fsi: move OPBus address space init to realize

[Cédric Le Goater]

On 4/30/26 18:09, Peter Maydell wrote:
> On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> The OPBus instance_init initializes an AddressSpace, registering it in
>> the global address_spaces list. When a bare OPBus object is created
>> and destroyed (e.g. by qom-tests), there is no finalize to remove the
>> stale entry, leading to a heap-use-after-free when a subsequent
>> flatviews_reset iterates the list.
>>
>> Move address_space_init to the bus realize callback and add the
>> corresponding address_space_destroy in unrealize, following the
>> NubusBus pattern. Also fix the memory_region_init owner from NULL to
>> the OPBus object, so the MR is properly parented instead of dangling
>> under the "unattached" container.
>>
>> Fixes: eb04c35da2c0 ("hw/fsi: Aspeed APB2OPB & On-chip peripheral bus")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   hw/fsi/aspeed_apb2opb.c | 31 +++++++++++++++++++++++--------
>>   1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/fsi/aspeed_apb2opb.c b/hw/fsi/aspeed_apb2opb.c
>> index b9d72f3ecf6..5d1e471288b 100644
>> --- a/hw/fsi/aspeed_apb2opb.c
>> +++ b/hw/fsi/aspeed_apb2opb.c
>> @@ -282,13 +282,6 @@ static void fsi_aspeed_apb2opb_realize(DeviceState *dev, Error **errp)
>>       AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
>>       int i;
>>
>> -    /*
>> -     * TODO: The OPBus model initializes the OPB address space in
>> -     * the .instance_init handler and this is problematic for test
>> -     * device-introspect-test. To avoid a memory corruption and a QEMU
>> -     * crash, qbus_init() should be called from realize(). Something to
>> -     * improve. Possibly, OPBus could also be removed.
>> -     */
> 
> I think that what this TODO comment is trying to note is that
> we do the qbus_init() in realize because the implementation
> of the bus does things in its instance_init that it ought
> to be deferring to realize. If we fix the bus to do those
> things in realize instead, we ought to be able to move
> this qbus_init() to the fsi_aspeed_apb2opb_init function
> where it more logically belongs.
Yes, I spent some time trying to understand and untangle
the issue, but eventually left a TODO comment as it was
too complex to address at the time.

> Cédric, I think you wrote this comment; 

I did.

> what do you think?

If it is now safe to call qbus_init() in fsi_aspeed_apb2opb_init() :

     for (i = 0; i < ASPEED_FSI_NUM; i++) {
         object_initialize_child(o, "fsi-master[*]", &s->fsi[i],
                                 TYPE_FSI_MASTER);
         qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, DEVICE(s),
                   NULL);
     }

then, let's do that and remove the comment. If not, I'd rather
keep the comment.

I don't remember why I thought OPBus could be removed.

Thanks,

C.


> 
>>       for (i = 0; i < ASPEED_FSI_NUM; i++) {
>>           qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, DEVICE(s),
>>                     NULL);
>> @@ -348,15 +341,37 @@ static void fsi_opb_init(Object *o)
>>   {
>>       OPBus *opb = OP_BUS(o);
>>
>> -    memory_region_init(&opb->mr, 0, TYPE_FSI_OPB, UINT32_MAX);
>> +    memory_region_init(&opb->mr, o, TYPE_FSI_OPB, UINT32_MAX);
>> +}
>> +
>> +static void fsi_opb_realize(BusState *bus, Error **errp)
>> +{
>> +    OPBus *opb = OP_BUS(bus);
>> +
>>       address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
>>   }
>>
>> +static void fsi_opb_unrealize(BusState *bus)
>> +{
>> +    OPBus *opb = OP_BUS(bus);
>> +
>> +    address_space_destroy(&opb->as);
>> +}
>> +
>> +static void fsi_opb_class_init(ObjectClass *klass, const void *data)
>> +{
>> +    BusClass *bc = BUS_CLASS(klass);
>> +
>> +    bc->realize = fsi_opb_realize;
>> +    bc->unrealize = fsi_opb_unrealize;
>> +}
>> +
>>   static const TypeInfo opb_info = {
>>       .name = TYPE_OP_BUS,
>>       .parent = TYPE_BUS,
>>       .instance_init = fsi_opb_init,
>>       .instance_size = sizeof(OPBus),
>> +    .class_init = fsi_opb_class_init,
>>   };
>>
>>   static void fsi_opb_register_types(void)
> 
> thanks
> -- PMM

[PATCH 30/41] hw/gpio/pca9552: fix state_str leak in pca955x_set_led

[Marc-André Lureau]

visit_type_str() allocates state_str, but the function never frees it
on any code path. Use g_autofree to ensure it is freed on return.

Fixes: a90d8f84674d ("misc/pca9552: Add qom set and get")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/gpio/pca9552.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
index dd3f1536b65..472d8ad9571 100644
--- a/hw/gpio/pca9552.c
+++ b/hw/gpio/pca9552.c
@@ -342,7 +342,7 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
     PCA955xState *s = PCA955X(obj);
     int led, rc, reg, val;
     uint8_t state;
-    char *state_str;
+    g_autofree char *state_str = NULL;
 
     if (!visit_type_str(v, name, &state_str, errp)) {
         return;

-- 
2.53.0

Re: [PATCH 30/41] hw/gpio/pca9552: fix state_str leak in pca955x_set_led

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

Thanks,

Glenn

On Mon, 2026-04-27 at 23:42 +0400, Marc-André Lureau wrote:
> visit_type_str() allocates state_str, but the function never frees it
> on any code path. Use g_autofree to ensure it is freed on return.
> 
> Fixes: a90d8f84674d ("misc/pca9552: Add qom set and get")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/gpio/pca9552.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index dd3f1536b65..472d8ad9571 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -342,7 +342,7 @@ static void pca955x_set_led(Object *obj, Visitor *v, const char *name,
>      PCA955xState *s = PCA955X(obj);
>      int led, rc, reg, val;
>      uint8_t state;
> -    char *state_str;
> +    g_autofree char *state_str = NULL;
>  
>      if (!visit_type_str(v, name, &state_str, errp)) {
>          return;
> 

Re: [PATCH 30/41] hw/gpio/pca9552: fix state_str leak in pca955x_set_led

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> visit_type_str() allocates state_str, but the function never frees it
> on any code path. Use g_autofree to ensure it is freed on return.
> 
> Fixes: a90d8f84674d ("misc/pca9552: Add qom set and get")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/gpio/pca9552.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 31/41] hw/arm/aspeed: free fmc_model and spi_model on finalization

[Marc-André Lureau]

The fmc_model and spi_model strings are allocated via g_strdup in
property setters but never freed when the machine object is destroyed.

Fixes: 9820e52fbef7 ("hw/arm/aspeed: Add machine properties to define the flash models")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/aspeed.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a3db3406c5f..a48c4420583 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -419,12 +419,21 @@ static void aspeed_machine_class_init(ObjectClass *oc, const void *data)
     aspeed_machine_class_props_init(oc);
 }
 
+static void aspeed_machine_instance_finalize(Object *obj)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+
+    g_free(bmc->fmc_model);
+    g_free(bmc->spi_model);
+}
+
 static const TypeInfo aspeed_machine_types[] = {
     {
         .name          = TYPE_ASPEED_MACHINE,
         .parent        = TYPE_MACHINE,
         .instance_size = sizeof(AspeedMachineState),
         .instance_init = aspeed_machine_instance_init,
+        .instance_finalize = aspeed_machine_instance_finalize,
         .class_size    = sizeof(AspeedMachineClass),
         .class_init    = aspeed_machine_class_init,
         .abstract      = true,

-- 
2.53.0

Re: [PATCH 31/41] hw/arm/aspeed: free fmc_model and spi_model on finalization

[Cédric Le Goater]

On 4/27/26 21:42, Marc-André Lureau wrote:
> The fmc_model and spi_model strings are allocated via g_strdup in
> property setters but never freed when the machine object is destroyed.
> 
> Fixes: 9820e52fbef7 ("hw/arm/aspeed: Add machine properties to define the flash models")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/arm/aspeed.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index a3db3406c5f..a48c4420583 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -419,12 +419,21 @@ static void aspeed_machine_class_init(ObjectClass *oc, const void *data)
>       aspeed_machine_class_props_init(oc);
>   }
>   
> +static void aspeed_machine_instance_finalize(Object *obj)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +
> +    g_free(bmc->fmc_model);
> +    g_free(bmc->spi_model);
> +}
> +
>   static const TypeInfo aspeed_machine_types[] = {
>       {
>           .name          = TYPE_ASPEED_MACHINE,
>           .parent        = TYPE_MACHINE,
>           .instance_size = sizeof(AspeedMachineState),
>           .instance_init = aspeed_machine_instance_init,
> +        .instance_finalize = aspeed_machine_instance_finalize,
>           .class_size    = sizeof(AspeedMachineClass),
>           .class_init    = aspeed_machine_class_init,
>           .abstract      = true,
> 


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

[PATCH 32/41] hw/tpm: free PPI buffer on finalization

[Marc-André Lureau]

The PPI buffer is allocated with qemu_memalign() in instance_init but
never freed when the device is destroyed.

Fixes: 46cd2c1050f0 ("hw/tpm: add PPI support to tpm-tis-device for ARM64 virt")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/tpm/tpm_tis_sysbus.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 6bec30c36fc..891766ee03c 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -150,11 +150,20 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
+static void tpm_tis_sysbus_finalize(Object *obj)
+{
+    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(obj);
+    TPMState *s = &sbdev->state;
+
+    qemu_vfree(s->ppi.buf);
+}
+
 static const TypeInfo tpm_tis_sysbus_info = {
     .name = TYPE_TPM_TIS_SYSBUS,
     .parent = TYPE_DYNAMIC_SYS_BUS_DEVICE,
     .instance_size = sizeof(TPMStateSysBus),
     .instance_init = tpm_tis_sysbus_initfn,
+    .instance_finalize = tpm_tis_sysbus_finalize,
     .class_init  = tpm_tis_sysbus_class_init,
     .interfaces = (const InterfaceInfo[]) {
         { TYPE_TPM_IF },

-- 
2.53.0

Re: [PATCH 32/41] hw/tpm: free PPI buffer on finalization

[Stefan Berger]

On 4/27/26 3:42 PM, Marc-André Lureau wrote:
> The PPI buffer is allocated with qemu_memalign() in instance_init but
> never freed when the device is destroyed.
> 
> Fixes: 46cd2c1050f0 ("hw/tpm: add PPI support to tpm-tis-device for ARM64 virt")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   hw/tpm/tpm_tis_sysbus.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
> index 6bec30c36fc..891766ee03c 100644
> --- a/hw/tpm/tpm_tis_sysbus.c
> +++ b/hw/tpm/tpm_tis_sysbus.c
> @@ -150,11 +150,20 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data)
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>   }
>   
> +static void tpm_tis_sysbus_finalize(Object *obj)
> +{
> +    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(obj);
> +    TPMState *s = &sbdev->state;
> +
> +    qemu_vfree(s->ppi.buf);
> +}
> +
>   static const TypeInfo tpm_tis_sysbus_info = {
>       .name = TYPE_TPM_TIS_SYSBUS,
>       .parent = TYPE_DYNAMIC_SYS_BUS_DEVICE,
>       .instance_size = sizeof(TPMStateSysBus),
>       .instance_init = tpm_tis_sysbus_initfn,
> +    .instance_finalize = tpm_tis_sysbus_finalize,
>       .class_init  = tpm_tis_sysbus_class_init,
>       .interfaces = (const InterfaceInfo[]) {
>           { TYPE_TPM_IF },
> 

[PATCH 33/41] hw/arm/sbsa-ref: free unrealized flash devices on finalization

[Marc-André Lureau]

Flash devices are created with qdev_new() in instance_init and added as
children, but the initial reference from qdev_new() is only dropped by
sysbus_realize_and_unref() during machine init. When the machine object
is destroyed before realization (e.g. during qtest device introspection),
the flash devices leak.

Fixes: e9fdf453240e ("hw/arm: Add arm SBSA reference machine, devices part")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/sbsa-ref.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 52c35e10c2d..484b90053e8 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -892,6 +892,17 @@ static void sbsa_ref_instance_init(Object *obj)
     sbsa_flash_create(sms);
 }
 
+static void sbsa_ref_instance_finalize(Object *obj)
+{
+    SBSAMachineState *sms = SBSA_MACHINE(obj);
+
+    for (int i = 0; i < ARRAY_SIZE(sms->flash); i++) {
+        if (sms->flash[i] && !qdev_is_realized(DEVICE(sms->flash[i]))) {
+            object_unref(OBJECT(sms->flash[i]));
+        }
+    }
+}
+
 static void sbsa_ref_class_init(ObjectClass *oc, const void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -930,6 +941,7 @@ static const TypeInfo sbsa_ref_info = {
     .name          = TYPE_SBSA_MACHINE,
     .parent        = TYPE_MACHINE,
     .instance_init = sbsa_ref_instance_init,
+    .instance_finalize = sbsa_ref_instance_finalize,
     .class_init    = sbsa_ref_class_init,
     .instance_size = sizeof(SBSAMachineState),
     .interfaces    = aarch64_machine_interfaces,

-- 
2.53.0

Re: [PATCH 33/41] hw/arm/sbsa-ref: free unrealized flash devices on finalization

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> Flash devices are created with qdev_new() in instance_init and added as
> children, but the initial reference from qdev_new() is only dropped by
> sysbus_realize_and_unref() during machine init. When the machine object
> is destroyed before realization (e.g. during qtest device introspection),
> the flash devices leak.

Hmm PFlashCFI01 is opaque, OK.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Fixes: e9fdf453240e ("hw/arm: Add arm SBSA reference machine, devices part")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/arm/sbsa-ref.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)

[PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Marc-André Lureau]

Flash devices created in instance_init via qdev_new() hold an extra
reference that is only dropped on sysbus_realize_and_unref(). When the
machine is destroyed before realization, the flash objects leak. Also,
the oem_id and oem_table_id strings from g_strndup() are never freed.

Fixes: 602b458201ff ("acpi: Permit OEM ID and OEM table ID fields to be changed")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/virt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f62253e1aba..fac825ccd47 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -4032,6 +4032,19 @@ static void virt_instance_init(Object *obj)
     cxl_machine_init(obj, &vms->cxl_devices_state);
 }
 
+static void virt_instance_finalize(Object *obj)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    for (int i = 0; i < ARRAY_SIZE(vms->flash); i++) {
+        if (vms->flash[i] && !qdev_is_realized(DEVICE(vms->flash[i]))) {
+            object_unref(OBJECT(vms->flash[i]));
+        }
+    }
+    g_free(vms->oem_id);
+    g_free(vms->oem_table_id);
+}
+
 static const TypeInfo virt_machine_info = {
     .name          = TYPE_VIRT_MACHINE,
     .parent        = TYPE_MACHINE,
@@ -4040,6 +4053,7 @@ static const TypeInfo virt_machine_info = {
     .class_size    = sizeof(VirtMachineClass),
     .class_init    = virt_machine_class_init,
     .instance_init = virt_instance_init,
+    .instance_finalize = virt_instance_finalize,
     .interfaces = (const InterfaceInfo[]) {
          { TYPE_HOTPLUG_HANDLER },
          { }

-- 
2.53.0

Re: [PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> Flash devices created in instance_init via qdev_new() hold an extra
> reference that is only dropped on sysbus_realize_and_unref(). When the
> machine is destroyed before realization, the flash objects leak. Also,
> the oem_id and oem_table_id strings from g_strndup() are never freed.
> 
> Fixes: 602b458201ff ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/arm/virt.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Flash devices created in instance_init via qdev_new() hold an extra
> reference that is only dropped on sysbus_realize_and_unref(). When the
> machine is destroyed before realization, the flash objects leak. Also,
> the oem_id and oem_table_id strings from g_strndup() are never freed.
>
> Fixes: 602b458201ff ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/arm/virt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f62253e1aba..fac825ccd47 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -4032,6 +4032,19 @@ static void virt_instance_init(Object *obj)
>      cxl_machine_init(obj, &vms->cxl_devices_state);
>  }
>
> +static void virt_instance_finalize(Object *obj)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    for (int i = 0; i < ARRAY_SIZE(vms->flash); i++) {
> +        if (vms->flash[i] && !qdev_is_realized(DEVICE(vms->flash[i]))) {
> +            object_unref(OBJECT(vms->flash[i]));
> +        }

We currently use qdev_is_realized() almost nowhere in
the codebase. Isn't there some way to do this such that we
don't have to care about this because the object is
parented and gets auto-dereffed ? Having each object
have to care about the possibility of "inited but
not realized" devices in finalize seems like it's going
to result in a lot of bugs...

> +    }
> +    g_free(vms->oem_id);
> +    g_free(vms->oem_table_id);
> +}

thanks
-- PMM

Re: [PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Philippe Mathieu-Daudé]

On 30/4/26 17:12, Peter Maydell wrote:
> On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> Flash devices created in instance_init via qdev_new() hold an extra
>> reference that is only dropped on sysbus_realize_and_unref(). When the
>> machine is destroyed before realization, the flash objects leak. Also,
>> the oem_id and oem_table_id strings from g_strndup() are never freed.
>>
>> Fixes: 602b458201ff ("acpi: Permit OEM ID and OEM table ID fields to be changed")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   hw/arm/virt.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f62253e1aba..fac825ccd47 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -4032,6 +4032,19 @@ static void virt_instance_init(Object *obj)
>>       cxl_machine_init(obj, &vms->cxl_devices_state);
>>   }
>>
>> +static void virt_instance_finalize(Object *obj)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(vms->flash); i++) {
>> +        if (vms->flash[i] && !qdev_is_realized(DEVICE(vms->flash[i]))) {
>> +            object_unref(OBJECT(vms->flash[i]));
>> +        }
> 
> We currently use qdev_is_realized() almost nowhere in
> the codebase. Isn't there some way to do this such that we
> don't have to care about this because the object is
> parented and gets auto-dereffed ? Having each object
> have to care about the possibility of "inited but
> not realized" devices in finalize seems like it's going
> to result in a lot of bugs...

Why are we creating pflash devices if we aren't realizing them
(thus not using them at all)? Destroying them when unneeded
would avoid that manual cleanup, right?

Re: [PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Peter Maydell]

On Thu, 30 Apr 2026 at 16:18, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 30/4/26 17:12, Peter Maydell wrote:
> > On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >>
> >> +static void virt_instance_finalize(Object *obj)
> >> +{
> >> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> +    for (int i = 0; i < ARRAY_SIZE(vms->flash); i++) {
> >> +        if (vms->flash[i] && !qdev_is_realized(DEVICE(vms->flash[i]))) {
> >> +            object_unref(OBJECT(vms->flash[i]));
> >> +        }
> >
> > We currently use qdev_is_realized() almost nowhere in
> > the codebase. Isn't there some way to do this such that we
> > don't have to care about this because the object is
> > parented and gets auto-dereffed ? Having each object
> > have to care about the possibility of "inited but
> > not realized" devices in finalize seems like it's going
> > to result in a lot of bugs...
>
> Why are we creating pflash devices if we aren't realizing them
> (thus not using them at all)? Destroying them when unneeded
> would avoid that manual cleanup, right?

I wondered that, but what happens is that we init the pflash
devices in the machine's instance_init by calling virt_flash_create(),
and then later we do the "configure, realize, map" part in
virt_flash_map() in the MachineClass::init method. So if
you init the Machine object but never call its init method
before finalizing it, the flash devices are still in the
inited-but-not-realized state. (This is similar to how with
a normal qdev object, if you init it but don't realize it
it will init but not realize all its child objects.)
(We need to create the flash devices earlier than we do
the other devices on the board to make the machine properties
that virt_flash_create() sets up work.)

Since virt_flash_create1() calls object_property_add_child()
to make the flash device a child of the machine, I think
we could make that be the only reference, by having
virt_flash_create1() do:

 Object *obj = object_new_with_props(TYPE_PFLASH_CFI01, OBJECT(vms),
                                     name, &error_fatal, NULL);
 DeviceState *dev = DEVICE(obj);

and then using sysbus_realize() rather than sysbus_realize_and_unref()
in virt_flash_map1(). Not tested, though...

-- PMM

Re: [PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Marc-André Lureau]

Hi

On Thu, Apr 30, 2026 at 7:32 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 30 Apr 2026 at 16:18, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > On 30/4/26 17:12, Peter Maydell wrote:
> > > On Mon, 27 Apr 2026 at 20:45, Marc-André Lureau
> > > <marcandre.lureau@redhat.com> wrote:
> > >>
> > >> +static void virt_instance_finalize(Object *obj)
> > >> +{
> > >> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > >> +
> > >> +    for (int i = 0; i < ARRAY_SIZE(vms->flash); i++) {
> > >> +        if (vms->flash[i] && !qdev_is_realized(DEVICE(vms->flash[i]))) {
> > >> +            object_unref(OBJECT(vms->flash[i]));
> > >> +        }
> > >
> > > We currently use qdev_is_realized() almost nowhere in
> > > the codebase. Isn't there some way to do this such that we
> > > don't have to care about this because the object is
> > > parented and gets auto-dereffed ? Having each object
> > > have to care about the possibility of "inited but
> > > not realized" devices in finalize seems like it's going
> > > to result in a lot of bugs...
> >
> > Why are we creating pflash devices if we aren't realizing them
> > (thus not using them at all)? Destroying them when unneeded
> > would avoid that manual cleanup, right?
>
> I wondered that, but what happens is that we init the pflash
> devices in the machine's instance_init by calling virt_flash_create(),
> and then later we do the "configure, realize, map" part in
> virt_flash_map() in the MachineClass::init method. So if
> you init the Machine object but never call its init method
> before finalizing it, the flash devices are still in the
> inited-but-not-realized state. (This is similar to how with
> a normal qdev object, if you init it but don't realize it
> it will init but not realize all its child objects.)
> (We need to create the flash devices earlier than we do
> the other devices on the board to make the machine properties
> that virt_flash_create() sets up work.)
>
> Since virt_flash_create1() calls object_property_add_child()
> to make the flash device a child of the machine, I think
> we could make that be the only reference, by having
> virt_flash_create1() do:
>
>  Object *obj = object_new_with_props(TYPE_PFLASH_CFI01, OBJECT(vms),
>                                      name, &error_fatal, NULL);
>  DeviceState *dev = DEVICE(obj);
>
> and then using sysbus_realize() rather than sysbus_realize_and_unref()
> in virt_flash_map1(). Not tested, though...

Yes, in general, if a device creates & keeps a reference to an
instance, it should be a strong reference (we don't have support for
weak reference the way gobject has for ex). It should then unref it
unconditonally on finalize. Is it ok if it is addressed in a follow-up
series after this one lands? I am afraid it would delay this series or
make it longer, and a shift its focus

thanks

Re: [PATCH 34/41] hw/arm/virt: free flash devices and OEM strings on finalization

[Peter Maydell]

On Mon, 11 May 2026 at 14:01, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Yes, in general, if a device creates & keeps a reference to an
> instance, it should be a strong reference (we don't have support for
> weak reference the way gobject has for ex). It should then unref it
> unconditonally on finalize. Is it ok if it is addressed in a follow-up
> series after this one lands? I am afraid it would delay this series or
> make it longer, and a shift its focus

Yes, that's OK.

-- PMM

[PATCH 35/41] hw/loongarch/virt: free flash devices and OEM strings on finalization

[Marc-André Lureau]

Flash devices created in virt_initfn() via qdev_new() hold an extra
reference that is only dropped on sysbus_realize_and_unref(). When the
machine is destroyed before realization, the flash objects leak. Also,
the oem_id and oem_table_id strings from g_strndup() are never freed.

Fixes: 445c9c645 ("hw/loongarch/virt: Allow user to customize OEM ID and OEM table ID")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/loongarch/virt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 2fc15261309..188fbc86c0c 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1535,6 +1535,19 @@ static void virt_class_init(ObjectClass *oc, const void *data)
                                           "for PCI MMIO");
 }
 
+static void virt_instance_finalize(Object *obj)
+{
+    LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(obj);
+
+    for (int i = 0; i < ARRAY_SIZE(lvms->flash); i++) {
+        if (lvms->flash[i] && !qdev_is_realized(DEVICE(lvms->flash[i]))) {
+            object_unref(OBJECT(lvms->flash[i]));
+        }
+    }
+    g_free(lvms->oem_id);
+    g_free(lvms->oem_table_id);
+}
+
 static const TypeInfo virt_machine_types[] = {
     {
         .name           = TYPE_LOONGARCH_VIRT_MACHINE,
@@ -1542,6 +1555,7 @@ static const TypeInfo virt_machine_types[] = {
         .instance_size  = sizeof(LoongArchVirtMachineState),
         .class_init     = virt_class_init,
         .instance_init  = virt_initfn,
+        .instance_finalize = virt_instance_finalize,
         .interfaces = (const InterfaceInfo[]) {
          { TYPE_HOTPLUG_HANDLER },
          { }

-- 
2.53.0

[PATCH 36/41] hw/ppc/spapr: free host_model and host_serial on finalization

[Marc-André Lureau]

The host_model and host_serial strings are allocated via g_strdup in
property setters but never freed when the machine is destroyed.

Fixes: 27461d69a0f ("ppc: add host-serial and host-model machine attributes (CVE-2019-8934)")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/ppc/spapr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 428b491a7df..5c673bbf0dd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3517,6 +3517,8 @@ static void spapr_machine_finalizefn(Object *obj)
     SpaprMachineState *spapr = SPAPR_MACHINE(obj);
 
     g_free(spapr->kvm_type);
+    g_free(spapr->host_model);
+    g_free(spapr->host_serial);
 }
 
 void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)

-- 
2.53.0

[PATCH 37/41] hw/ppc/pnv: drop extra ref on PHB after adding as child

[Marc-André Lureau]

object_new() returns an object with refcount 1, and
object_property_add_child() adds another reference. The initial
reference must be dropped so the parent becomes the sole owner,
otherwise the PHB objects leak when the chip is destroyed.

Fixes: 0d512c7120a2 ("ppc/pnv: turn chip8->phbs[] into a PnvPHB* array")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/ppc/pnv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 524563dcfc2..476255b722f 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1590,6 +1590,7 @@ static void pnv_chip_power8_instance_init(Object *obj)
              */
             object_property_add_child(obj, "phb[*]", phb);
             chip8->phbs[i] = PNV_PHB(phb);
+            object_unref(phb);
         }
     }
 

-- 
2.53.0

Re: [PATCH 37/41] hw/ppc/pnv: drop extra ref on PHB after adding as child

[Miles Glenn]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

Thanks,

Glenn

On Mon, 2026-04-27 at 23:42 +0400, Marc-André Lureau wrote:
> object_new() returns an object with refcount 1, and
> object_property_add_child() adds another reference. The initial
> reference must be dropped so the parent becomes the sole owner,
> otherwise the PHB objects leak when the chip is destroyed.
> 
> Fixes: 0d512c7120a2 ("ppc/pnv: turn chip8->phbs[] into a PnvPHB* array")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/ppc/pnv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 524563dcfc2..476255b722f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1590,6 +1590,7 @@ static void pnv_chip_power8_instance_init(Object *obj)
>               */
>              object_property_add_child(obj, "phb[*]", phb);
>              chip8->phbs[i] = PNV_PHB(phb);
> +            object_unref(phb);
>          }
>      }
>  
> 

Re: [PATCH 37/41] hw/ppc/pnv: drop extra ref on PHB after adding as child

[Aditya Gupta]

On 28/04/26 01:12, Marc-André Lureau wrote:

> object_new() returns an object with refcount 1, and
> object_property_add_child() adds another reference. The initial
> reference must be dropped so the parent becomes the sole owner,
> otherwise the PHB objects leak when the chip is destroyed.
>
> Fixes: 0d512c7120a2 ("ppc/pnv: turn chip8->phbs[] into a PnvPHB* array")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/ppc/pnv.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 524563dcfc2..476255b722f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1590,6 +1590,7 @@ static void pnv_chip_power8_instance_init(Object *obj)
>                */
>               object_property_add_child(obj, "phb[*]", phb);
>               chip8->phbs[i] = PNV_PHB(phb);
> +            object_unref(phb);
>           }
>       }
>   

Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>


Thanks,

- Aditya G

[PATCH 38/41] target/riscv: fix general_user_opts hash table leak

[Marc-André Lureau]

The global general_user_opts hash table is recreated on every
riscv_cpu_init() call, leaking the previous one. Clear any existing
table before creating a new one, and destroy it on CPU finalization.

Fixes: d167a2247ede ("target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 target/riscv/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8ac935ac06e..15fa03e22a4 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1101,6 +1101,7 @@ static void riscv_cpu_init(Object *obj)
                             "riscv.cpu.rnmi", RNMI_MAX);
 #endif /* CONFIG_USER_ONLY */
 
+    g_clear_pointer(&general_user_opts, g_hash_table_destroy);
     general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
 
     /*
@@ -2961,6 +2962,11 @@ void riscv_isa_write_fdt(RISCVCPU *cpu, void *fdt, char *nodename)
     DEFINE_RISCV_CPU(type_name, parent_type_name,             \
         .profile = &(profile_))
 
+static void riscv_cpu_instance_finalize(Object *obj)
+{
+    g_clear_pointer(&general_user_opts, g_hash_table_destroy);
+}
+
 static const TypeInfo riscv_cpu_type_infos[] = {
     {
         .name = TYPE_RISCV_CPU,
@@ -2968,6 +2974,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
         .instance_size = sizeof(RISCVCPU),
         .instance_align = __alignof(RISCVCPU),
         .instance_init = riscv_cpu_init,
+        .instance_finalize = riscv_cpu_instance_finalize,
         .abstract = true,
         .class_size = sizeof(RISCVCPUClass),
         .class_init = riscv_cpu_common_class_init,

-- 
2.53.0

Re: [PATCH 38/41] target/riscv: fix general_user_opts hash table leak

[Alistair Francis]

On Tue, Apr 28, 2026 at 5:50 AM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> The global general_user_opts hash table is recreated on every
> riscv_cpu_init() call, leaking the previous one. Clear any existing
> table before creating a new one, and destroy it on CPU finalization.

@Daniel Henrique Barboza I'm pretty sure there should only ever be one
instance of general_user_opts and that's the bug

Alistair

>
> Fixes: d167a2247ede ("target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  target/riscv/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8ac935ac06e..15fa03e22a4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1101,6 +1101,7 @@ static void riscv_cpu_init(Object *obj)
>                              "riscv.cpu.rnmi", RNMI_MAX);
>  #endif /* CONFIG_USER_ONLY */
>
> +    g_clear_pointer(&general_user_opts, g_hash_table_destroy);
>      general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
>
>      /*
> @@ -2961,6 +2962,11 @@ void riscv_isa_write_fdt(RISCVCPU *cpu, void *fdt, char *nodename)
>      DEFINE_RISCV_CPU(type_name, parent_type_name,             \
>          .profile = &(profile_))
>
> +static void riscv_cpu_instance_finalize(Object *obj)
> +{
> +    g_clear_pointer(&general_user_opts, g_hash_table_destroy);
> +}
> +
>  static const TypeInfo riscv_cpu_type_infos[] = {
>      {
>          .name = TYPE_RISCV_CPU,
> @@ -2968,6 +2974,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>          .instance_size = sizeof(RISCVCPU),
>          .instance_align = __alignof(RISCVCPU),
>          .instance_init = riscv_cpu_init,
> +        .instance_finalize = riscv_cpu_instance_finalize,
>          .abstract = true,
>          .class_size = sizeof(RISCVCPUClass),
>          .class_init = riscv_cpu_common_class_init,
>
> --
> 2.53.0
>
>

Re: [PATCH 38/41] target/riscv: fix general_user_opts hash table leak

[marcandre.lureau]

Hi

On 2026-05-01 11:53:40+10:00, Alistair Francis wrote:
> On Tue, Apr 28, 2026 at 5:50 AM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> 
> > The global general_user_opts hash table is recreated on every
> > riscv_cpu_init() call, leaking the previous one. Clear any existing
> > table before creating a new one, and destroy it on CPU finalization.
> 
> @Daniel Henrique Barboza I'm pretty sure there should only ever be one
> instance of general_user_opts and that's the bug
>

Actually, it looks like it should be per-instance. And we could use a
hash set rather than a hashmap. I will update the patch.

Re: [PATCH 38/41] target/riscv: fix general_user_opts hash table leak

[Daniel Henrique Barboza]

On 4/30/2026 10:53 PM, Alistair Francis wrote:
> On Tue, Apr 28, 2026 at 5:50 AM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> The global general_user_opts hash table is recreated on every
>> riscv_cpu_init() call, leaking the previous one. Clear any existing
>> table before creating a new one, and destroy it on CPU finalization.
> 
> @Daniel Henrique Barboza I'm pretty sure there should only ever be one
> instance of general_user_opts and that's the bug

Yeah, ideally we would need a single instance of that since the user options
are global and not per-instance, i.e. all instances would see the same
array.  Not sure where to put that though, or even if we can declare it
another way ... maybe declare it as static, let CPU0 populate it and
the other CPUs just consume it?

I didn't sweat it because we have bigger problems w.r.t memory fingerprint
(e.g. the extension arrays we're carrying), and this hash will be populated
based on user options in the command line that usually aren't many.  But
it's something that we can look at, definitely.


Thanks,
Daniel


> 
> Alistair
> 
>>
>> Fixes: d167a2247ede ("target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   target/riscv/cpu.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 8ac935ac06e..15fa03e22a4 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1101,6 +1101,7 @@ static void riscv_cpu_init(Object *obj)
>>                               "riscv.cpu.rnmi", RNMI_MAX);
>>   #endif /* CONFIG_USER_ONLY */
>>
>> +    g_clear_pointer(&general_user_opts, g_hash_table_destroy);
>>       general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
>>
>>       /*
>> @@ -2961,6 +2962,11 @@ void riscv_isa_write_fdt(RISCVCPU *cpu, void *fdt, char *nodename)
>>       DEFINE_RISCV_CPU(type_name, parent_type_name,             \
>>           .profile = &(profile_))
>>
>> +static void riscv_cpu_instance_finalize(Object *obj)
>> +{
>> +    g_clear_pointer(&general_user_opts, g_hash_table_destroy);
>> +}
>> +
>>   static const TypeInfo riscv_cpu_type_infos[] = {
>>       {
>>           .name = TYPE_RISCV_CPU,
>> @@ -2968,6 +2974,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>>           .instance_size = sizeof(RISCVCPU),
>>           .instance_align = __alignof(RISCVCPU),
>>           .instance_init = riscv_cpu_init,
>> +        .instance_finalize = riscv_cpu_instance_finalize,
>>           .abstract = true,
>>           .class_size = sizeof(RISCVCPUClass),
>>           .class_init = riscv_cpu_common_class_init,
>>
>> --
>> 2.53.0
>>
>>

[PATCH 39/41] hw/riscv/virt: free flash devices and OEM strings on finalization

[Marc-André Lureau]

Add instance_finalize to free the two pflash devices (when unrealized)
and the OEM ID strings allocated during instance_init. Fixes leaks
found by ASan.

Fixes: 71eb522c4063 ("riscv/virt: Add the PFlash CFI01 device")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/riscv/virt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index a1c323e66df..315049bc86c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1745,6 +1745,19 @@ static void virt_machine_init(MachineState *machine)
     qemu_add_machine_init_done_notifier(&s->machine_done);
 }
 
+static void virt_machine_instance_finalize(Object *obj)
+{
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
+
+    for (int i = 0; i < ARRAY_SIZE(s->flash); i++) {
+        if (s->flash[i] && !qdev_is_realized(DEVICE(s->flash[i]))) {
+            object_unref(OBJECT(s->flash[i]));
+        }
+    }
+    g_free(s->oem_id);
+    g_free(s->oem_table_id);
+}
+
 static void virt_machine_instance_init(Object *obj)
 {
     RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
@@ -1984,6 +1997,7 @@ static const TypeInfo virt_machine_typeinfo = {
     .parent     = TYPE_MACHINE,
     .class_init = virt_machine_class_init,
     .instance_init = virt_machine_instance_init,
+    .instance_finalize = virt_machine_instance_finalize,
     .instance_size = sizeof(RISCVVirtState),
     .interfaces = (const InterfaceInfo[]) {
          { TYPE_HOTPLUG_HANDLER },

-- 
2.53.0

Re: [PATCH 39/41] hw/riscv/virt: free flash devices and OEM strings on finalization

[Philippe Mathieu-Daudé]

On 27/4/26 21:42, Marc-André Lureau wrote:
> Add instance_finalize to free the two pflash devices (when unrealized)
> and the OEM ID strings allocated during instance_init. Fixes leaks
> found by ASan.
> 
> Fixes: 71eb522c4063 ("riscv/virt: Add the PFlash CFI01 device")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/riscv/virt.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 39/41] hw/riscv/virt: free flash devices and OEM strings on finalization

[Alistair Francis]

On Tue, Apr 28, 2026 at 5:49 AM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Add instance_finalize to free the two pflash devices (when unrealized)
> and the OEM ID strings allocated during instance_init. Fixes leaks
> found by ASan.
>
> Fixes: 71eb522c4063 ("riscv/virt: Add the PFlash CFI01 device")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index a1c323e66df..315049bc86c 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1745,6 +1745,19 @@ static void virt_machine_init(MachineState *machine)
>      qemu_add_machine_init_done_notifier(&s->machine_done);
>  }
>
> +static void virt_machine_instance_finalize(Object *obj)
> +{
> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> +
> +    for (int i = 0; i < ARRAY_SIZE(s->flash); i++) {
> +        if (s->flash[i] && !qdev_is_realized(DEVICE(s->flash[i]))) {
> +            object_unref(OBJECT(s->flash[i]));
> +        }
> +    }
> +    g_free(s->oem_id);
> +    g_free(s->oem_table_id);
> +}
> +
>  static void virt_machine_instance_init(Object *obj)
>  {
>      RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> @@ -1984,6 +1997,7 @@ static const TypeInfo virt_machine_typeinfo = {
>      .parent     = TYPE_MACHINE,
>      .class_init = virt_machine_class_init,
>      .instance_init = virt_machine_instance_init,
> +    .instance_finalize = virt_machine_instance_finalize,
>      .instance_size = sizeof(RISCVVirtState),
>      .interfaces = (const InterfaceInfo[]) {
>           { TYPE_HOTPLUG_HANDLER },
>
> --
> 2.53.0
>
>

[PATCH 40/41] hw/i2c/pmbus: fix undefined behavior in pmbus_direct_mode2data

[Marc-André Lureau]

The intermediate result of (Y * 10^-R - b) / m can be negative when
the bias (b) is large and the raw register value is small (e.g. zero
on an uninitialized device). Assigning that negative double to uint32_t
is undefined behavior, caught by UBSan/clang.

Use a double intermediate and clamp negative results to zero.

Fixes: 3746d5c15e70 ("hw/i2c: add support for PMBus")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i2c/pmbus_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index 853dc4b4342..861f5b4fb63 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -23,8 +23,8 @@ uint16_t pmbus_data2direct_mode(PMBusCoefficients c, uint32_t value)
 uint32_t pmbus_direct_mode2data(PMBusCoefficients c, uint16_t value)
 {
     /* X = (Y * 10^-R - b) / m */
-    uint32_t x = (value / pow(10, c.R) - c.b) / c.m;
-    return x;
+    double x = (value / pow(10, c.R) - c.b) / c.m;
+    return (x > 0) ? (uint32_t)x : 0;
 }
 
 uint16_t pmbus_data2linear_mode(uint16_t value, int exp)

-- 
2.53.0

[PATCH 41/41] qtest: add "qom-tests" command

[Marc-André Lureau]

Add a new "qom-tests" to exercise basic object lifecycle. Instantiate
all non-abstract objects, get and set properties and unref.

This should quickly find leaks and other related issues that are
eventually triggerable at run-time with QMP qom commands.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qtest/libqtest.h |  8 ++++++++
 system/qtest.c         | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qtest/libqtest.c |  6 ++++++
 tests/qtest/qom-test.c | 12 ++++++++++++
 4 files changed, 72 insertions(+)

diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 9c118c89ca0..02d65ecb65d 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -426,6 +426,14 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
 
 void qtest_module_load(QTestState *s, const char *prefix, const char *libname);
 
+/**
+ * qtest_qom_tests:
+ * @s: #QTestState instance to operate on.
+ *
+ * Run QOM property get/set round-trip tests on all non-abstract types.
+ */
+void qtest_qom_tests(QTestState *s);
+
 /**
  * qtest_get_irq:
  * @s: #QTestState instance to operate on.
diff --git a/system/qtest.c b/system/qtest.c
index e9e5773341f..217fccf8544 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -31,6 +31,8 @@
 #include "qemu/cutils.h"
 #include "qemu/target-info.h"
 #include "qom/object_interfaces.h"
+#include "qom/qom-qobject.h"
+#include "qobject/qobject.h"
 
 #define MAX_IRQ 256
 
@@ -754,6 +756,50 @@ static void qtest_process_command(CharFrontend *chr, gchar **words)
         new_ns = qemu_clock_advance_virtual_time(ns);
         qtest_sendf(chr, "%s %"PRIi64"\n",
                     new_ns == ns ? "OK" : "FAIL", new_ns);
+    } else if (strcmp(words[0], "qom-tests") == 0) {
+        GSList *list, *l;
+
+        list = object_class_get_list(NULL, false);
+        for (l = list; l; l = l->next) {
+            ObjectClass *klass = l->data;
+            const char *type_name = object_class_get_name(klass);
+            Object *obj;
+            ObjectPropertyIterator iter;
+            ObjectProperty *prop;
+
+            obj = object_new_with_class(klass);
+            object_property_iter_init(&iter, obj);
+            while ((prop = object_property_iter_next(&iter))) {
+                QObject *value;
+                Error *local_err = NULL;
+
+                value = object_property_get_qobject(obj, prop->name,
+                                                    &local_err);
+                if (local_err) {
+                    error_report("qom-tests: %s.%s: get failed: %s",
+                                 type_name, prop->name,
+                                 error_get_pretty(local_err));
+                    error_free(local_err);
+                    continue;
+                }
+
+                if (prop->set) {
+                    if (!object_property_set_qobject(obj, prop->name, value,
+                                                     &local_err)) {
+                        error_report("qom-tests: %s.%s: set failed: %s",
+                                     type_name, prop->name,
+                                     error_get_pretty(local_err));
+                        error_free(local_err);
+                    }
+                }
+
+                qobject_unref(value);
+            }
+
+            object_unref(obj);
+        }
+        g_slist_free(list);
+        qtest_send(chr, "OK\n");
     } else if (process_command_cb && process_command_cb(chr, words)) {
         /* Command got consumed by the callback handler */
     } else {
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 051faf31e14..5f4588b128d 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1121,6 +1121,12 @@ void qtest_module_load(QTestState *s, const char *prefix, const char *libname)
     qtest_rsp(s);
 }
 
+void qtest_qom_tests(QTestState *s)
+{
+    qtest_sendf(s, "qom-tests\n");
+    qtest_rsp(s);
+}
+
 static int64_t qtest_clock_rsp(QTestState *s)
 {
     gchar **words;
diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
index 6421f2d9d9f..cf4c6b5add5 100644
--- a/tests/qtest/qom-test.c
+++ b/tests/qtest/qom-test.c
@@ -227,6 +227,17 @@ static void add_machine_test_case(const char *mname)
     g_free(path);
 }
 
+static void test_qom_qtests(void)
+{
+    QTestState *qts;
+
+    qts = qtest_initf("-machine none");
+
+    qtest_qom_tests(qts);
+
+    qtest_quit(qts);
+}
+
 int main(int argc, char **argv)
 {
     char *v_env = getenv("V");
@@ -238,6 +249,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_cb_for_every_machine(add_machine_test_case, g_test_quick());
+    qtest_add_func("qom/qom-qtests", test_qom_qtests);
 
     return g_test_run();
 }

-- 
2.53.0

Re: [PATCH 41/41] qtest: add "qom-tests" command

[Peter Maydell]

On Mon, 27 Apr 2026 at 20:50, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Add a new "qom-tests" to exercise basic object lifecycle. Instantiate
> all non-abstract objects, get and set properties and unref.
>
> This should quickly find leaks and other related issues that are
> eventually triggerable at run-time with QMP qom commands.

Can we have a comment please that says what you think the
basic object lifecycle is?

Also, the qom-test already has coverage of "init the object,
introspect its properties, unref it" -- how does this differ?
Does it duplicate parts of it? (qom-test is already a very
slow test so we don't want to duplicate work if we can avoid it.)

-- PMM

Re: [PATCH 41/41] qtest: add "qom-tests" command

[Marc-André Lureau]

Hi Peter

On Tue, Apr 28, 2026 at 12:08 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 27 Apr 2026 at 20:50, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Add a new "qom-tests" to exercise basic object lifecycle. Instantiate
> > all non-abstract objects, get and set properties and unref.
> >
> > This should quickly find leaks and other related issues that are
> > eventually triggerable at run-time with QMP qom commands.
>
> Can we have a comment please that says what you think the
> basic object lifecycle is?

Hmm, what is not clear about the comment above and its list?

> Also, the qom-test already has coverage of "init the object,
> introspect its properties, unref it" -- how does this differ?

It does introspect on already living and reachable objects, over QMP.
It doesn't instantiate, set, or unref.

> Does it duplicate parts of it? (qom-test is already a very
> slow test so we don't want to duplicate work if we can avoid it.)

I wouldn't say they overlap much. I wrote it as a qtest command to
make the test relatively fast (~100ms per target), and easy to invoke
on CLI (-qtest stdio <<< qom-tests). Its purpose is not to exercise
QMP. I agree though that adding a 1-2 seconds to CI is costly. We may
run it under g_test_thorough().

Re: [PATCH 41/41] qtest: add "qom-tests" command

[Peter Maydell]

On Tue, 28 Apr 2026 at 07:37, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi Peter
>
> On Tue, Apr 28, 2026 at 12:08 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 27 Apr 2026 at 20:50, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> > >
> > > Add a new "qom-tests" to exercise basic object lifecycle. Instantiate
> > > all non-abstract objects, get and set properties and unref.
> > >
> > > This should quickly find leaks and other related issues that are
> > > eventually triggerable at run-time with QMP qom commands.
> >
> > Can we have a comment please that says what you think the
> > basic object lifecycle is?
>
> Hmm, what is not clear about the comment above and its list?

It's quite brief and it's not in the actual source file.
More generally, there is a lifecycle for the basic QOM types,
and a more complicated one for QOM types that happen to be
qdev devices, some of which we are testing already. I think
we are not very good with documentation of what we expect
from QOM objects lifecycle-wise, so I think it's helpful to
be more explicit and detailed rather than less.

> > Also, the qom-test already has coverage of "init the object,
> > introspect its properties, unref it" -- how does this differ?
>
> It does introspect on already living and reachable objects, over QMP.
> It doesn't instantiate, set, or unref.

Ah, sorry; I was getting this confused with device-introspect-test,
which uses QMP and does a 'qom-list-types' to get all the
QOM types QEMU knows about, and then does a 'device-list-properties',
which implicitly will create the object with instance_init and then
finalize it afterwards.

thanks
-- PMM