Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e

[Philippe Mathieu-Daudé]

On 6/14/19 2:08 PM, Palmer Dabbelt wrote:
> Coverity pointed out a memory leak in riscv_sifive_e_soc_realize(),
> where a pair of recently added MemoryRegion instances would not be freed
> if there were errors elsewhere in the function.  The fix here is to
> simply not use dynamic allocation for these instances: there's always
> one of each in SiFiveESoCState, so instead we just include them within
> the struct.
> 
> Thanks to Peter for pointing out the bug and suggesting the fix!

a.k.a. Suggested-by: Peter Maydell <address@hidden>

Maybe the thanks can go below the '---' tag, so it doesn't stay in the
git history.

> 
> Fixes: 30efbf330a45 ("SiFive RISC-V GPIO Device")
> Signed-off-by: Palmer Dabbelt <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  hw/riscv/sifive_e.c         | 12 +++++-------
>  include/hw/riscv/sifive_e.h |  2 ++
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 80ac56fa7d5e..83375afcd1d6 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -158,17 +158,15 @@ static void riscv_sifive_e_soc_realize(DeviceState 
> *dev, Error **errp)
>  
>      SiFiveESoCState *s = RISCV_E_SOC(dev);
>      MemoryRegion *sys_mem = get_system_memory();
> -    MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> -    MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>  
>      object_property_set_bool(OBJECT(&s->cpus), true, "realized",
>                              &error_abort);
>  
>      /* Mask ROM */
> -    memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
> +    memory_region_init_rom(&s->mask_rom, NULL, "riscv.sifive.e.mrom",
>          memmap[SIFIVE_E_MROM].size, &error_fatal);
>      memory_region_add_subregion(sys_mem,
> -        memmap[SIFIVE_E_MROM].base, mask_rom);
> +        memmap[SIFIVE_E_MROM].base, &s->mask_rom);
>  
>      /* MMIO */
>      s->plic = sifive_plic_create(memmap[SIFIVE_E_PLIC].base,
> @@ -228,10 +226,10 @@ static void riscv_sifive_e_soc_realize(DeviceState 
> *dev, Error **errp)
>          memmap[SIFIVE_E_PWM2].base, memmap[SIFIVE_E_PWM2].size);
>  
>      /* Flash memory */
> -    memory_region_init_ram(xip_mem, NULL, "riscv.sifive.e.xip",
> +    memory_region_init_ram(&s->xip_mem, NULL, "riscv.sifive.e.xip",
>          memmap[SIFIVE_E_XIP].size, &error_fatal);
> -    memory_region_set_readonly(xip_mem, true);
> -    memory_region_add_subregion(sys_mem, memmap[SIFIVE_E_XIP].base, xip_mem);
> +    memory_region_set_readonly(&s->xip_mem, true);
> +    memory_region_add_subregion(sys_mem, memmap[SIFIVE_E_XIP].base, 
> &s->xip_mem);
>  }
>  
>  static void riscv_sifive_e_machine_init(MachineClass *mc)
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 3b14eb74621f..d175b24cb209 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -33,6 +33,8 @@ typedef struct SiFiveESoCState {
>      RISCVHartArrayState cpus;
>      DeviceState *plic;
>      SIFIVEGPIOState gpio;
> +    MemoryRegion xip_mem;
> +    MemoryRegion mask_rom;
>  } SiFiveESoCState;
>  
>  typedef struct SiFiveEState {
>