Re: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool

["Zeng, Star" <star.zeng@intel.com>]

Fine, https://bugzilla.tianocore.org/show_bug.cgi?id=762 is submitted.

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Wang, Jian J 
Sent: Wednesday, November 8, 2017 10:46 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Bi, Dandan <dandan.bi@intel.com>
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 patch.
I'd suggest to file a new bug tracker for them instead of fixing them in this one.

Thanks,
Jian

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, November 08, 2017 10:38 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Bi, Dandan 
> <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 1/3] MdeModulePkg: Fix misuses of 
> AllocateCopyPool
> 
> 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 += UI_HII_DRIVER_LIST_SIZE".
> 
> Same comment to BootMaintenanceManagerCustomizedUiSupport.c
> 
> In HiiLib.c, suggest removing "FormSetBuffer = NULL".
> And the code logic below in HiiLib.c is strange and suggest refining the code.
> TempBuffer = AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *) 
> OpCodeData)-
> >Length);
> ...
> CopyMem (TempBuffer, OpCodeData, ((EFI_IFR_OP_HEADER *) OpCodeData)-
> >Length);
> 
> 
> 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 <star.zeng@intel.com>; Dong, Eric 
> <eric.dong@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: [PATCH v3 1/3] MdeModulePkg: Fix misuses of AllocateCopyPool
> 
> >v3:
> > a. Add ASSERT for returned pointer
> > b. Correct DestMax parameter in calling StrCpyS  c. Fix coding style
> 
> >v2:
> > a. Use ReallocatePool to replace AllocateCopyPool wherever applicable.
> 
> AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" 
> bytes of memory from old "Buffer" to new allocated one. If 
> "AllocationSize" is bigger than size of "Buffer", heap memory overflow occurs during copy.
> 
> One solution is to allocate pool first then copy the necessary bytes 
> to new memory. Another is using ReallocatePool instead if old buffer 
> will be freed on spot.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Bi Dandan <dandan.bi@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  .../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(-)
> 
> 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 (
> 
>      Count++;
>      if (Count >= CurrentSize) {
> -      DriverListPtr = AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SIZE) *
> sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList);
> +      DriverListPtr = ReallocatePool (
> +                        CurrentSize * sizeof (UI_HII_DRIVER_INSTANCE),
> +                        (Count + UI_HII_DRIVER_LIST_SIZE)
> +                          * sizeof (UI_HII_DRIVER_INSTANCE),
> +                        gHiiDriverList
> +                        );
>        ASSERT (DriverListPtr != NULL);
> -      FreePool (gHiiDriverList);
>        gHiiDriverList = DriverListPtr;
>        CurrentSize += 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 (
> 
>      Count++;
>      if (Count >= CurrentSize) {
> -      DriverListPtr = AllocateCopyPool ((Count + UI_HII_DRIVER_LIST_SIZE) *
> sizeof (UI_HII_DRIVER_INSTANCE), gHiiDriverList);
> +      DriverListPtr = ReallocatePool (
> +                        CurrentSize * sizeof (UI_HII_DRIVER_INSTANCE),
> +                        (Count + UI_HII_DRIVER_LIST_SIZE)
> +                        * sizeof (UI_HII_DRIVER_INSTANCE),
> +                        gHiiDriverList
> +                        );
>        ASSERT (DriverListPtr != NULL);
> -      FreePool (gHiiDriverList);
>        gHiiDriverList = DriverListPtr;
>        CurrentSize += 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 += MAX_MAC_ADDRESS_NODE_LIST_LEN;
>      if (mMacDeviceList.CurListLen != 0) {
> -      TempDeviceList = (MENU_INFO_ITEM *)AllocateCopyPool (sizeof
> (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen, (VOID 
> *)mMacDeviceList.NodeList);
> +      TempDeviceList = ReallocatePool (
> +                         sizeof (MENU_INFO_ITEM) * mMacDeviceList.CurListLen,
> +                         sizeof (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen,
> +                         mMacDeviceList.NodeList
> +                         );
>      } else {
>        TempDeviceList = (MENU_INFO_ITEM *)AllocatePool (sizeof
> (MENU_INFO_ITEM) * mMacDeviceList.MaxListLen);
>      }
> @@ -251,10 +255,6 @@ AddIdToMacDeviceList (
>      TempDeviceList[mMacDeviceList.CurListLen].PromptId = PromptId;
>      TempDeviceList[mMacDeviceList.CurListLen].QuestionId =
> (EFI_QUESTION_ID) (mMacDeviceList.CurListLen + 
> NETWORK_DEVICE_LIST_KEY_OFFSET);
> 
> -    if (mMacDeviceList.CurListLen > 0) {
> -      FreePool(mMacDeviceList.NodeList);
> -    }
> -
>      mMacDeviceList.NodeList = 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(
>        }
> 
>        if (FormSetBuffer != NULL){
> -        TempBuffer = AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length, FormSetBuffer);
> -        FreePool(FormSetBuffer);
> -        FormSetBuffer = NULL;
> +        TempBuffer = ReallocatePool (
> +                       TempSize,
> +                       TempSize + ((EFI_IFR_OP_HEADER *) OpCodeData)->Length,
> +                       FormSetBuffer
> +                       );
>          if (TempBuffer == NULL) {
>            Status = EFI_OUT_OF_RESOURCES;
>            goto Done;
>          }
>          CopyMem (TempBuffer + TempSize,  OpCodeData, 
> ((EFI_IFR_OP_HEADER *) OpCodeData)->Length);
> +        FormSetBuffer = NULL;
>        } else {
> -        TempBuffer = AllocateCopyPool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length, OpCodeData);
> +        TempBuffer = AllocatePool (TempSize + ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length);
>          if (TempBuffer == NULL) {
>            Status = EFI_OUT_OF_RESOURCES;
>            goto Done;
>          }
> +        CopyMem (TempBuffer, OpCodeData, ((EFI_IFR_OP_HEADER *)
> OpCodeData)->Length);
>        }
>        TempSize += ((EFI_IFR_OP_HEADER *) OpCodeData)->Length;
>        FormSetBuffer = 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 = FileNameLength + 1 + 4 = (Number of 
> non-null
> character) + (file extension) + (a null character)
>        NewFileNameLength = FileNameLength + 1 + 4;
> -      FileNameWithExtension = AllocateCopyPool (NewFileNameLength * 2,
> FileName);
> +      FileNameWithExtension = AllocatePool (NewFileNameLength * 2);
> +      StrCpyS (FileNameWithExtension, NewFileNameLength, FileName);
>        StrCatS (FileNameWithExtension, NewFileNameLength, L".EFI");
> 
>        for (FvFileInfoLink = 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 (
> 
>    MultiKeywordRespLen = (StrLen (*MultiKeywordResp) + 1 + StrLen
> (*KeywordResp) + 1) * sizeof (CHAR16);
> 
> -  StringPtr = AllocateCopyPool (MultiKeywordRespLen, 
> *MultiKeywordResp);
> +  StringPtr = ReallocatePool (
> +                StrSize (*MultiKeywordResp),
> +                MultiKeywordRespLen,
> +                *MultiKeywordResp
> +                );
>    if (StringPtr == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> -
> -  FreePool (*MultiKeywordResp);
> +
>    *MultiKeywordResp = StringPtr;
> 
>    StrCatS (StringPtr, MultiKeywordRespLen / sizeof (CHAR16), L"&");
> --
> 2.14.1.windows.1