bsd-user: Fix possible memory leaks

Submitted by Stefan Weil on Jan. 16, 2011, 12:56 p.m.

Details

Message ID 1295182585-4323-1-git-send-email-weil@mail.berlios.de
State Superseded
Headers show

Commit Message

Stefan Weil Jan. 16, 2011, 12:56 p.m.
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(-)

Comments

Blue Swirl Jan. 16, 2011, 1:09 p.m.
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?
Peter Maydell Jan. 16, 2011, 2:07 p.m.
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
Stefan Weil Jan. 16, 2011, 3:27 p.m.
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

Patch hide | download patch | download mbox

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)