From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 C69002035520F for ; Tue, 7 Nov 2017 18:42:27 -0800 (PST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP; 07 Nov 2017 18:46:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,362,1505804400"; d="scan'208";a="1241198481" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 07 Nov 2017 18:46:27 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 7 Nov 2017 18:46:27 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 7 Nov 2017 18:46:26 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by shsmsx102.ccr.corp.intel.com ([169.254.2.175]) with mapi id 14.03.0319.002; Wed, 8 Nov 2017 10:46:24 +0800 From: "Wang, Jian J" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Bi, Dandan" Thread-Topic: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool Thread-Index: AQHTWDcB/rckUu13/keH3rXx8KaBvKMJvo3QgAAHgSA= Date: Wed, 8 Nov 2017 02:46:23 +0000 Message-ID: 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: <0C09AFA07DD0434D9E2A0C6AEB0483103B9B2D26@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjYyNjJjYzQtZDdmYS00NzVkLTllZmItYmYyNTc3ZWYxYWEyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJVenJhZjh4b1wvZFhUSVNNOFlPQjI4WVZPY1kzd1wvcEZ4YnNja1wvK21vXC81Q2tRXC91c094M2lIbG0ydnp0SmJsWkUifQ== x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action 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:42:27 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; > Zeng, Star > Subject: RE: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool >=20 > In FrontPageCustomizedUiSupport.c, suggest to use "(CurrentSize + > UI_HII_DRIVER_LIST_SIZE)" instead of "(Count + UI_HII_DRIVER_LIST_SIZE)" = to > be consistent with the following code "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 *) 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 ; B= i, > 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" byte= s of > memory from old "Buffer" to new allocated one. If "AllocationSize" is big= ger > than size of "Buffer", heap memory overflow occurs during copy. >=20 > One solution is to allocate pool first then copy the necessary bytes to n= ew > memory. Another is using ReallocatePool instead if old buffer will be fre= ed > 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 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 > *)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 + > 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, ((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 > --- 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 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); > diff --git 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, *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