Re: [PATCH] hw/loongarch/virt: Fix memory leak

[Philippe Mathieu-Daudé]

On 7/5/24 04:22, Song Gao wrote:

The char pointer 'ramName' point to a block of memory,
but never free it. Use 'g_autofree' to automatically free it.

Resolves: Coverity CID 1544773

Fixes: 0cf1478d6 ("hw/loongarch: Add numa support")
Signed-off-by: Song Gao 
---
  hw/loongarch/virt.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)


Thanks, patch queued to hw-misc tree.

Re: [PATCH] hw/loongarch/virt: Fix memory leak

[Peter Maydell]

On Tue, 7 May 2024 at 10:52, Michael Tokarev  wrote:
>
> 07.05.2024 05:22, Song Gao wrote:
>
> >   for (i = 1; i < nb_numa_nodes; i++) {
> >   MemoryRegion *nodemem = g_new(MemoryRegion, 1);
> > -ramName = g_strdup_printf("loongarch.node%d.ram", i);
> > +g_autofree char *ramName = g_strdup_printf("loongarch.node%d.ram", 
> > i);
>
> Can't this be a fixed-size buffer on stack?

No, this is a really bad idea. It's a pain to audit that the
array really doesn't get overwritten, and if the string we want
to write changes, now we have to re-count characters to decide
if we need to increase the size of the array. The memory allocation
on the heap here is a tiny overhead that we only incur at startup.
The g_autofree approach is much better.

For this version of the patch:

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH] hw/loongarch/virt: Fix memory leak

[gaosong]

在 2024/5/7 下午5:52, Michael Tokarev 写道:

07.05.2024 05:22, Song Gao wrote:


  for (i = 1; i < nb_numa_nodes; i++) {
  MemoryRegion *nodemem = g_new(MemoryRegion, 1);
-    ramName = g_strdup_printf("loongarch.node%d.ram", i);
+    g_autofree char *ramName = 
g_strdup_printf("loongarch.node%d.ram", i);


Can't this be a fixed-size buffer on stack?

Maybe I'm old-minded, but such obviously fixed and
very small allocations on the heap hurt my eyes ;)


I had send v2 patch.

Thanks.
Song Gao

/mjt

Re: [PATCH] hw/loongarch/virt: Fix memory leak

[Michael Tokarev]

07.05.2024 05:22, Song Gao wrote:


  for (i = 1; i < nb_numa_nodes; i++) {
  MemoryRegion *nodemem = g_new(MemoryRegion, 1);
-ramName = g_strdup_printf("loongarch.node%d.ram", i);
+g_autofree char *ramName = g_strdup_printf("loongarch.node%d.ram", i);


Can't this be a fixed-size buffer on stack?

Maybe I'm old-minded, but such obviously fixed and
very small allocations on the heap hurt my eyes ;)

/mjt
--
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 
ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 
8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH] hw/loongarch/virt: Fix memory leak

[Philippe Mathieu-Daudé]

On 7/5/24 04:22, Song Gao wrote:

The char pointer 'ramName' point to a block of memory,
but never free it. Use 'g_autofree' to automatically free it.

Resolves: Coverity CID 1544773

Fixes: 0cf1478d6 ("hw/loongarch: Add numa support")
Signed-off-by: Song Gao 
---
  hw/loongarch/virt.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

[PATCH] hw/loongarch/virt: Fix memory leak

[Song Gao]

The char pointer 'ramName' point to a block of memory,
but never free it. Use 'g_autofree' to automatically free it.

Resolves: Coverity CID 1544773

Fixes: 0cf1478d6 ("hw/loongarch: Add numa support")
Signed-off-by: Song Gao 
---
 hw/loongarch/virt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index c0999878df..ea5100be6d 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -887,7 +887,6 @@ static void loongarch_init(MachineState *machine)
 const CPUArchIdList *possible_cpus;
 MachineClass *mc = MACHINE_GET_CLASS(machine);
 CPUState *cpu;
-char *ramName = NULL;
 
 if (!cpu_model) {
 cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
@@ -946,7 +945,7 @@ static void loongarch_init(MachineState *machine)
 
 for (i = 1; i < nb_numa_nodes; i++) {
 MemoryRegion *nodemem = g_new(MemoryRegion, 1);
-ramName = g_strdup_printf("loongarch.node%d.ram", i);
+g_autofree char *ramName = g_strdup_printf("loongarch.node%d.ram", i);
 memory_region_init_alias(nodemem, NULL, ramName, machine->ram,
  offset,  numa_info[i].node_mem);
 memory_region_add_subregion(address_space_mem, phyAddr, nodemem);
-- 
2.25.1