bsd-user: Fix possible memory leaks

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(-)

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?

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

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