From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CDF46820A1 for ; Wed, 8 Feb 2017 01:41:20 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga105.jf.intel.com with ESMTP; 08 Feb 2017 01:41:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,346,1477983600"; d="scan'208";a="62555157" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga005.fm.intel.com with ESMTP; 08 Feb 2017 01:41:20 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 8 Feb 2017 01:41:20 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.59]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.204]) with mapi id 14.03.0248.002; Wed, 8 Feb 2017 17:41:17 +0800 From: "Ni, Ruiyu" To: "Wang, Sunny (HPS SW)" , Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Fan, Jeff" Thread-Topic: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize Handle before using it Thread-Index: AQHSgO2C8o8dPuOhiUyUfSmTzpKySKFePlsAgAASCYCAAAXRAIAAhtEg Date: Wed, 8 Feb 2017 09:41:17 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5B89A6F1@SHSMSX104.ccr.corp.intel.com> References: <20170207025345.599532-1-ruiyu.ni@intel.com> <4c038ec5-b8a2-2f47-afd8-a58a349464de@redhat.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize Handle before using it X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Feb 2017 09:41:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; Ni, Ruiyu ; > edk2-devel@lists.01.org > Cc: Fan, Jeff > Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize > Handle before using it >=20 > 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. :) >=20 > Regards, > Sunny Wang >=20 > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, February 08, 2017 5:18 PM > To: Wang, Sunny (HPS SW) ; Ruiyu Ni > ; edk2-devel@lists.01.org > Cc: Jeff Fan > Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: Initialize > Handle before using it > Importance: High >=20 > On 02/08/17 09:13, Wang, Sunny (HPS SW) wrote: > > Looks good to me. > > Reviewed-by: Sunny Wang > > > > 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 =3D gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid, > > &TempDevicePath, &Handle); ASSERT_EFI_ERROR (Status); >=20 > 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 checkin= g. >=20 > 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. >=20 > Perhaps the wording could be clarified, as in: >=20 > ... BlockIo is *guaranteed* to be installed on the Handle. >=20 > Just my two cents anyway. > Laszlo >=20 > > > > -----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 > > 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 Hand= le, > BlockIo should be installed on the Handle. > > The fix initializes the Handle by locating the BlockIo protocol from th= e > device path. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Ruiyu Ni > > Cc: Jeff Fan > > --- > > 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 =3D 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 m= edia > 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 =3D gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid, > > &TempDevicePath, &Handle); > > - ASSERT_EFI_ERROR (Status); > > Status =3D gBS->HandleProtocol (Handle, &gEfiBlockIoProtocolGuid, (V= OID > **) &BlockIo); > > ASSERT_EFI_ERROR (Status); > > Buffer =3D 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 > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel