[Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks

[Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks

[Stefan Weil]

These errors were reported by cppcheck:

bsd-user/elfload.c:1076: error: Memory leak: s
bsd-user/elfload.c:1079: error: Memory leak: syms

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 bsd-user/elfload.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 7374912..313ddc6 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -1072,11 +1072,16 @@ static void load_symbols(struct elfhdr *hdr, int fd)
     /* Now know where the strtab and symtab are.  Snarf them. */
     s = malloc(sizeof(*s));
     syms = malloc(symtab.sh_size);
-    if (!syms)
+    if (!syms) {
+        free(s);
         return;
+    }
     s->disas_strtab = strings = malloc(strtab.sh_size);
-    if (!s->disas_strtab)
+    if (!s->disas_strtab) {
+        free(s);
+        free(syms);
         return;
+    }
 
     lseek(fd, symtab.sh_offset, SEEK_SET);
     if (read(fd, syms, symtab.sh_size) != symtab.sh_size)
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH] bsd-user: Fix possible memory leaks

[Blue Swirl]

On Sun, Jan 16, 2011 at 12:56 PM, Stefan Weil <weil@mail.berlios.de> wrote:
> These errors were reported by cppcheck:
>
> bsd-user/elfload.c:1076: error: Memory leak: s
> bsd-user/elfload.c:1079: error: Memory leak: syms
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  bsd-user/elfload.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 7374912..313ddc6 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -1072,11 +1072,16 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>     /* Now know where the strtab and symtab are.  Snarf them. */
>     s = malloc(sizeof(*s));
>     syms = malloc(symtab.sh_size);
> -    if (!syms)
> +    if (!syms) {

If we used qemu_malloc(), this wouldn't happen since it will exit if
malloc() fails. But is that OK, maybe we want to load the file without
symbols then?

Re: [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks

[Peter Maydell]

On 16 January 2011 12:56, Stefan Weil <weil@mail.berlios.de> wrote:
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 7374912..313ddc6 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -1072,11 +1072,16 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>     /* Now know where the strtab and symtab are.  Snarf them. */
>     s = malloc(sizeof(*s));
>     syms = malloc(symtab.sh_size);
> -    if (!syms)
> +    if (!syms) {
> +        free(s);
>         return;
> +    }
>     s->disas_strtab = strings = malloc(strtab.sh_size);
> -    if (!s->disas_strtab)
> +    if (!s->disas_strtab) {
> +        free(s);
> +        free(syms);
>         return;
> +    }
>
>     lseek(fd, symtab.sh_offset, SEEK_SET);
>     if (read(fd, syms, symtab.sh_size) != symtab.sh_size)

Don't we also need to free s, syms and strings in the
places later in the function where we return early
(if read() calls fail)?

The function also has an unchecked call to realloc().

(All these things are handled correctly in the linux-user/
version of this function.)

Blue Swirl wrote:
> If we used qemu_malloc(), this wouldn't happen since it will exit if
> malloc() fails. But is that OK, maybe we want to load the file without
> symbols then?

AFAICT symbols are only used for debug tracing, so carrying
on without them is a reasonable strategy. (Although if you
fail a malloc this early on the chances of successfully running
anything are not good, so it's a bit moot.)

-- PMM

Re: [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks

[Stefan Weil]

Am 16.01.2011 15:07, schrieb Peter Maydell:
> On 16 January 2011 12:56, Stefan Weil <weil@mail.berlios.de> wrote:
>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>> index 7374912..313ddc6 100644
>> --- a/bsd-user/elfload.c
>> +++ b/bsd-user/elfload.c
>> @@ -1072,11 +1072,16 @@ static void load_symbols(struct elfhdr *hdr, 
>> int fd)
>>     /* Now know where the strtab and symtab are.  Snarf them. */
>>     s = malloc(sizeof(*s));
>>     syms = malloc(symtab.sh_size);
>> -    if (!syms)
>> +    if (!syms) {
>> +        free(s);
>>         return;
>> +    }
>>     s->disas_strtab = strings = malloc(strtab.sh_size);
>> -    if (!s->disas_strtab)
>> +    if (!s->disas_strtab) {
>> +        free(s);
>> +        free(syms);
>>         return;
>> +    }
>>
>>     lseek(fd, symtab.sh_offset, SEEK_SET);
>>     if (read(fd, syms, symtab.sh_size) != symtab.sh_size)
>
> Don't we also need to free s, syms and strings in the
> places later in the function where we return early
> (if read() calls fail)?

You are correct. cppcheck only reports the first location in a function.
I should have called it again after my modifications.

>
> The function also has an unchecked call to realloc().
>
> (All these things are handled correctly in the linux-user/
> version of this function.)

The realloc() is handled in linux-user, but not in a correct way.

A new version of my patch fixes the missing memory leaks
and realloc for bsd-user.

A second patch is needed to fix realloc for linux-user, too.

> Blue Swirl wrote:
>> If we used qemu_malloc(), this wouldn't happen since it will exit if
>> malloc() fails. But is that OK, maybe we want to load the file without
>> symbols then?
>
> AFAICT symbols are only used for debug tracing, so carrying
> on without them is a reasonable strategy. (Although if you
> fail a malloc this early on the chances of successfully running
> anything are not good, so it's a bit moot.)
>
> -- PMM

[Qemu-devel] [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call

[Stefan Weil]

These errors were reported by cppcheck:

[bsd-user/elfload.c:1108]: (error) Common realloc mistake: "syms" nulled but not freed upon failure
[bsd-user/elfload.c:1076]: (error) Memory leak: s
[bsd-user/elfload.c:1079]: (error) Memory leak: syms

v2:
* The previous fix for memory leaks was incomplete (thanks to Peter Maydell for te hint).
* Fix wrong realloc usage, too.

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 bsd-user/elfload.c |   37 +++++++++++++++++++++++++++++++------
 1 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 7374912..1ef1f97 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -1044,7 +1044,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
     struct elf_shdr sechdr, symtab, strtab;
     char *strings;
     struct syminfo *s;
-    struct elf_sym *syms;
+    struct elf_sym *syms, *new_syms;
 
     lseek(fd, hdr->e_shoff, SEEK_SET);
     for (i = 0; i < hdr->e_shnum; i++) {
@@ -1072,15 +1072,24 @@ static void load_symbols(struct elfhdr *hdr, int fd)
     /* Now know where the strtab and symtab are.  Snarf them. */
     s = malloc(sizeof(*s));
     syms = malloc(symtab.sh_size);
-    if (!syms)
+    if (!syms) {
+        free(s);
         return;
+    }
     s->disas_strtab = strings = malloc(strtab.sh_size);
-    if (!s->disas_strtab)
+    if (!s->disas_strtab) {
+        free(s);
+        free(syms);
         return;
+    }
 
     lseek(fd, symtab.sh_offset, SEEK_SET);
-    if (read(fd, syms, symtab.sh_size) != symtab.sh_size)
+    if (read(fd, syms, symtab.sh_size) != symtab.sh_size) {
+        free(s);
+        free(syms);
+        free(strings);
         return;
+    }
 
     nsyms = symtab.sh_size / sizeof(struct elf_sym);
 
@@ -1105,13 +1114,29 @@ static void load_symbols(struct elfhdr *hdr, int fd)
 #endif
         i++;
     }
-    syms = realloc(syms, nsyms * sizeof(*syms));
+
+     /* Attempt to free the storage associated with the local symbols
+        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. */
+    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);
 
     lseek(fd, strtab.sh_offset, SEEK_SET);
-    if (read(fd, strings, strtab.sh_size) != strtab.sh_size)
+    if (read(fd, strings, strtab.sh_size) != strtab.sh_size) {
+        free(s);
+        free(syms);
+        free(strings);
         return;
+    }
     s->disas_num_syms = nsyms;
 #if ELF_CLASS == ELFCLASS32
     s->disas_symtab.elf32 = syms;
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call

[Blue Swirl]

Thanks, applied.

On Sun, Jan 16, 2011 at 3:28 PM, Stefan Weil <weil@mail.berlios.de> wrote:
> These errors were reported by cppcheck:
>
> [bsd-user/elfload.c:1108]: (error) Common realloc mistake: "syms" nulled but not freed upon failure
> [bsd-user/elfload.c:1076]: (error) Memory leak: s
> [bsd-user/elfload.c:1079]: (error) Memory leak: syms
>
> v2:
> * The previous fix for memory leaks was incomplete (thanks to Peter Maydell for te hint).
> * Fix wrong realloc usage, too.
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  bsd-user/elfload.c |   37 +++++++++++++++++++++++++++++++------
>  1 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 7374912..1ef1f97 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -1044,7 +1044,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>     struct elf_shdr sechdr, symtab, strtab;
>     char *strings;
>     struct syminfo *s;
> -    struct elf_sym *syms;
> +    struct elf_sym *syms, *new_syms;
>
>     lseek(fd, hdr->e_shoff, SEEK_SET);
>     for (i = 0; i < hdr->e_shnum; i++) {
> @@ -1072,15 +1072,24 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>     /* Now know where the strtab and symtab are.  Snarf them. */
>     s = malloc(sizeof(*s));
>     syms = malloc(symtab.sh_size);
> -    if (!syms)
> +    if (!syms) {
> +        free(s);
>         return;
> +    }
>     s->disas_strtab = strings = malloc(strtab.sh_size);
> -    if (!s->disas_strtab)
> +    if (!s->disas_strtab) {
> +        free(s);
> +        free(syms);
>         return;
> +    }
>
>     lseek(fd, symtab.sh_offset, SEEK_SET);
> -    if (read(fd, syms, symtab.sh_size) != symtab.sh_size)
> +    if (read(fd, syms, symtab.sh_size) != symtab.sh_size) {
> +        free(s);
> +        free(syms);
> +        free(strings);
>         return;
> +    }
>
>     nsyms = symtab.sh_size / sizeof(struct elf_sym);
>
> @@ -1105,13 +1114,29 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>  #endif
>         i++;
>     }
> -    syms = realloc(syms, nsyms * sizeof(*syms));
> +
> +     /* Attempt to free the storage associated with the local symbols
> +        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. */
> +    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);
>
>     lseek(fd, strtab.sh_offset, SEEK_SET);
> -    if (read(fd, strings, strtab.sh_size) != strtab.sh_size)
> +    if (read(fd, strings, strtab.sh_size) != strtab.sh_size) {
> +        free(s);
> +        free(syms);
> +        free(strings);
>         return;
> +    }
>     s->disas_num_syms = nsyms;
>  #if ELF_CLASS == ELFCLASS32
>     s->disas_symtab.elf32 = syms;
> --
> 1.7.2.3
>
>