| Message ID | 20240630163225.2973081-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/usb: Fix memory leak in musb_reset() | expand |
> -----Original Message----- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org > <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of Zheyu > Ma > Sent: Monday, July 1, 2024 12:32 AM > Cc: Zheyu Ma <zheyuma97@gmail.com>; qemu-devel@nongnu.org > Subject: [PATCH] hw/usb: Fix memory leak in musb_reset() > > 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); > } > -- > 2.34.1 > Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
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
Hi Peter, On Mon, Jul 1, 2024 at 2:43 PM Peter Maydell <peter.maydell@linaro.org> wrote: > 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 for your kind reminder. I'll check the bug list and ignore the devices which will be deprecated :) Regards, Zheyu
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); }
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(+)