* [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
@ 2019-06-14 12:08 Palmer Dabbelt
2019-06-14 12:25 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 4+ messages in thread
From: Palmer Dabbelt @ 2019-06-14 12:08 UTC (permalink / raw)
To: qemu-riscv; +Cc: Palmer Dabbelt, qemu-devel
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!
Fixes: 30efbf330a45 ("SiFive RISC-V GPIO Device")
Signed-off-by: Palmer Dabbelt <palmer@sifive.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 {
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
2019-06-14 12:08 [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e Palmer Dabbelt
@ 2019-06-14 12:25 ` Philippe Mathieu-Daudé
2019-06-16 8:15 ` Palmer Dabbelt
0 siblings, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-14 12:25 UTC (permalink / raw)
To: Palmer Dabbelt, qemu-riscv; +Cc: qemu-devel
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.maydell@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 <palmer@sifive.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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 {
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
2019-06-14 12:25 ` Philippe Mathieu-Daudé
@ 2019-06-16 8:15 ` Palmer Dabbelt
2019-06-17 17:18 ` Alistair Francis
0 siblings, 1 reply; 4+ messages in thread
From: Palmer Dabbelt @ 2019-06-16 8:15 UTC (permalink / raw)
To: philmd; +Cc: qemu-riscv, qemu-devel
On Fri, 14 Jun 2019 05:25:50 PDT (-0700), philmd@redhat.com 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 <peter.maydell@linaro.org>
>
> Maybe the thanks can go below the '---' tag, so it doesn't stay in the
> git history.
Works for me.
>
>>
>> Fixes: 30efbf330a45 ("SiFive RISC-V GPIO Device")
>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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 {
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
2019-06-16 8:15 ` Palmer Dabbelt
@ 2019-06-17 17:18 ` Alistair Francis
0 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2019-06-17 17:18 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: open list:RISC-V, Philippe Mathieu-Daudé,
qemu-devel@nongnu.org Developers
On Sun, Jun 16, 2019 at 1:15 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Fri, 14 Jun 2019 05:25:50 PDT (-0700), philmd@redhat.com 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 <peter.maydell@linaro.org>
> >
> > 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 <alistair.francis@wdc.com>
Alistair
>
> >
> >>
> >> Fixes: 30efbf330a45 ("SiFive RISC-V GPIO Device")
> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> 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 {
> >>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-17 17:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-14 12:08 [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e Palmer Dabbelt
2019-06-14 12:25 ` Philippe Mathieu-Daudé
2019-06-16 8:15 ` Palmer Dabbelt
2019-06-17 17:18 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).