Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
Le 23/04/2020 à 22:20, Peter Maydell a écrit : > Calling g_mapped_file_unref() on a NULL pointer is not valid, and > glib will assert if you try it. > > $ qemu-system-arm -M virt -display none -device loader,file=/tmp/bad.elf > qemu-system-arm: -device loader,file=/tmp/bad.elf: GLib: g_mapped_file_unref: > assertion 'file != NULL' failed > > (One way to produce an ELF file that fails like this is to copy just > the first 16 bytes of a valid ELF file; this is sufficient to fool > the code in load_elf_ram_sym() into thinking it's an ELF file and > calling load_elf32() or load_elf64().) > > The failure-exit path in load_elf can be reached from various points > in execution, and for some of those we haven't yet called > g_mapped_file_new_from_fd(). Add a condition to the unref call so we > only call it if we successfully created the GMappedFile to start with. > > This will fix the assertion; for the specific case of the generic > loader it will then fall back from "guess this is an ELF file" to > "maybe it's a uImage or a hex file" and eventually to "just load as > a raw data file". > > Reported-by: Randy Yates > Signed-off-by: Peter Maydell > --- > include/hw/elf_ops.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index e0bb47bb678..398a4a2c85b 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -606,7 +606,9 @@ static int glue(load_elf, SZ)(const char *name, int fd, > *highaddr = (uint64_t)(elf_sword)high; > ret = total_size; > fail: > -g_mapped_file_unref(mapped_file); > +if (mapped_file) { > +g_mapped_file_unref(mapped_file); > +} > g_free(phdr); > return ret; > } > Applied to my trivial-patches branch. Thanks, Laurent
Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
On Thu, Apr 23, 2020 at 09:20:11PM +0100, Peter Maydell wrote: > Calling g_mapped_file_unref() on a NULL pointer is not valid, and > glib will assert if you try it. > > $ qemu-system-arm -M virt -display none -device loader,file=/tmp/bad.elf > qemu-system-arm: -device loader,file=/tmp/bad.elf: GLib: g_mapped_file_unref: > assertion 'file != NULL' failed > > (One way to produce an ELF file that fails like this is to copy just > the first 16 bytes of a valid ELF file; this is sufficient to fool > the code in load_elf_ram_sym() into thinking it's an ELF file and > calling load_elf32() or load_elf64().) > > The failure-exit path in load_elf can be reached from various points > in execution, and for some of those we haven't yet called > g_mapped_file_new_from_fd(). Add a condition to the unref call so we > only call it if we successfully created the GMappedFile to start with. > > This will fix the assertion; for the specific case of the generic > loader it will then fall back from "guess this is an ELF file" to > "maybe it's a uImage or a hex file" and eventually to "just load as > a raw data file". > > Reported-by: Randy Yates > Signed-off-by: Peter Maydell > --- > include/hw/elf_ops.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index e0bb47bb678..398a4a2c85b 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -606,7 +606,9 @@ static int glue(load_elf, SZ)(const char *name, int fd, > *highaddr = (uint64_t)(elf_sword)high; > ret = total_size; > fail: > -g_mapped_file_unref(mapped_file); > +if (mapped_file) { > +g_mapped_file_unref(mapped_file); > +} > g_free(phdr); > return ret; > } Oooops, my fault :-( Reviewed-by: Stefano Garzarella Maybe we can add: Fixes: 816b9fe450 ("elf-ops.h: Map into memory the ELF to load") Thanks, Stefano
Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
On Thu, 23 Apr 2020 at 21:25, Philippe Mathieu-Daudé wrote: > > On 4/23/20 10:20 PM, Peter Maydell wrote: > > This will fix the assertion; for the specific case of the generic > > loader it will then fall back from "guess this is an ELF file" to > > "maybe it's a uImage or a hex file" and eventually to "just load as > > a raw data file". > Reviewed-by: Philippe Mathieu-Daudé Thanks; as a side note "let me just load this as a raw data file" is not a very user-friendly way to report issues with the ELF file like "seems to be truncated" or "has no program headers". Not sure what to do about that... thanks -- PMM
Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
On 4/23/20 10:20 PM, Peter Maydell wrote: Calling g_mapped_file_unref() on a NULL pointer is not valid, and glib will assert if you try it. $ qemu-system-arm -M virt -display none -device loader,file=/tmp/bad.elf qemu-system-arm: -device loader,file=/tmp/bad.elf: GLib: g_mapped_file_unref: assertion 'file != NULL' failed (One way to produce an ELF file that fails like this is to copy just the first 16 bytes of a valid ELF file; this is sufficient to fool the code in load_elf_ram_sym() into thinking it's an ELF file and calling load_elf32() or load_elf64().) The failure-exit path in load_elf can be reached from various points in execution, and for some of those we haven't yet called g_mapped_file_new_from_fd(). Add a condition to the unref call so we only call it if we successfully created the GMappedFile to start with. This will fix the assertion; for the specific case of the generic loader it will then fall back from "guess this is an ELF file" to "maybe it's a uImage or a hex file" and eventually to "just load as a raw data file". Reported-by: Randy Yates Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé --- include/hw/elf_ops.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index e0bb47bb678..398a4a2c85b 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -606,7 +606,9 @@ static int glue(load_elf, SZ)(const char *name, int fd, *highaddr = (uint64_t)(elf_sword)high; ret = total_size; fail: -g_mapped_file_unref(mapped_file); +if (mapped_file) { +g_mapped_file_unref(mapped_file); +} g_free(phdr); return ret; }