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

[Stefan Weil <weil@mail.berlios.de>]

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