Re: [PATCH] efi: remove unreachable code in read_file()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.08.2025 21:17, Dmytro Prokopchuk1 wrote:
> MISRA C Rule 2.1 states: "A project shall not contain unreachable code."
> 
> Function `PrintErrMesg(const CHAR16*, EFI_STATUS)` isn't intended to return
> control to its caller. At the end, it calls `blexit()`, which, in turn,
> invokes the `__builtin_unreachable()` function, making subsequent return
> statements in `read_file()` unreachable:
> 
>     PrintErrMesg(name, ret);
>     /* not reached */
>     return false;
> 
> This also causes unreachability of the code meant to handle `read_file()`
> errors, as seen in these examples:
> 
> In this block:
>     if ( read_file(dir_handle, file_name, &cfg, NULL) )
>         break;
>     *tail = 0;
>     }
>     if ( !tail )
>         blexit(L"No configuration file found.");
> 
> And here:
>     else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
>         blexit(L"Configuration file not found.");
> 
> And here:
>     if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
>     {
>         PrintStr(L"Chained configuration file '");
>         PrintStr(name.w);
>         efi_bs->FreePool(name.w);
>         blexit(L"'not found.");
>     }
> 
> The issue arises because when an error occurs inside `read_file()`, it calls
> `PrintErrMesg()` and does not return to the caller.
> 
> To address this the following changes are applied:
> 1. Remove `PrintErrMesg(name, ret);` from the `read_file()` function.
> 2. Replaced it with `PrintErr(name);`, which prints the file name and returns
>    control to the caller.
> 3. Change the `read_file()` return type from `bool` to `EFI_STATUS`, allowing
>    file operation result codes to be returned to the caller.
> 4. Properly handle error codes returned from the `read_file()` function in the
>    relevant areas of the code.
> 5. Replace `blexit()` calls with informative error codes using 
> `PrintErrMesg()`
>    where appropriate.
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
> ---
> Test CI pipeline:
> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1980590118
> ---
>  xen/common/efi/boot.c | 57 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 50ff1d1bd2..ddbafb2f9c 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -132,7 +132,7 @@ struct file {
>      };
>  };
>  
> -static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> +static EFI_STATUS read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>                        struct file *file, const char *options);
>  static bool read_section(const EFI_LOADED_IMAGE *image, const CHAR16 *name,
>                           struct file *file, const char *options);
> @@ -782,7 +782,7 @@ static void __init handle_file_info(const CHAR16 *name,
>      efi_arch_handle_module(file, name, options);
>  }
>  
> -static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> +static EFI_STATUS __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>                               struct file *file, const char *options)
>  {
>      EFI_FILE_HANDLE FileHandle = NULL;
> @@ -791,7 +791,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>      const CHAR16 *what = NULL;
>  
>      if ( !name )
> -        PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
> +        return EFI_INVALID_PARAMETER;

Why the change in error code? EFI_OUT_OF_RESOURCES() was used deliberately for
cases where the result of s2w() is passed directly into here.

Between this hunk and ...

> @@ -842,7 +842,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>  
>      efi_arch_flush_dcache_area(file->ptr, file->size);
>  
> -    return true;
> +    return ret;
>  
>   fail:
>      if ( FileHandle )

... this one there's at least one "return false" which you leave untouched, thus
wrongly reporting EFI_SUCCESS now to the caller.

> @@ -850,10 +850,9 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>  
>      PrintErr(what);
>      PrintErr(L" failed for ");
> -    PrintErrMesg(name, ret);
> +    PrintErr(name);
>  
> -    /* not reached */
> -    return false;
> +    return ret;
>  }

With the comment here - possibly adjusted to become a SAF one - all should be
fine with no other changes? Because of the other "return false" callers simply
can't assume the function would never indicate failure back to them. (New
"return false" could in principle also appear, which is why I think the base
structure wants keeping as is, including in the callers.)

Jan