public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
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: [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize Handle before using it
Date: Wed, 8 Feb 2017 10:17:33 +0100	[thread overview]
Message-ID: <4c038ec5-b8a2-2f47-afd8-a58a349464de@redhat.com> (raw)
In-Reply-To: <CS1PR84MB0295BB195A9C8B137E055EDDA8420@CS1PR84MB0295.NAMPRD84.PROD.OUTLOOK.COM>

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
> 



  reply	other threads:[~2017-02-08  9:17 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-07  2:53 [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize Handle before using it Ruiyu Ni
2017-02-08  8:13 ` Wang, Sunny (HPS SW)
2017-02-08  9:17   ` Laszlo Ersek [this message]
2017-02-08  9:38     ` Wang, Sunny (HPS SW)
2017-02-08  9:41       ` Ni, Ruiyu
2017-02-08  9:18   ` Ni, Ruiyu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4c038ec5-b8a2-2f47-afd8-a58a349464de@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox