Re: [PATCH] virtio-pci: fix memory leak from device realization failure

[Zheng Huang <hz1624917200@gmail.com>]

Hi Philippe,

On 2025/2/28 17:24, Philippe Mathieu-Daudé wrote:
> Hi Zheng,
> 
> On 28/2/25 06:03, Zheng Huang wrote:
>> This commit adds failback routine for `virtio_pci_realize` to
>> fix the memory leak of an address space and the virtio-net device object.
>> If the realization of the device failed, the address space should be
>> destroyed too.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2845
>>
>> Signed-off-by: Zheng Huang <hz1624917200@outlook.com>
>>
>> ---
>>   hw/virtio/virtio-pci.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index c773a9130c..4b0d8cd90a 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2266,6 +2266,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>>       virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
>>       if (k->realize) {
>>           k->realize(proxy, errp);
>> +        if (*errp) {
>> +            address_space_destroy(&proxy->modern_cfg_mem_as);
>> +        }
>>       }
>>   }
>>   
> 
> I think instead we want to add an instance_init in virtio_pci_class_init
> and move the address_space_init call from virtio_pci_realize there.
> 
> Regards,
> 
> Phil.

I have reviewed the relevant code again and found that if address_space_init
is moved into instance_init, it will not be able to take follow-up actions
such as free the AS if device realization failed, thus failing to address the
issue. Additionally, I referred to the code for AS initialization and
destruction in other devices and found that they are managed in device
realize and unrealize handlers. Therefore, I still believe the previous
approach is a better choice.

If there are other potential solutions or considerations that I might have
missed, please let me know. I'm looking forward to hearing your thoughts!

Best regards,

Zheng.