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

[Alistair Francis]

On Sun, Jun 16, 2019 at 1:15 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Fri, 14 Jun 2019 05:25:50 PDT (-0700), address@hidden wrote:
> > 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.
>
> Works for me.

With the updated Suggested-by line:

Reviewed-by: Alistair Francis <address@hidden>

Alistair

>
> >
> >>
> >> Fixes: 30efbf330a45 ("SiFive RISC-V GPIO Device")
> >> Signed-off-by: Palmer Dabbelt <address@hidden>
> >
> > Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
> Thanks!
>
> >
> >> ---
> >>  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 {
> >>
>