Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize Handle before using it

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

I will change the commit message as Laszlo suggested.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Wang, Sunny (HPS SW)
> Sent: Wednesday, February 8, 2017 5:38 PM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
> Cc: Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize
> Handle before using it
> 
> Yeah, I misread the commit message and had a misunderstanding on what
> situation BmExpandMediaDevicePath() is called .
> Ray and Lazlo, thanks for both of you clarifying this. :)
> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 08, 2017 5:18 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; Ruiyu Ni
> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize
> Handle before using it
> Importance: High
> 
> On 02/08/17 09:13, Wang, Sunny (HPS SW) wrote:
> > Looks good to me.
> > Reviewed-by: Sunny Wang <sunnywang@hpe.com>
> >
> > However, I saw the other potential issue below. If you also think the
> > potential issue is valid, you can fix this in the other patch.
> > We need to consider the case where MDEPKG_NDEBUG is defined. We're
> > using ASSERT_EFI_ERROR to handle the error status from
> > LocateDevicePath() in the following code block, which contains
> > potential bug that we still use uninitialized Handle for the case
> > where LocateDevicePath returns error status.
> >
> > Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid,
> > &TempDevicePath, &Handle); ASSERT_EFI_ERROR (Status);
> 
> I think that Ray made a sufficient argument in the commit message why the
> assertion was correct -- it is indeed an assertion, and not error checking.
> 
>     Since the function is called when the Handle supports BlockIo or
>     SimpleFileSystem, when there is no SimpleFileSystem installed on
>     the Handle, BlockIo should be installed on the Handle.
> 
> Perhaps the wording could be clarified, as in:
> 
>     ... BlockIo is *guaranteed* to be installed on the Handle.
> 
> Just my two cents anyway.
> Laszlo
> 
> >
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Ruiyu Ni
> > Sent: Tuesday, February 07, 2017 10:54 AM
> > To: edk2-devel@lists.01.org
> > Cc: Jeff Fan <jeff.fan@intel.com>
> > Subject: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize
> > Handle before using it
> >
> > BmExpandMediaDevicePath contains a bug that it uses the uninitialized
> Handle.
> >
> > Since the function is called when the Handle supports BlockIo or
> SimpleFileSystem, when there is no SimpleFileSystem installed on the Handle,
> BlockIo should be installed on the Handle.
> > The fix initializes the Handle by locating the BlockIo protocol from the
> device path.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jeff Fan <jeff.fan@intel.com>
> > ---
> >  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > index 75bd5dc..9f99122 100644
> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > @@ -991,9 +991,13 @@ BmExpandMediaDevicePath (
> >      return FileBuffer;
> >    }
> >
> > +  Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid,
> > + &TempDevicePath, &Handle);  ASSERT_EFI_ERROR (Status);
> > +
> >    //
> >    // For device boot option only pointing to the removable device
> > handle,
> > -  // should make sure all its children handles (its child partion or media
> handles) are created and connected.
> > +  // should make sure all its children handles (its child partion or
> > + media handles)  // are created and connected.
> >    //
> >    gBS->ConnectController (Handle, NULL, NULL, TRUE);
> >
> > @@ -1004,8 +1008,6 @@ BmExpandMediaDevicePath (
> >    // returned. After the Block IO protocol is reinstalled, subsequent
> >    // Block IO read/write will success.
> >    //
> > -  Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid,
> > &TempDevicePath, &Handle);
> > -  ASSERT_EFI_ERROR (Status);
> >    Status = gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (VOID
> **) &BlockIo);
> >    ASSERT_EFI_ERROR (Status);
> >    Buffer = AllocatePool (BlockIo->Media->BlockSize);
> > --
> > 2.9.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel