[Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Stefan Weil]

Extract from "man realloc":
"If realloc() fails the original block is left untouched;
it is not freed or moved."

Fix a possible memory leak (reported by cppcheck).

Cc: Riku Voipio <riku.voipio@iki.fi>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 linux-user/elfload.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index ab03e16..f9bd849 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1474,7 +1474,7 @@ static void load_symbols(struct elfhdr *hdr, int fd, abi_ulong load_bias)
     struct elf_shdr *shdr;
     char *strings;
     struct syminfo *s;
-    struct elf_sym *syms;
+    struct elf_sym *syms, *new_syms;
 
     shnum = hdr->e_shnum;
     i = shnum * sizeof(struct elf_shdr);
@@ -1543,12 +1543,14 @@ static void load_symbols(struct elfhdr *hdr, int fd, abi_ulong load_bias)
        that we threw away.  Whether or not this has any effect on the
        memory allocation depends on the malloc implementation and how
        many symbols we managed to discard.  */
-    syms = realloc(syms, nsyms * sizeof(*syms));
-    if (syms == NULL) {
+    new_syms = realloc(syms, nsyms * sizeof(*syms));
+    if (new_syms == NULL) {
         free(s);
+        free(syms);
         free(strings);
         return;
     }
+    syms = new_syms;
 
     qsort(syms, nsyms, sizeof(*syms), symcmp);
 
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Markus Armbruster]

Stefan Weil <weil@mail.berlios.de> writes:

> Extract from "man realloc":
> "If realloc() fails the original block is left untouched;
> it is not freed or moved."
>
> Fix a possible memory leak (reported by cppcheck).
>
> Cc: Riku Voipio <riku.voipio@iki.fi>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

Sidestep the problem via qemu_realloc() instead?

Re: [Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Stefan Weil]

Am 18.01.2011 09:26, schrieb Markus Armbruster:
> Stefan Weil <weil@mail.berlios.de> writes:
>
>> Extract from "man realloc":
>> "If realloc() fails the original block is left untouched;
>> it is not freed or moved."
>>
>> Fix a possible memory leak (reported by cppcheck).
>>
>> Cc: Riku Voipio <riku.voipio@iki.fi>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>
> Sidestep the problem via qemu_realloc() instead?

The same change was applied to bsd-user/elfload.c.

As symbol loading is not essential in most applications,
returning after out-of-memory should be better than
aborting (that's what qemu_realloc does).

Re: [Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Markus Armbruster]

Stefan Weil <weil@mail.berlios.de> writes:

> Am 18.01.2011 09:26, schrieb Markus Armbruster:
>> Stefan Weil <weil@mail.berlios.de> writes:
>>
>>> Extract from "man realloc":
>>> "If realloc() fails the original block is left untouched;
>>> it is not freed or moved."
>>>
>>> Fix a possible memory leak (reported by cppcheck).
>>>
>>> Cc: Riku Voipio <riku.voipio@iki.fi>
>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>
>> Sidestep the problem via qemu_realloc() instead?
>
> The same change was applied to bsd-user/elfload.c.
>
> As symbol loading is not essential in most applications,
> returning after out-of-memory should be better than
> aborting (that's what qemu_realloc does).

Unless the requested size is *really* large, I'd expect this to stave
off the out-of-memory failure for a few microseconds at best.

Re: [Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Stefan Weil]

Am 18.01.2011 18:51, schrieb Markus Armbruster:
> Stefan Weil<weil@mail.berlios.de>  writes:
>
>    
>> Am 18.01.2011 09:26, schrieb Markus Armbruster:
>>      
>>> Stefan Weil<weil@mail.berlios.de>  writes:
>>>
>>>        
>>>> Extract from "man realloc":
>>>> "If realloc() fails the original block is left untouched;
>>>> it is not freed or moved."
>>>>
>>>> Fix a possible memory leak (reported by cppcheck).
>>>>
>>>> Cc: Riku Voipio<riku.voipio@iki.fi>
>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>>>>          
>>> Sidestep the problem via qemu_realloc() instead?
>>>        
>> The same change was applied to bsd-user/elfload.c.
>>
>> As symbol loading is not essential in most applications,
>> returning after out-of-memory should be better than
>> aborting (that's what qemu_realloc does).
>>      
> Unless the requested size is *really* large, I'd expect this to stave
> off the out-of-memory failure for a few microseconds at best.
>    

If realloc fails, some memory is released before returning,
so maybe you would be surprised that your program
finishes without any more problems :-)

Re: [Qemu-devel] [PATCH] linux-user: Fix possible realloc memory leak

[Peter Maydell]

On 18 January 2011 17:51, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Weil <weil@mail.berlios.de> writes:
>> Am 18.01.2011 09:26, schrieb Markus Armbruster:
>>> Stefan Weil <weil@mail.berlios.de> writes:
>>>> Extract from "man realloc":
>>>> "If realloc() fails the original block is left untouched;
>>>> it is not freed or moved."

>>> Sidestep the problem via qemu_realloc() instead?
>>
>> The same change was applied to bsd-user/elfload.c.
>>
>> As symbol loading is not essential in most applications,
>> returning after out-of-memory should be better than
>> aborting (that's what qemu_realloc does).
>
> Unless the requested size is *really* large, I'd expect this to stave
> off the out-of-memory failure for a few microseconds at best.

Yeah, but the patch is OK, it fixes an actual bug and
it does so in line with the malloc-failure handling of
the rest of the function. It doesn't seem to me to
be important enough an issue to worry about one way
or the other.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM