* [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks @ 2011-01-16 12:56 Stefan Weil 2011-01-16 13:09 ` [Qemu-devel] " Blue Swirl 2011-01-16 14:07 ` [Qemu-devel] " Peter Maydell 0 siblings, 2 replies; 6+ messages in thread From: Stefan Weil @ 2011-01-16 12:56 UTC (permalink / raw) To: QEMU Developers; +Cc: Blue Swirl 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) { + 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) -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] bsd-user: Fix possible memory leaks 2011-01-16 12:56 [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks Stefan Weil @ 2011-01-16 13:09 ` Blue Swirl 2011-01-16 14:07 ` [Qemu-devel] " Peter Maydell 1 sibling, 0 replies; 6+ messages in thread From: Blue Swirl @ 2011-01-16 13:09 UTC (permalink / raw) To: Stefan Weil; +Cc: QEMU Developers 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks 2011-01-16 12:56 [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks Stefan Weil 2011-01-16 13:09 ` [Qemu-devel] " Blue Swirl @ 2011-01-16 14:07 ` Peter Maydell 2011-01-16 15:27 ` Stefan Weil 1 sibling, 1 reply; 6+ messages in thread From: Peter Maydell @ 2011-01-16 14:07 UTC (permalink / raw) To: Stefan Weil; +Cc: Blue Swirl, QEMU Developers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks 2011-01-16 14:07 ` [Qemu-devel] " Peter Maydell @ 2011-01-16 15:27 ` Stefan Weil 2011-01-16 15:28 ` [Qemu-devel] [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call Stefan Weil 0 siblings, 1 reply; 6+ messages in thread From: Stefan Weil @ 2011-01-16 15:27 UTC (permalink / raw) To: Peter Maydell; +Cc: Blue Swirl, QEMU Developers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call 2011-01-16 15:27 ` Stefan Weil @ 2011-01-16 15:28 ` Stefan Weil 2011-01-17 20:24 ` [Qemu-devel] " Blue Swirl 0 siblings, 1 reply; 6+ messages in thread From: Stefan Weil @ 2011-01-16 15:28 UTC (permalink / raw) To: QEMU Developers; +Cc: Blue Swirl 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call 2011-01-16 15:28 ` [Qemu-devel] [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call Stefan Weil @ 2011-01-17 20:24 ` Blue Swirl 0 siblings, 0 replies; 6+ messages in thread From: Blue Swirl @ 2011-01-17 20:24 UTC (permalink / raw) To: Stefan Weil; +Cc: QEMU Developers 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-17 20:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-16 12:56 [Qemu-devel] [PATCH] bsd-user: Fix possible memory leaks Stefan Weil 2011-01-16 13:09 ` [Qemu-devel] " Blue Swirl 2011-01-16 14:07 ` [Qemu-devel] " Peter Maydell 2011-01-16 15:27 ` Stefan Weil 2011-01-16 15:28 ` [Qemu-devel] [PATCH v2] bsd-user: Fix possible memory leaks and wrong realloc call Stefan Weil 2011-01-17 20:24 ` [Qemu-devel] " Blue Swirl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).