Re: [PATCH] hw/usb: Fix memory leak in musb_reset()

[Peter Maydell]

On Sun, 30 Jun 2024 at 17:33, Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> The musb_reset function was causing a memory leak by not properly freeing
> the memory associated with USBPacket instances before reinitializing them.
> This commit addresses the memory leak by adding calls to usb_packet_cleanup
> for each USBPacket instance before reinitializing them with usb_packet_init.
>
> Asan log:
>
> =2970623==ERROR: LeakSanitizer: detected memory leaks
> Direct leak of 256 byte(s) in 16 object(s) allocated from:
>     #0 0x561e20629c3d in malloc 
> llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3
>     #1 0x7fee91885738 in g_malloc 
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5e738)
>     #2 0x561e21b4d0e1 in usb_packet_init hw/usb/core.c:531:5
>     #3 0x561e21c5016b in musb_reset hw/usb/hcd-musb.c:372:9
>     #4 0x561e21c502a9 in musb_init hw/usb/hcd-musb.c:385:5
>     #5 0x561e21c893ef in tusb6010_realize hw/usb/tusb6010.c:827:15
>     #6 0x561e23443355 in device_set_realized hw/core/qdev.c:510:13
>     #7 0x561e2346ac1b in property_set_bool qom/object.c:2354:5
>     #8 0x561e23463895 in object_property_set qom/object.c:1463:5
>     #9 0x561e23477909 in object_property_set_qobject qom/qom-qobject.c:28:10
>     #10 0x561e234645ed in object_property_set_bool qom/object.c:1533:15
>     #11 0x561e2343c830 in qdev_realize hw/core/qdev.c:291:12
>     #12 0x561e2343c874 in qdev_realize_and_unref hw/core/qdev.c:298:11
>     #13 0x561e20ad5091 in sysbus_realize_and_unref hw/core/sysbus.c:261:12
>     #14 0x561e22553283 in n8x0_usb_setup hw/arm/nseries.c:800:5
>     #15 0x561e2254e99b in n8x0_init hw/arm/nseries.c:1356:5
>     #16 0x561e22561170 in n810_init hw/arm/nseries.c:1418:5
>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  hw/usb/hcd-musb.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
> index 6dca373cb1..0300aeaec6 100644
> --- a/hw/usb/hcd-musb.c
> +++ b/hw/usb/hcd-musb.c
> @@ -368,6 +368,8 @@ void musb_reset(MUSBState *s)
>          s->ep[i].maxp[1] = 0x40;
>          s->ep[i].musb = s;
>          s->ep[i].epnum = i;
> +        usb_packet_cleanup(&s->ep[i].packey[0].p);
> +        usb_packet_cleanup(&s->ep[i].packey[1].p);
>          usb_packet_init(&s->ep[i].packey[0].p);
>          usb_packet_init(&s->ep[i].packey[1].p);
>      }

I don't think it's valid to call usb_packet_cleanup() on
a USBPacket that's never been inited, which is what will
happen on the first reset with this patch.

Looking at how hcd-ehci.c handles its s->ipacket (which has
the same "allocated at device creation and reused for the
whole lifetime of the device" semantics) I think what we
want is:
 * at device init, call usb_packet_init()
 * at device reset, do nothing
 * at device finalize, call usb_packet_cleanup()

(There is currently no hcd-musb function for finalize
because the only uses of it are for devices which continue
to exist for the whole lifetime of the simulation. So we
can ignore the last part of that.)

PS: the tusb6010 and hcd-musb are both used only by deprecated
board types which are scheduled to be deleted by the end
of the year (basically after we get the 9.1 release out,
and before 9.2). You might prefer to target your fuzzing
efforts to board types which aren't deprecated.

thanks
-- PMM