diff mbox series

hw/loongarch/virt: Fix memory leak

Message ID 20240507022239.3113987-1-gaosong@loongson.cn
State New
Headers show
Series hw/loongarch/virt: Fix memory leak | expand

Commit Message

Song Gao May 7, 2024, 2:22 a.m. UTC
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 <gaosong@loongson.cn>
---
 hw/loongarch/virt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé May 7, 2024, 7:28 a.m. UTC | #1
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 <gaosong@loongson.cn>
> ---
>   hw/loongarch/virt.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Michael Tokarev May 7, 2024, 9:52 a.m. UTC | #2
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
Song Gao May 8, 2024, 2:49 a.m. UTC | #3
在 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
Peter Maydell May 8, 2024, 1:05 p.m. UTC | #4
On Tue, 7 May 2024 at 10:52, Michael Tokarev <mjt@tls.msk.ru> 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 <peter.maydell@linaro.org>

thanks
-- PMM
Philippe Mathieu-Daudé May 8, 2024, 9:17 p.m. UTC | #5
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 <gaosong@loongson.cn>
> ---
>   hw/loongarch/virt.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Thanks, patch queued to hw-misc tree.
diff mbox series

Patch

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);