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

Palmer Dabbelt posted 1 patch 4 years, 9 months ago
Test s390x passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190614120830.21850-1-palmer@sifive.com
Maintainers: Alistair Francis <Alistair.Francis@wdc.com>, Palmer Dabbelt <palmer@sifive.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
hw/riscv/sifive_e.c         | 12 +++++-------
include/hw/riscv/sifive_e.h |  2 ++
2 files changed, 7 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
Posted by Palmer Dabbelt 4 years, 9 months ago
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


Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
Posted by Philippe Mathieu-Daudé 4 years, 9 months ago
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 {
> 

Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
Posted by Palmer Dabbelt 4 years, 9 months ago
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 {
>>

Re: [Qemu-devel] [PATCH] RISC-V: Fix a memory leak when realizing a sifive_e
Posted by Alistair Francis 4 years, 9 months ago
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 {
> >>
>