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 <address@hidden> 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