Hi Peter, On Mon, Jul 1, 2024 at 2:43 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Sun, 30 Jun 2024 at 17:33, Zheyu Ma <zheyum...@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 <zheyum...@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 for your kind reminder. I'll check the bug list and ignore the devices which will be deprecated :) Regards, Zheyu
