Patchwork [v2] bsd-user: Fix possible memory leaks and wrong realloc call

login
register
mail settings
Submitter Stefan Weil
Date Jan. 16, 2011, 3:28 p.m.
Message ID <1295191700-5312-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/79089/
State Accepted
Headers show

Comments

Stefan Weil - Jan. 16, 2011, 3:28 p.m.
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(-)
Blue Swirl - Jan. 17, 2011, 8:24 p.m.
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
>
>

Patch

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;