[Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Nikita Belov]

Variable 'ram_lo' is allocated unconditionally, but used only in some cases.
When it is unused pointer will be lost at function exit, resulting in a
memory leak. Free memory in this case.

Valgrind output:
==16879== 240 bytes in 1 blocks are definitely lost in loss record 6,033 of 7,018
==16879==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
==16879==    by 0x509E610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==16879==    by 0x288836: realview_init (realview.c:55)
==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
==16879==    by 0x341426: main (vl.c:4413)

Signed-off-by: Nikita Belov <zodiac@ispras.ru>
---
 hw/arm/realview.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index af65aa4..673a540 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -141,6 +141,8 @@ static void realview_init(MachineState *machine,
                                &error_abort);
         vmstate_register_ram_global(ram_lo);
         memory_region_add_subregion(sysmem, 0x20000000, ram_lo);
+    } else {
+        g_free(ram_lo);
     }

     memory_region_init_ram(ram_hi, NULL, "realview.highmem", ram_size,
--
1.9.0.msysgit.0

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Peter Maydell]

On 29 October 2014 14:03, Nikita Belov <zodiac@ispras.ru> wrote:
> Variable 'ram_lo' is allocated unconditionally, but used only in some cases.
> When it is unused pointer will be lost at function exit, resulting in a
> memory leak. Free memory in this case.
>
> Valgrind output:
> ==16879== 240 bytes in 1 blocks are definitely lost in loss record 6,033 of 7,018
> ==16879==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
> ==16879==    by 0x509E610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==16879==    by 0x288836: realview_init (realview.c:55)
> ==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
> ==16879==    by 0x341426: main (vl.c:4413)
>
> Signed-off-by: Nikita Belov <zodiac@ispras.ru>
> ---
>  hw/arm/realview.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index af65aa4..673a540 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -141,6 +141,8 @@ static void realview_init(MachineState *machine,
>                                 &error_abort);
>          vmstate_register_ram_global(ram_lo);
>          memory_region_add_subregion(sysmem, 0x20000000, ram_lo);
> +    } else {
> +        g_free(ram_lo);
>      }
>
>      memory_region_init_ram(ram_hi, NULL, "realview.highmem", ram_size,

We leak all of the MemoryRegions we allocate here, because we
don't have a persistent state struct to keep them in. This
doesn't really matter much because they're generally needed
for the lifetime of the QEMU process anyway, and we only call
board init functions once. So why worry about ram_lo in
particular (and why this board in particular)?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Nikita Belov]

On 2014-10-29 19:03, Peter Maydell wrote:
> On 29 October 2014 14:03, Nikita Belov <zodiac@ispras.ru> wrote:
>> Variable 'ram_lo' is allocated unconditionally, but used only in some 
>> cases.
>> When it is unused pointer will be lost at function exit, resulting in 
>> a
>> memory leak. Free memory in this case.
>> 
>> Valgrind output:
>> ==16879== 240 bytes in 1 blocks are definitely lost in loss record 
>> 6,033 of 7,018
>> ==16879==    at 0x4C2AB80: malloc (in 
>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
>> ==16879==    by 0x509E610: g_malloc (in 
>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
>> ==16879==    by 0x288836: realview_init (realview.c:55)
>> ==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
>> ==16879==    by 0x341426: main (vl.c:4413)
>> 
>> Signed-off-by: Nikita Belov <zodiac@ispras.ru>
>> ---
>>  hw/arm/realview.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
>> index af65aa4..673a540 100644
>> --- a/hw/arm/realview.c
>> +++ b/hw/arm/realview.c
>> @@ -141,6 +141,8 @@ static void realview_init(MachineState *machine,
>>                                 &error_abort);
>>          vmstate_register_ram_global(ram_lo);
>>          memory_region_add_subregion(sysmem, 0x20000000, ram_lo);
>> +    } else {
>> +        g_free(ram_lo);
>>      }
>> 
>>      memory_region_init_ram(ram_hi, NULL, "realview.highmem", 
>> ram_size,
> 
> We leak all of the MemoryRegions we allocate here, because we
> don't have a persistent state struct to keep them in. This
> doesn't really matter much because they're generally needed
> for the lifetime of the QEMU process anyway, and we only call
> board init functions once. So why worry about ram_lo in
> particular (and why this board in particular)?
> 
> thanks
> -- PMM

Indeed, generally we need memory regions for the lifetime of QEMU, but 
'mem_lo'
is different. It may not be used at all. We use 'ram_lo' only when a 
condition is
true, in other case we will lose this pointer. Because of that if the 
condition is
false we have memory leak immediately (not when QEMU exits).

Semantically the touched code looks like this:
   if (/* condition */) {
       /* ... */
       memory_region_init_ram(ram_lo, NULL, "realview.lowmem", 
low_ram_size);
       vmstate_register_ram_global(ram_lo);
       memory_region_add_subregion(sysmem, 0x20000000, ram_lo);
+ } else {
+     g_free(ram_lo);
   }
   /* ram_lo is not used any more */

> and why this board in particular?
I just memory leak where I found it.

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Peter Maydell]

On 31 October 2014 10:42, Nikita Belov <zodiac@ispras.ru> wrote:
> On 2014-10-29 19:03, Peter Maydell wrote:
>> We leak all of the MemoryRegions we allocate here, because we
>> don't have a persistent state struct to keep them in. This
>> doesn't really matter much because they're generally needed
>> for the lifetime of the QEMU process anyway, and we only call
>> board init functions once. So why worry about ram_lo in
>> particular (and why this board in particular)?

> Indeed, generally we need memory regions for the lifetime of QEMU, but
> 'mem_lo'
> is different. It may not be used at all. We use 'ram_lo' only when a
> condition is
> true, in other case we will lose this pointer. Because of that if the
> condition is
> false we have memory leak immediately (not when QEMU exits).

No, ram_lo is exactly the same as the other memory regions
here: we allocate it in this function, we don't keep any
kind of pointer to it after we leave this function, and
we rely on it being freed on QEMU exit. The fact that we
don't happen to use ram_lo in all cases is irrelevant.

This isn't any more of a bug than the similar code for any of
the other memory regions in this board or in many of our other
boards. I don't think there's any point changing this code
unless you want to refactor the board so it is a proper
subclass of MachineState with its own state structure to
hold the MemoryRegion pointers in.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Kirill Batuzov]

On Fri, 31 Oct 2014, Peter Maydell wrote:

> On 31 October 2014 10:42, Nikita Belov <zodiac@ispras.ru> wrote:
> > On 2014-10-29 19:03, Peter Maydell wrote:
> >> We leak all of the MemoryRegions we allocate here, because we
> >> don't have a persistent state struct to keep them in. This
> >> doesn't really matter much because they're generally needed
> >> for the lifetime of the QEMU process anyway, and we only call
> >> board init functions once. So why worry about ram_lo in
> >> particular (and why this board in particular)?
> 
> > Indeed, generally we need memory regions for the lifetime of QEMU, but
> > 'mem_lo'
> > is different. It may not be used at all. We use 'ram_lo' only when a
> > condition is
> > true, in other case we will lose this pointer. Because of that if the
> > condition is
> > false we have memory leak immediately (not when QEMU exits).
> 
> No, ram_lo is exactly the same as the other memory regions
> here: we allocate it in this function, we don't keep any
> kind of pointer to it after we leave this function,

This is not true. We keep pointer to the memory region when we add it as
a subregion of another region. As long as we have a pointer to a root
region(s) we have a pointer to any other used region. Which is not the
case for unused ones.

Actually it is impossible to use a dinamically allocated piece of
memory and not to have a pointer to it at the same time.

> and
> we rely on it being freed on QEMU exit. The fact that we
> don't happen to use ram_lo in all cases is irrelevant.
>

-- 
Kirill

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Nikita Belov]

On 2014-10-29 17:03, Nikita Belov wrote:
> Variable 'ram_lo' is allocated unconditionally, but used only in some 
> cases.
> When it is unused pointer will be lost at function exit, resulting in 
> a
> memory leak. Free memory in this case.
> 
> Valgrind output:
> ==16879== 240 bytes in 1 blocks are definitely lost in loss record
> 6,033 of 7,018
> ==16879==    at 0x4C2AB80: malloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
> ==16879==    by 0x509E610: g_malloc (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==16879==    by 0x288836: realview_init (realview.c:55)
> ==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
> ==16879==    by 0x341426: main (vl.c:4413)
> 
> Signed-off-by: Nikita Belov <zodiac@ispras.ru>
> ---
>  hw/arm/realview.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index af65aa4..673a540 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -141,6 +141,8 @@ static void realview_init(MachineState *machine,
>                                 &error_abort);
>          vmstate_register_ram_global(ram_lo);
>          memory_region_add_subregion(sysmem, 0x20000000, ram_lo);
> +    } else {
> +        g_free(ram_lo);
>      }
> 
>      memory_region_init_ram(ram_hi, NULL, "realview.highmem", 
> ram_size,
> --
> 1.9.0.msysgit.0

ping

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Peter Maydell]

On 19 November 2014 15:05, Nikita Belov <zodiac@ispras.ru> wrote:
> ping

Not for 2.2, and I'm still not really convinced in
general that it's worthwhile at all.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Kirill Batuzov]

On Wed, 19 Nov 2014, Peter Maydell wrote:
> 
> Not for 2.2,

Fair enough.

> and I'm still not really convinced in
> general that it's worthwhile at all.
>

I'm surprised that this small patch caused so much controversy. It seems
very simple and straightforward to me.

This patch fixes a memory leak. The fact that it indeed was a memory
leak is indicated by Valgrind output (Memcheck's false-positives are
extremely rare unless you do some really nasty things with your pointers).
It can be verified manually too: there are only 4 occurrences of 'ram_lo'
in realview.c.

By fixing memory leak this patch silences warnings from automatic checking
tools like Valgrind. Not having minor warnings is good because it simplifies
usage of such tools in order to find new and important bugs.

This patch is local: it does not affect any other function except
realview_init.

Given all this I can see benefits of this patch with no real downsides to it.
Is this enough to proof it's worthwhile?

-- 
Kirill

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Peter Maydell]

On 20 November 2014 11:53, Kirill Batuzov <batuzovk@ispras.ru> wrote:
> I'm surprised that this small patch caused so much controversy. It seems
> very simple and straightforward to me.
>
> This patch fixes a memory leak. The fact that it indeed was a memory
> leak is indicated by Valgrind output (Memcheck's false-positives are
> extremely rare unless you do some really nasty things with your pointers).
> It can be verified manually too: there are only 4 occurrences of 'ram_lo'
> in realview.c.

It's in exactly the same situation as the other blocks of memory
like ram_hi in that file: we allocate it and then don't care about
freeing it, because we don't happen to have a board state struct.
The correct fix if you care about this kind of thing would be
to have a board state struct which had MemoryRegion fields (not
MemoryRegion* fields). We have lots of bits of memory that we
allocate once on startup and then don't care about freeing.

I'll probably put it in, because it's not very harmful. It just
doesn't seem to me very useful to merely silence the warning
rather than actually fixing the underlying thing that the
warning is telling you about.

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Kirill Batuzov]

> On 20 November 2014 11:53, Kirill Batuzov <batuzovk@ispras.ru> wrote:
> > I'm surprised that this small patch caused so much controversy. It seems
> > very simple and straightforward to me.
> >
> > This patch fixes a memory leak. The fact that it indeed was a memory
> > leak is indicated by Valgrind output (Memcheck's false-positives are
> > extremely rare unless you do some really nasty things with your pointers).
> > It can be verified manually too: there are only 4 occurrences of 'ram_lo'
> > in realview.c.
> 
> It's in exactly the same situation as the other blocks of memory
> like ram_hi in that file: we allocate it and then don't care about
> freeing it, because we don't happen to have a board state struct.
> The correct fix if you care about this kind of thing would be
> to have a board state struct which had MemoryRegion fields (not
> MemoryRegion* fields). We have lots of bits of memory that we
> allocate once on startup and then don't care about freeing.
>

I think we are talking about a bit different problems here. Indeed
ram_hi is allocated, then used until QEMU exits but is never freed. Yet
it is never completely lost: there is at least one pointer to it in
memory hierarchy. Valgrind calls such situations "still reachable" and
does not consider them errors (because memory is in use until the very
moment the program exits; at least it can not be proven different).

ram_lo is different. It can be added to memory hierarchy in which case
it will behave exactly the same way as ram_hi. But it may be not used at
all in which case all pointers to it will be lost. This is the real
memory leak. Valgrind reports such situations as "definitely lost" and
they are considered errors (because it can be proven that memory was
allocated, is not in use and was not freed).

In our case ram_lo was reported as "definitely lost" while ram_hi was
"still reachable" and was never reported as error.

This patch addresses the second problem (when ram_lo is "definitely
lost") because it has very short and simple solution. While you are
arguing that we need to address the first problem - which is also true
but it is different problem that will need different solution and a much
larger one.

> It just
> doesn't seem to me very useful to merely silence the warning
> rather than actually fixing the underlying thing that the
> warning is telling you about.
> 

As I described above, it actually solves the problem Valgrind reports.
It is just a different problem than you are talking about.


> I'll probably put it in, because it's not very harmful.

Either way is fine with me. I'm still sure this patch is worthwhile but on
the other hand it is not that big of an issue to be arguing about it for
too long.

-- 
Kirill

Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Markus Armbruster]

Nikita Belov <zodiac@ispras.ru> writes:

> Variable 'ram_lo' is allocated unconditionally, but used only in some cases.
> When it is unused pointer will be lost at function exit, resulting in a
> memory leak. Free memory in this case.
>
> Valgrind output:
> ==16879== 240 bytes in 1 blocks are definitely lost in loss record 6,033 of 7,018
> ==16879==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
> ==16879==    by 0x509E610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==16879==    by 0x288836: realview_init (realview.c:55)
> ==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
> ==16879==    by 0x341426: main (vl.c:4413)
>
> Signed-off-by: Nikita Belov <zodiac@ispras.ru>
> ---
>  hw/arm/realview.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index af65aa4..673a540 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -141,6 +141,8 @@ static void realview_init(MachineState *machine,
>                                 &error_abort);
>          vmstate_register_ram_global(ram_lo);
>          memory_region_add_subregion(sysmem, 0x20000000, ram_lo);
> +    } else {
> +        g_free(ram_lo);
>      }
>
>      memory_region_init_ram(ram_hi, NULL, "realview.highmem", ram_size,
> --
> 1.9.0.msysgit.0

ram_log is allocate unconditionally, but used only conditionally.  Your
patch frees it when it's not used.  Allocating it only when it's used
would be simpler.

[Qemu-devel] [PATCH v2] hw/arm/realview.c: Fix memory leak in realview_init()

[Nikita Belov]

Variable 'ram_lo' is allocated unconditionally, but used only in some cases.
When it is unused pointer will be lost at function exit, resulting in a
memory leak. Allocate memory for 'ram_lo' only if it is needed.

Valgrind output:
==16879== 240 bytes in 1 blocks are definitely lost in loss record 6,033 of 7,018
==16879==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
==16879==    by 0x509E610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
==16879==    by 0x288836: realview_init (realview.c:55)
==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
==16879==    by 0x341426: main (vl.c:4413)

Signed-off-by: Nikita Belov <zodiac@ispras.ru>
---
 hw/arm/realview.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index af65aa4..9bf2392 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -52,7 +52,7 @@ static void realview_init(MachineState *machine,
     CPUARMState *env;
     ObjectClass *cpu_oc;
     MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram_lo = g_new(MemoryRegion, 1);
+    MemoryRegion *ram_lo;
     MemoryRegion *ram_hi = g_new(MemoryRegion, 1);
     MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
     MemoryRegion *ram_hack = g_new(MemoryRegion, 1);
@@ -135,6 +135,7 @@ static void realview_init(MachineState *machine,
 
     if (is_pb && ram_size > 0x20000000) {
         /* Core tile RAM.  */
+        ram_lo = g_new(MemoryRegion, 1);
         low_ram_size = ram_size - 0x20000000;
         ram_size = 0x20000000;
         memory_region_init_ram(ram_lo, NULL, "realview.lowmem", low_ram_size,
-- 
1.9.0.msysgit.0

Re: [Qemu-devel] [PATCH v2] hw/arm/realview.c: Fix memory leak in realview_init()

[Markus Armbruster]

Nikita Belov <zodiac@ispras.ru> writes:

> Variable 'ram_lo' is allocated unconditionally, but used only in some cases.
> When it is unused pointer will be lost at function exit, resulting in a
> memory leak. Allocate memory for 'ram_lo' only if it is needed.
>
> Valgrind output:
> ==16879== 240 bytes in 1 blocks are definitely lost in loss record 6,033 of 7,018
> ==16879==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
> ==16879==    by 0x509E610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==16879==    by 0x288836: realview_init (realview.c:55)
> ==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
> ==16879==    by 0x341426: main (vl.c:4413)
>
> Signed-off-by: Nikita Belov <zodiac@ispras.ru>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2] hw/arm/realview.c: Fix memory leak in realview_init()

[Peter Maydell]

On 21 November 2014 at 13:51, Nikita Belov <zodiac@ispras.ru> wrote:
> Variable 'ram_lo' is allocated unconditionally, but used only in some cases.
> When it is unused pointer will be lost at function exit, resulting in a
> memory leak. Allocate memory for 'ram_lo' only if it is needed.
>
> Valgrind output:
> ==16879== 240 bytes in 1 blocks are definitely lost in loss record 6,033 of 7,018
> ==16879==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==16879==    by 0x33D2CE: malloc_and_trace (vl.c:2804)
> ==16879==    by 0x509E610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0)
> ==16879==    by 0x288836: realview_init (realview.c:55)
> ==16879==    by 0x28988C: realview_pb_a8_init (realview.c:375)
> ==16879==    by 0x341426: main (vl.c:4413)
>
> Signed-off-by: Nikita Belov <zodiac@ispras.ru>
> ---

Applied to target-arm.next; thanks.

-- PMM