| Message ID | 20200327094945.23768-2-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A selection of sanitiser fixes | expand |
On 3/27/20 10:49 AM, Alex Bennée wrote: > It's perfectly possible to have no function symbols in your elf file > and if we do the undefined behaviour sanitizer rightly complains about > us passing NULL to qsort. Check nsyms before we go ahead. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/elf_ops.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index a1411bfcab6..b5d4074d1e3 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -170,8 +170,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, > } > i++; > } > - syms = g_realloc(syms, nsyms * sizeof(*syms)); > > + /* check we have symbols left */ > + if (nsyms == 0) { > + goto fail; > + } > + > + syms = g_realloc(syms, nsyms * sizeof(*syms)); > qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ)); > for (i = 0; i < nsyms - 1; i++) { > if (syms[i].st_size == 0) { > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 3/27/20 11:53 AM, Philippe Mathieu-Daudé wrote: > On 3/27/20 10:49 AM, Alex Bennée wrote: >> It's perfectly possible to have no function symbols in your elf file >> and if we do the undefined behaviour sanitizer rightly complains about >> us passing NULL to qsort. Check nsyms before we go ahead. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> include/hw/elf_ops.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h >> index a1411bfcab6..b5d4074d1e3 100644 >> --- a/include/hw/elf_ops.h >> +++ b/include/hw/elf_ops.h >> @@ -170,8 +170,13 @@ static int glue(load_symbols, SZ)(struct elfhdr >> *ehdr, int fd, int must_swab, >> } >> i++; >> } >> - syms = g_realloc(syms, nsyms * sizeof(*syms)); Something was bugging me why looking at this line, now I remembered: another patch from 2 years ago :) https://www.mail-archive.com/qemu-devel@nongnu.org/msg528713.html Is this the same emitted warning? It seems. $ qemu-system-xtensa -M kc705 -m 128M -semihosting -nographic -monitor null -kernel Image.elf include/hw/elf_ops.h:179:5: runtime error: null pointer passed as argument 1, which is declared to never be null If so, can you add it to the commit description? Thanks! >> + /* check we have symbols left */ >> + if (nsyms == 0) { >> + goto fail; >> + } >> + >> + syms = g_realloc(syms, nsyms * sizeof(*syms)); >> qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ)); >> for (i = 0; i < nsyms - 1; i++) { >> if (syms[i].st_size == 0) { >> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Fri, 27 Mar 2020 at 09:50, Alex Bennée <alex.bennee@linaro.org> wrote: > > It's perfectly possible to have no function symbols in your elf file > and if we do the undefined behaviour sanitizer rightly complains about > us passing NULL to qsort. Check nsyms before we go ahead. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/elf_ops.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index a1411bfcab6..b5d4074d1e3 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -170,8 +170,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, > } > i++; > } > - syms = g_realloc(syms, nsyms * sizeof(*syms)); > > + /* check we have symbols left */ > + if (nsyms == 0) { > + goto fail; > + } > + > + syms = g_realloc(syms, nsyms * sizeof(*syms)); > qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ)); > for (i = 0; i < nsyms - 1; i++) { > if (syms[i].st_size == 0) { If "ELF file has no symbols" is valid, it's a bit odd for load_symbols to report it as a failure by returning -1. This only works because load_elf (the only caller) just ignores the return value entirely. OTOH I suppose you could argue that we can just ignore any oddity in the attempt to load symbols (eg bogus/malformad symtab section). If so, we should probably drop the return value from load_symbols(). thanks -- PMM
On 3/27/20 2:49 AM, Alex Bennée wrote: > It's perfectly possible to have no function symbols in your elf file > and if we do the undefined behaviour sanitizer rightly complains about > us passing NULL to qsort. Check nsyms before we go ahead. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/elf_ops.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index a1411bfcab6..b5d4074d1e3 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -170,8 +170,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, } i++; } - syms = g_realloc(syms, nsyms * sizeof(*syms)); + /* check we have symbols left */ + if (nsyms == 0) { + goto fail; + } + + syms = g_realloc(syms, nsyms * sizeof(*syms)); qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ)); for (i = 0; i < nsyms - 1; i++) { if (syms[i].st_size == 0) {
It's perfectly possible to have no function symbols in your elf file and if we do the undefined behaviour sanitizer rightly complains about us passing NULL to qsort. Check nsyms before we go ahead. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/hw/elf_ops.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) -- 2.20.1