Re: [PATCH] multiboot: fix memory leak

[Daniel Kiper]

On Sat, Oct 15, 2022 at 06:41:12PM +0800, t.feng via Grub-devel wrote:
> follow up the commit:
> eb33e61b31902a5493468895aaf83fa0b4f5f59d
>
> it seems that old commit can not fix memory leak completely.

The commit message should look more or less like that:

The commit eb33e61b3 (multiboot: fix memory leak) did not fix all
issues. Fix all of them right now.

Fixes: eb33e61b3 (multiboot: fix memory leak)

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

> ---
>  grub-core/loader/multiboot_elfxx.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index b76451f73..056992d80 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;

There is an additional leak in line 271. Please fix it too and double
check there are no other ones.

> @@ -286,14 +283,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;
>             grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
> -           return err;
> +           goto fail;
>           }
>         src = get_virtual_current_address (ch);
>         target = get_physical_target_address (ch);
>
>         if (grub_file_seek (mld->file, sh->sh_offset) == (grub_off_t) -1)
> -         return grub_errno;
> +         goto fail;
>
>            if (grub_file_read (mld->file, src, sh->sh_size)
>                != (grub_ssize_t) sh->sh_size)
> @@ -301,16 +299,19 @@ 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;
>           }
>         sh->sh_addr = target;
>       }
>        GRUB_MULTIBOOT (add_elfsyms) (shnum, ehdr->e_shentsize,
>                                   shstrndx, shdr);
> +      return GRUB_ERR_NONE;
>      }
>
>  #undef phdr
>
> +fail:

Missing space before "fail:" label.

> +  grub_free (shdr);
>    return grub_errno;
>  }

Daniel