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

["Cédric Le Goater" <clg@kaod.org>]

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