[01/29] hw/ide/ahci.c: Fix memory leak spotted by valgrind

Message ID 1432814932-12608-2-git-send-email-zhaoshenglong@huawei.com
State New
Headers show

Commit Message

Shannon Zhao May 28, 2015, 12:08 p.m.
From: Shannon Zhao <shannon.zhao@linaro.org>

valgrind complains about:
==23381== 8 bytes in 1 blocks are definitely lost in loss record 438 of 2,785
==23381==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23381==    by 0x35478F: malloc_and_trace (vl.c:2556)
==23381==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
==23381==    by 0x3ED98B: qemu_extend_irqs (irq.c:55)
==23381==    by 0x3EDA17: qemu_allocate_irqs (irq.c:64)
==23381==    by 0x43D767: ahci_init (ahci.c:1356)
==23381==    by 0x43DAC5: sysbus_ahci_realize (ahci.c:1520)
==23381==    by 0x3EA4CC: device_set_realized (qdev.c:1058)
==23381==    by 0x516CD2: property_set_bool (object.c:1514)
==23381==    by 0x5155CC: object_property_set (object.c:837)
==23381==    by 0x5178EE: object_property_set_qobject (qom-qobject.c:24)
==23381==    by 0x51583B: object_property_set_bool (object.c:905)

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/ide/ahci.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Peter Maydell May 28, 2015, 12:16 p.m. | #1
On 28 May 2015 at 13:08, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> valgrind complains about:
> ==23381== 8 bytes in 1 blocks are definitely lost in loss record 438 of 2,785
> ==23381==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==23381==    by 0x35478F: malloc_and_trace (vl.c:2556)
> ==23381==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==23381==    by 0x3ED98B: qemu_extend_irqs (irq.c:55)
> ==23381==    by 0x3EDA17: qemu_allocate_irqs (irq.c:64)
> ==23381==    by 0x43D767: ahci_init (ahci.c:1356)
> ==23381==    by 0x43DAC5: sysbus_ahci_realize (ahci.c:1520)
> ==23381==    by 0x3EA4CC: device_set_realized (qdev.c:1058)
> ==23381==    by 0x516CD2: property_set_bool (object.c:1514)
> ==23381==    by 0x5155CC: object_property_set (object.c:837)
> ==23381==    by 0x5178EE: object_property_set_qobject (qom-qobject.c:24)
> ==23381==    by 0x51583B: object_property_set_bool (object.c:905)
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/ide/ahci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 9e5d862..4702a05 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1367,6 +1367,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
>          ad->port.dma->ops = &ahci_dma_ops;
>          ide_register_restart_cb(&ad->port);
>      }
> +    g_free(irqs);
>  }

Freeing the return value of qemu_allocate_irqs()
directly rather than using qemu_free_irqs() looks
dubious to me.

-- PMM
Peter Maydell May 28, 2015, 12:39 p.m. | #2
On 28 May 2015 at 13:24, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>
>
> On 2015/5/28 20:16, Peter Maydell wrote:
>> On 28 May 2015 at 13:08, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>> > --- a/hw/ide/ahci.c
>>> > +++ b/hw/ide/ahci.c
>>> > @@ -1367,6 +1367,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
>>> >          ad->port.dma->ops = &ahci_dma_ops;
>>> >          ide_register_restart_cb(&ad->port);
>>> >      }
>>> > +    g_free(irqs);
>>> >  }
>> Freeing the return value of qemu_allocate_irqs()
>> directly rather than using qemu_free_irqs() looks
>> dubious to me.
>
> qemu_free_irqs() will free the return value self of qemu_allocate_irqs()
> and also will free all the contents of this array while some of them are
> still in use.

Yes, but you're still looking into the internal implementation
details of this API, which is what I'm suggesting is dubious.

For instance, in this case, why are we calling
qemu_allocate_irqs() in the first place, rather than
just calling qemu_allocate_irq() inside the for() loop in
ahci_init()?

If code wants to deal with the array of irqs as an array,
it should probably be saving the pointer somewhere in its
state struct and freeing the whole thing on deinit. If it
doesn't actually want an array of irqs (as here) it should
probably not be calling qemu_allocate_irqs() in the first
place.

thanks
-- PMM

Patch

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9e5d862..4702a05 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1367,6 +1367,7 @@  void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
         ad->port.dma->ops = &ahci_dma_ops;
         ide_register_restart_cb(&ad->port);
     }
+    g_free(irqs);
 }
 
 void ahci_uninit(AHCIState *s)