Re: [PATCH] multiboot: fix memory leak

[Daniel Kiper]

On Fri, Oct 28, 2022 at 08:52:01PM +0800, t.feng via Grub-devel wrote:
> The commit eb33e61b3 (multiboot: fix memory leak) did not fix all
> issues. Fix all of them right now.
>
> Fixes: eb33e61b3 (multiboot: fix memory leak)

Much better. Thank you!

However, please add

Signed-off-by: "t.feng" <fengtao40@huawei.com>

... or something like that here.

> ---
>  grub-core/loader/multiboot_elfxx.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index b76451f73..8cd96537f 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -246,10 +246,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>       return grub_errno;
>
>        if (grub_file_seek (mld->file, ehdr->e_shoff) == (grub_off_t) -1)
> -     {
> -       grub_free (shdr);
> -       return grub_errno;
> -     }
> +     goto fail;
>
>        if (grub_file_read (mld->file, shdr, shnum * ehdr->e_shentsize)
>                != (grub_ssize_t) shnum * ehdr->e_shentsize)
> @@ -257,7 +254,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>         if (!grub_errno)
>           grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file 
> %s"),
>                       mld->filename);
> -       return grub_errno;
> +       goto fail;
>       }
>
>        for (shdrptr = shdr, i = 0; i < shnum;
> @@ -268,7 +265,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>         grub_addr_t target;
>
>         if (mld->mbi_ver >= 2 && (sh->sh_type == SHT_REL || sh->sh_type == 
> SHT_RELA))
> -         return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "ELF files with 
> relocs are not supported yet");
> +         {
> +           grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "ELF files with relocs 
> are not supported yet");
> +           goto fail;
> +         }
>
>         /* This section is a loaded section,
>            so we don't care.  */
> @@ -286,14 +286,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>                                                 mld->avoid_efi_boot_services);
>         if (err != GRUB_ERR_NONE)
>           {
> +           grub_errno = err;

This assignment...

>             grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);

... should be moved here. Otherwise grub_dprintf() may wipe error code
stored in grub_errno.

> -           return err;
> +           goto fail;
>           }

Otherwise patch LGTM.

Daniel