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

["Carsey, Jaben" <jaben.carsey@intel.com>]

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

Given the time gap between now and when this library was originally written, I think that we should revisit maintaining support for EFI Shell. Do we still need to have UEFI Applications that can go between EFI and UEFI versions of the shell? Are there really many EFI Shell instances that currently developed applications need to maintain support for?

Thanks
-Jaben

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, September 26, 2019 5:47 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify
> workaround for unfixable EdkShell bug
> 
> 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@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Signed-off-by: Laszlo Ersek <lersek@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));
> >      //
> >