[edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug

[Laszlo Ersek]

Jaben, Ray, Zhichao,

can one of you guys please review this patch?

Thanks
Laszlo

On 09/17/19 21:49, Laszlo Ersek wrote:
> The EDK 1 Shell (available at <https://github.com/tianocore/edk-Shell>)
> has a bug in its EFI_SHELL_ENVIRONMENT2.Execute() implementation that
> edk2's UefiShellLib has no choice but to work around.
> 
> Improve the explanation in the code. Also, document the implicit
> EFI_HANDLE -> (EFI_HANDLE*) conversion, which happens implicitly after
> dereferencing ParentHandle, with an explicit cast.
> 
> In practice, this patch is a no-op.
> 
> Cc: Jaben Carsey <jaben.carsey at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Zhichao Gao <zhichao.gao at intel.com>
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
> 
> Notes:
>     build-tested only
> 
>  ShellPkg/Library/UefiShellLib/UefiShellLib.c | 22 ++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> index 835d0f88ca74..9f07a58eb23d 100644
> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> @@ -1291,9 +1291,27 @@ ShellExecute (
>    if (mEfiShellEnvironment2 != NULL) {
>      //
>      // Call EFI Shell version.
> -    // Due to oddity in the EFI shell we want to dereference the ParentHandle here
>      //
> -    CmdStatus = (mEfiShellEnvironment2->Execute(*ParentHandle,
> +    // Due to an unfixable bug in the EdkShell implementation, we must
> +    // dereference "ParentHandle" here:
> +    //
> +    // 1. The EFI shell installs the EFI_SHELL_ENVIRONMENT2 protocol,
> +    //    identified by gEfiShellEnvironment2Guid.
> +    // 2. The Execute() member function takes "ParentImageHandle" as first
> +    //    parameter, with type (EFI_HANDLE*).
> +    // 3. In the EdkShell implementation, SEnvExecute() implements the
> +    //    Execute() member function. It passes "ParentImageHandle" correctly to
> +    //    SEnvDoExecute().
> +    // 4. SEnvDoExecute() takes the (EFI_HANDLE*), and passes it directly --
> +    //    without de-referencing -- to the HandleProtocol() boot service.
> +    // 5. But HandleProtocol() takes an EFI_HANDLE.
> +    //
> +    // Therefore we must
> +    // - de-reference "ParentHandle" here, to mask the bug in
> +    //   SEnvDoExecute(), and
> +    // - pass the resultant EFI_HANDLE as an (EFI_HANDLE*).
> +    //
> +    CmdStatus = (mEfiShellEnvironment2->Execute((EFI_HANDLE *)*ParentHandle,
>                                            CommandLine,
>                                            Output));
>      //
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48106): https://edk2.groups.io/g/devel/message/48106
Mute This Topic: https://groups.io/mt/34180236/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-