From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 AD21F2035520D for ; Tue, 7 Nov 2017 18:53:47 -0800 (PST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Nov 2017 18:57:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,362,1505804400"; d="scan'208";a="173436198" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga006.fm.intel.com with ESMTP; 07 Nov 2017 18:57:47 -0800 Received: from fmsmsx118.amr.corp.intel.com (10.18.116.18) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 7 Nov 2017 18:57:47 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx118.amr.corp.intel.com (10.18.116.18) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 7 Nov 2017 18:57:46 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.218]) with mapi id 14.03.0319.002; Wed, 8 Nov 2017 10:57:44 +0800 From: "Zeng, Star" To: "Wang, Jian J" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Bi, Dandan" , "Zeng, Star" Thread-Topic: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool Thread-Index: AQHTWDcB/rckUu13/keH3rXx8KaBvKMJvo3QgAAHgSCAAAQTkA== Date: Wed, 8 Nov 2017 02:57:43 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9B2D88@shsmsx102.ccr.corp.intel.com> References: <20171108021201.17436-1-jian.j.wang@intel.com> <20171108021201.17436-2-jian.j.wang@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9B2D26@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Nov 2017 02:53:47 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Fine, https://bugzilla.tianocore.org/show_bug.cgi?id=3D762 is submitted. Reviewed-by: Star Zeng -----Original Message----- From: Wang, Jian J=20 Sent: Wednesday, November 8, 2017 10:46 AM To: Zeng, Star ; edk2-devel@lists.01.org Cc: Dong, Eric ; Bi, Dandan Subject: RE: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool Hi Star, I agree the issues you mentioned. But they're already there before this pat= ch. I'd suggest to file a new bug tracker for them instead of fixing them in th= is one. Thanks, Jian > -----Original Message----- > From: Zeng, Star > Sent: Wednesday, November 08, 2017 10:38 AM > To: Wang, Jian J ; edk2-devel@lists.01.org > Cc: Dong, Eric ; Bi, Dandan=20 > ; Zeng, Star > Subject: RE: [PATCH v3 1/3] MdeModulePkg: Fix misuses of=20 > AllocateCopyPool >=20 > In FrontPageCustomizedUiSupport.c, suggest to use "(CurrentSize +=20 > UI_HII_DRIVER_LIST_SIZE)" instead of "(Count +=20 > UI_HII_DRIVER_LIST_SIZE)" to be consistent with the following code=20 > "CurrentSize +=3D UI_HII_DRIVER_LIST_SIZE". >=20 > Same comment to BootMaintenanceManagerCustomizedUiSupport.c >=20 > In HiiLib.c, suggest removing "FormSetBuffer =3D NULL". > And the code logic below in HiiLib.c is strange and suggest refining the = code. > TempBuffer =3D AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *)=20 > OpCodeData)- > >Length); > ... > CopyMem (TempBuffer, OpCodeData, ((EFI_IFR_OP_HEADER *) OpCodeData)- > >Length); >=20 >=20 > Thanks, > Star > -----Original Message----- > From: Wang, Jian J > Sent: Wednesday, November 8, 2017 10:12 AM > To: edk2-devel@lists.01.org > Cc: Zeng, Star ; Dong, Eric=20 > ; Bi, Dandan > Subject: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool >=20 > >v3: > > a. Add ASSERT for returned pointer > > b. Correct DestMax parameter in calling StrCpyS c. Fix coding style >=20 > >v2: > > a. Use ReallocatePool to replace AllocateCopyPool wherever applicable. >=20 > AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize"=20 > bytes of memory from old "Buffer" to new allocated one. If=20 > "AllocationSize" is bigger than size of "Buffer", heap memory overflow oc= curs during copy. >=20 > One solution is to allocate pool first then copy the necessary bytes=20 > to new memory. Another is using ReallocatePool instead if old buffer=20 > will be freed on spot. >=20 > Cc: Star Zeng > Cc: Eric Dong > Cc: Bi Dandan > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > .../Application/UiApp/FrontPageCustomizedUiSupport.c | 8 ++++++= -- > .../BootMaintenanceManagerCustomizedUiSupport.c | 8 ++++++= -- > MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 10 +++++-= - > --- > MdeModulePkg/Library/UefiHiiLib/HiiLib.c | 12 ++++++= ++---- > .../Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c | 3 ++- > MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 9 > ++++++--- > 6 files changed, 33 insertions(+), 17 deletions(-) >=20 > diff --git=20 > a/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c > b/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c > index 1505ef9319..17fc3db507 100644 > --- a/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c > +++ b/MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c > @@ -639,9 +639,13 @@ UiListThirdPartyDrivers ( >=20 > Count++; > if (Count >=3D CurrentSize) { > - DriverListPtr =3D AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SI= ZE) * > sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList); > + DriverListPtr =3D ReallocatePool ( > + CurrentSize * sizeof (UI_HII_DRIVER_INSTANCE), > + (Count + UI_HII_DRIVER_LIST_SIZE) > + * sizeof (UI_HII_DRIVER_INSTANCE), > + gHiiDriverList > + ); > ASSERT (DriverListPtr !=3D NULL); > - FreePool (gHiiDriverList); > gHiiDriverList =3D DriverListPtr; > CurrentSize +=3D UI_HII_DRIVER_LIST_SIZE; > } > diff --git > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM > anagerCustomizedUiSupport.c > b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM > anagerCustomizedUiSupport.c > index b25bc67c06..6dd4fce139 100644 > --- > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM > anagerCustomizedUiSupport.c > +++ > b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceM > anagerCustomizedUiSupport.c > @@ -435,9 +435,13 @@ BmmListThirdPartyDrivers ( >=20 > Count++; > if (Count >=3D CurrentSize) { > - DriverListPtr =3D AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SI= ZE) * > sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList); > + DriverListPtr =3D ReallocatePool ( > + CurrentSize * sizeof (UI_HII_DRIVER_INSTANCE), > + (Count + UI_HII_DRIVER_LIST_SIZE) > + * sizeof (UI_HII_DRIVER_INSTANCE), > + gHiiDriverList > + ); > ASSERT (DriverListPtr !=3D NULL); > - FreePool (gHiiDriverList); > gHiiDriverList =3D DriverListPtr; > CurrentSize +=3D UI_HII_DRIVER_LIST_SIZE; > } > diff --git a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c > index 23ae6c5392..ac8a975bf6 100644 > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c > @@ -240,7 +240,11 @@ AddIdToMacDeviceList ( > } else { > mMacDeviceList.MaxListLen +=3D MAX_MAC_ADDRESS_NODE_LIST_LEN; > if (mMacDeviceList.CurListLen !=3D 0) { > - TempDeviceList =3D (MENU_INFO_ITEM *)AllocateCopyPool (sizeof > (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen, (VOID=20 > *)mMacDeviceList.NodeList); > + TempDeviceList =3D ReallocatePool ( > + sizeof (MENU_INFO_ITEM) * mMacDeviceList.CurLis= tLen, > + sizeof (MENU_INFO_ITEM) * mMacDeviceList.MaxLis= tLen, > + mMacDeviceList.NodeList > + ); > } else { > TempDeviceList =3D (MENU_INFO_ITEM *)AllocatePool (sizeof > (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen); > } > @@ -251,10 +255,6 @@ AddIdToMacDeviceList ( > TempDeviceList[mMacDeviceList.CurListLen].PromptId =3D PromptId; > TempDeviceList[mMacDeviceList.CurListLen].QuestionId =3D > (EFI_QUESTION_ID) (mMacDeviceList.CurListLen +=20 > NETWORK_DEVICE_LIST_KEY_OFFSET); >=20 > - if (mMacDeviceList.CurListLen > 0) { > - FreePool(mMacDeviceList.NodeList); > - } > - > mMacDeviceList.NodeList =3D TempDeviceList; > } > mMacDeviceList.CurListLen ++; > diff --git a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c > b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c > index ce894c08b5..f9b8c3df27 100644 > --- a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c > +++ b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c > @@ -464,20 +464,24 @@ HiiGetFormSetFromHiiHandle( > } >=20 > if (FormSetBuffer !=3D NULL){ > - TempBuffer =3D AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER = *) > OpCodeData)->Length, FormSetBuffer); > - FreePool(FormSetBuffer); > - FormSetBuffer =3D NULL; > + TempBuffer =3D ReallocatePool ( > + TempSize, > + TempSize + ((EFI_IFR_OP_HEADER *) OpCodeData)->Le= ngth, > + FormSetBuffer > + ); > if (TempBuffer =3D=3D NULL) { > Status =3D EFI_OUT_OF_RESOURCES; > goto Done; > } > CopyMem (TempBuffer + TempSize, OpCodeData,=20 > ((EFI_IFR_OP_HEADER *) OpCodeData)->Length); > + FormSetBuffer =3D NULL; > } else { > - TempBuffer =3D AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER = *) > OpCodeData)->Length, OpCodeData); > + TempBuffer =3D AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *) > OpCodeData)->Length); > if (TempBuffer =3D=3D NULL) { > Status =3D EFI_OUT_OF_RESOURCES; > goto Done; > } > + CopyMem (TempBuffer, OpCodeData, ((EFI_IFR_OP_HEADER *) > OpCodeData)->Length); > } > TempSize +=3D ((EFI_IFR_OP_HEADER *) OpCodeData)->Length; > FormSetBuffer =3D TempBuffer; > diff --git > a/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c > b/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c > index b81110ff98..e39036aed9 100644 > ---=20 > a/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem.c > +++ b/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystem. > +++ c > @@ -562,7 +562,8 @@ FvSimpleFileSystemOpen ( > // No, there was no extension. So add one and search again for the= file > // NewFileNameLength =3D FileNameLength + 1 + 4 =3D (Number of=20 > non-null > character) + (file extension) + (a null character) > NewFileNameLength =3D FileNameLength + 1 + 4; > - FileNameWithExtension =3D AllocateCopyPool (NewFileNameLength * 2, > FileName); > + FileNameWithExtension =3D AllocatePool (NewFileNameLength * 2); > + StrCpyS (FileNameWithExtension, NewFileNameLength, FileName); > StrCatS (FileNameWithExtension, NewFileNameLength, L".EFI"); >=20 > for (FvFileInfoLink =3D GetFirstNode (&Instance->FileInfoHead);=20 > diff --git=20 > a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c > b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c > index 1b48c1cebe..5d5f17fb17 100644 > --- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c > +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c > @@ -2543,12 +2543,15 @@ MergeToMultiKeywordResp ( >=20 > MultiKeywordRespLen =3D (StrLen (*MultiKeywordResp) + 1 + StrLen > (*KeywordResp) + 1) * sizeof (CHAR16); >=20 > - StringPtr =3D AllocateCopyPool (MultiKeywordRespLen,=20 > *MultiKeywordResp); > + StringPtr =3D ReallocatePool ( > + StrSize (*MultiKeywordResp), > + MultiKeywordRespLen, > + *MultiKeywordResp > + ); > if (StringPtr =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } > - > - FreePool (*MultiKeywordResp); > + > *MultiKeywordResp =3D StringPtr; >=20 > StrCatS (StringPtr, MultiKeywordRespLen / sizeof (CHAR16), L"&"); > -- > 2.14.1.windows.1