Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

[wliang]

[-- Attachment #1.1: Type: text/plain, Size: 1004 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 5048 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aspeed.c.patch --]
[-- Type: text/x-patch; name=aspeed.c.patch, Size: 360 bytes --]

--- ./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;
     }
 

Re: Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

[Philippe Mathieu-Daudé]

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);
  }
---

Re: Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

[Cédric Le Goater]

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);
>   }
> ---

Re: Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

[wliang]

[-- Attachment #1: Type: text/plain, Size: 122 bytes --]


> 
> yes. Could you please send a patch using  g_autofree ?
> 
> Thanks,
> 
> C.


Here is the new patch.

Thanks,
Wentao

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-a-potential-memory-leak-bug-in-write_boot_rom-v6.patch --]
[-- Type: text/x-patch;  name=0001-Fix-a-potential-memory-leak-bug-in-write_boot_rom-v6.patch, Size: 798 bytes --]

From 8ed76446f78ab1b4152403fdb9dd6f349d6fd52e Mon Sep 17 00:00:00 2001
From: Wentao_Liang <Wentao_Liang_g@163.com>
Date: Fri, 25 Feb 2022 11:17:33 +0800
Subject: [PATCH] Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
---
 hw/arm/aspeed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index d911dc904f..9da5f87429 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -276,6 +276,7 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
     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;
     }
 
-- 
2.25.1

Re: Fix a potential memory leak bug in write_boot_rom() (v6.2.0).

[Peter Maydell]

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