Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)

[Stefano Garzarella]

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 <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  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 <address@hidden>

Maybe we can add:
Fixes: 816b9fe450 ("elf-ops.h: Map into memory the ELF to load")

Thanks,
Stefano