Re: [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles

["Gao, Zhichao" <zhichao.gao@intel.com>]

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, May 4, 2023 6:08 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Pawel Polawski <ppolawsk@redhat.com>;
> Oliver Steffen <osteffen@redhat.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: Re: [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles
> 
> Ping.
> Any comments on this?
> 
> On Wed, Apr 26, 2023 at 01:39:27PM +0200, Gerd Hoffmann wrote:
> > In case ShellConvertStringToUint64() fails the Handles are left
> > uninitialized.  That can for example happen for Handle2 and Handle3 in
> > case only one parameter was specified on the command line.  Which can
> > trigger the ASSERT() in line 185.
> >
> > Reproducer: boot ovmf to efi shell in qemu, using q35 machine type,
> > then try disconnect the sata controller in efi shell.
> >
> > Fix that by explicitly setting them to NULL in that case.  While being
> > at it also simplify the logic and avoid pointlessly calling
> > ShellConvertStringToUint64() in case ParamN is NULL.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  .../UefiShellDriver1CommandsLib/Disconnect.c  | 19
> > +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > index fd49d1f7ceb4..fac6463e3c28 100644
> > --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > @@ -160,16 +160,23 @@ ShellCommandRunDisconnect (
> >          Param1 = ShellCommandLineGetRawValue (Package, 1);
> >          Param2 = ShellCommandLineGetRawValue (Package, 2);
> >          Param3 = ShellCommandLineGetRawValue (Package, 3);
> > -        if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1,
> TRUE, FALSE))) {
> > -          Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate1) : NULL;
> > +
> > +        if (Param1 && !EFI_ERROR (ShellConvertStringToUint64 (Param1,
> &Intermediate1, TRUE, FALSE))) {
> > +          Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate1);
> > +        } else {
> > +          Handle1 = NULL;
> >          }
> >
> > -        if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2,
> TRUE, FALSE))) {
> > -          Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate2) : NULL;
> > +        if (Param2 && !EFI_ERROR (ShellConvertStringToUint64 (Param2,
> &Intermediate2, TRUE, FALSE))) {
> > +          Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate2);
> > +        } else {
> > +          Handle2 = NULL;
> >          }
> >
> > -        if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3,
> TRUE, FALSE))) {
> > -          Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate3) : NULL;
> > +        if (Param3 && !EFI_ERROR (ShellConvertStringToUint64 (Param3,
> &Intermediate3, TRUE, FALSE))) {
> > +          Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate3);
> > +        } else {
> > +          Handle3 = NULL;
> >          }
> >
> >          if ((Param1 != NULL) && (Handle1 == NULL)) {
> > --
> > 2.40.0
> >
> 
> --