| Message ID | 6e7748f1.25d8.17f2705c420.Coremail.wliang@stu.xidian.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a potential memory leak bug in write_boot_rom() (v6.2.0). | expand |
On 23/2/22 15:39, wliang@stu.xidian.edu.cn wrote: > Hi all, > > I find a memory leak bug in QEMU 6.2.0, which is in > write_boot_rom()(./hw/arm/aspeed.c). > > Specifically, at line 276, a memory chunk is allocated with g_new0() and > assigned to the variable 'storage'. However, if the branch takes true at > line 277, there will be only an error report at line 278 but not a free > operation for 'storage' before function returns. As a result, a memory > leak bug is triggered. > > > 259 BlockBackend *blk = blk_by_legacy_dinfo(dinfo); > ... > 276 storage = g_new0(uint8_t, rom_size); > 277 if (blk_pread(blk, 0, storage, rom_size) < 0) { > 278 error_setg(errp, "failed to read the initial flash content"); > 279 return; > 280 } > > > I believe that the problem can be fixed by adding a g_free() before the > function returns. > > > 277 if (blk_pread(blk, 0, storage, rom_size) < 0) { > 278 error_setg(errp, "failed to read the initial flash content"); > +++ g_free(storage); > 279 return; > 280 } > > > I'm looking forward to your confirmation. Correct. Or using g_autofree: -- >8 -- diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index d911dc904f..170e773ef8 100644 --- a/hw/arm/aspeed.c +++ b/hw/arm/aspeed.c @@ -257,7 +257,7 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size, Error **errp) { BlockBackend *blk = blk_by_legacy_dinfo(dinfo); - uint8_t *storage; + g_autofree void *storage = NULL; int64_t size; /* The block backend size should have already been 'validated' by @@ -273,14 +273,13 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size, rom_size = size; } - storage = g_new0(uint8_t, rom_size); + storage = g_malloc0(rom_size); if (blk_pread(blk, 0, storage, rom_size) < 0) { error_setg(errp, "failed to read the initial flash content"); return; } rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr); - g_free(storage); } ---
Hello Wentao, On 2/23/22 17:15, Philippe Mathieu-Daudé wrote: > On 23/2/22 15:39, wliang@stu.xidian.edu.cn wrote: >> Hi all, >> >> I find a memory leak bug in QEMU 6.2.0, which is in write_boot_rom()(./hw/arm/aspeed.c). >> >> Specifically, at line 276, a memory chunk is allocated with g_new0() and assigned to the variable 'storage'. However, if the branch takes true at line 277, there will be only an error report at line 278 but not a free operation for 'storage' before function returns. As a result, a memory leak bug is triggered. >> >> >> 259 BlockBackend *blk = blk_by_legacy_dinfo(dinfo); >> ... >> 276 storage = g_new0(uint8_t, rom_size); >> 277 if (blk_pread(blk, 0, storage, rom_size) < 0) { >> 278 error_setg(errp, "failed to read the initial flash content"); >> 279 return; >> 280 } >> >> >> I believe that the problem can be fixed by adding a g_free() before the function returns. >> >> >> 277 if (blk_pread(blk, 0, storage, rom_size) < 0) { >> 278 error_setg(errp, "failed to read the initial flash content"); >> +++ g_free(storage); >> 279 return; >> 280 } >> >> >> I'm looking forward to your confirmation. > > Correct. > > Or using g_autofree: yes. Could you please send a patch using g_autofree ? Thanks, C. > > -- >8 -- > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index d911dc904f..170e773ef8 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -257,7 +257,7 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size, > Error **errp) > { > BlockBackend *blk = blk_by_legacy_dinfo(dinfo); > - uint8_t *storage; > + g_autofree void *storage = NULL; > int64_t size; > > /* The block backend size should have already been 'validated' by > @@ -273,14 +273,13 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size, > rom_size = size; > } > > - storage = g_new0(uint8_t, rom_size); > + storage = g_malloc0(rom_size); > if (blk_pread(blk, 0, storage, rom_size) < 0) { > error_setg(errp, "failed to read the initial flash content"); > return; > } > > rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr); > - g_free(storage); > } > ---
> > yes. Could you please send a patch using g_autofree ? > > Thanks, > > C. Here is the new patch. Thanks, Wentao
On Fri, 25 Feb 2022 at 03:33, <wliang@stu.xidian.edu.cn> wrote: > > > > > > yes. Could you please send a patch using g_autofree ? > > > > Thanks, > > > > C. > > > Here is the new patch. Hi; that patch doesn't seem to be using g_autofree. Did you attach the wrong version of it? You've sent a few patches recently, and they're all attachments to the email. This is a bit awkward as our automatic tooling doesn't handle attached patches -- it wants them inline in the email. For a few one-off patches we can handle that at our end, but if you're planning to send many more patches in future it might be worth trying to sort out how to send them as non-attachments. https://www.qemu.org/docs/master/devel/submitting-a-patch.html has the details, including notes on using either git send-email or the sourcehut service. A couple more housekeeping type suggestions: * please don't send new versions of patches as followups to the email thread of the first patch; start a new thread * subject lines should start with a prefix indicating what part of the tree they apply to: in this case "hw/arm/aspeed". This helps people scanning the email list to pick out patches which they care about: from the function name alone it's often hard to figure out which part of QEMU is involved thanks -- PMM
--- ./hw/arm/aspeed.c 2022-02-23 15:06:31.928708083 +0800 +++ ./hw/arm/aspeed-PATCH.c 2022-02-23 21:22:28.200802801 +0800 @@ -276,6 +276,7 @@ storage = g_new0(uint8_t, rom_size); if (blk_pread(blk, 0, storage, rom_size) < 0) { error_setg(errp, "failed to read the initial flash content"); + g_free(storage); return; }
Hi all, I find a memory leak bug in QEMU 6.2.0, which is in write_boot_rom()(./hw/arm/aspeed.c). Specifically, at line 276, a memory chunk is allocated with g_new0() and assigned to the variable 'storage'. However, if the branch takes true at line 277, there will be only an error report at line 278 but not a free operation for 'storage' before function returns. As a result, a memory leak bug is triggered. 259 BlockBackend *blk = blk_by_legacy_dinfo(dinfo); ... 276 storage = g_new0(uint8_t, rom_size); 277 if (blk_pread(blk, 0, storage, rom_size) < 0) { 278 error_setg(errp, "failed to read the initial flash content"); 279 return; 280 } I believe that the problem can be fixed by adding a g_free() before the function returns. 277 if (blk_pread(blk, 0, storage, rom_size) < 0) { 278 error_setg(errp, "failed to read the initial flash content"); +++ g_free(storage); 279 return; 280 } I'm looking forward to your confirmation. Best, Wentao