Re: [PATCH] MULTIBOOT: Fix handling of errors in broken aout-kludge

[Daniel Kiper]

> On Tue, Feb 20, 2024 at 01:14:42PM +0300, Vladimir 'phcoder' Serbinenko wrote:
> Current code in some codepaths neither discards nor reports
> errors. Properly surface the error

You are doing more than that, i.e., shuffling err variable definition.
Do we really need that? If yes please create separate patch or add
a blurb to the commit message listing that change.

> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
>  grub-core/loader/i386/multiboot_mbi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/loader/i386/multiboot_mbi.c 
> b/grub-core/loader/i386/multiboot_mbi.c
> index fae5d6fb8..ce1381f05 100644
> --- a/grub-core/loader/i386/multiboot_mbi.c
> +++ b/grub-core/loader/i386/multiboot_mbi.c
> @@ -69,7 +69,6 @@ static grub_err_t
>  load_kernel (grub_file_t file, const char *filename,
>            char *buffer, struct multiboot_header *header)
>  {
> -  grub_err_t err;
>    mbi_load_data_t mld;
>
>    mld.file = file;
> @@ -81,12 +80,15 @@ load_kernel (grub_file_t file, const char *filename,
>
>    if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
>      {
> +      grub_err_t err;

Please add empty line here...

>        err = grub_multiboot_load_elf (&mld);

... or...

grub_err_t err = grub_multiboot_load_elf (&mld);

...and than empty line after it...

>        if (err == GRUB_ERR_NONE) {
>       return GRUB_ERR_NONE;
>        }
>        if (err == GRUB_ERR_UNKNOWN_OS && (header->flags & 
> MULTIBOOT_AOUT_KLUDGE))
> -     grub_errno = GRUB_ERR_NONE;
> +     grub_errno = err = GRUB_ERR_NONE;
> +      if (err)

Even if that works I prefer to be fully correct...

if (err != GRUB_ERR_NONE)

> +     return err;
>      }
>    if (header->flags & MULTIBOOT_AOUT_KLUDGE)
>      {
> @@ -97,6 +99,7 @@ load_kernel (grub_file_t file, const char *filename,
>        grub_size_t code_size;
>        void *source;
>        grub_relocator_chunk_t ch;
> +      grub_err_t err;
>
>        if (header->bss_end_addr)
>       code_size = (header->bss_end_addr - header->load_addr);

Daniel