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 <peter.mayd...@linaro.org>
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 <pal...@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> ---
> 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 {
>
