Jim, RellocatePool() will free old buffer but AllocateCopyPool() will not. So not all cases in which they can be replaced for each other.
Thanks, Jian > -----Original Message----- > From: jim.dai...@dell.com [mailto:jim.dai...@dell.com] > Sent: Friday, November 03, 2017 7:44 PM > To: Ni, Ruiyu <ruiyu...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > edk2-devel@lists.01.org > Cc: Carsey, Jaben <jaben.car...@intel.com>; Bi, Dandan <dandan...@intel.com> > Subject: RE: [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool > > Isn't ReallocatePool is the correct function to use in these cases? > For example: > > NewCommandLine1 = ReallocatePool(NewSize, StrSize(OriginalCommandLine), > OriginalCommandLine; > > Regards, > Jim > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni, > Ruiyu > Sent: Friday, November 3, 2017 3:23 AM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Carsey, Jaben <jaben.car...@intel.com>; Bi, Dandan <dandan...@intel.com> > Subject: Re: [edk2] [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool > > 2 comments below. > > -----Original Message----- > From: Wang, Jian J > Sent: Friday, November 3, 2017 12:58 PM > To: edk2-devel@lists.01.org > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; > Bi, > Dandan <dandan...@intel.com> > Subject: [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool > > 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. > > The solution is to allocate pool first then copy the necessary bytes to new > memory. This can avoid copying extra bytes from unknown memory range. > > Cc: Jaben Carsey <jaben.car...@intel.com> > Cc: Ruiyu Ni <ruiyu...@intel.com> > Cc: Bi Dandan <dandan...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > --- > ShellPkg/Application/Shell/Shell.c | 4 +++- > ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 6 > ++++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/ShellPkg/Application/Shell/Shell.c > b/ShellPkg/Application/Shell/Shell.c > index 5471930ba1..24a04ca323 100644 > --- a/ShellPkg/Application/Shell/Shell.c > +++ b/ShellPkg/Application/Shell/Shell.c > @@ -1646,7 +1646,9 @@ ShellConvertVariables ( > // > // now do the replacements... > // > - NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine); > + NewCommandLine1 = AllocatePool(NewSize); > + ASSERT (NewCommandLine1 != NULL); > [Ray] 1. Please do not use assertion because there is NULL check in the below > if- > statement. > The rule in ShellPkg is avoid using assertion. > > + CopyMem (NewCommandLine1, OriginalCommandLine, StrSize > (OriginalCommandLine)); > NewCommandLine2 = AllocateZeroPool(NewSize); > ItemTemp = AllocateZeroPool(ItemSize+(2*sizeof(CHAR16))); > if (NewCommandLine1 == NULL || NewCommandLine2 == NULL || ItemTemp > == NULL) { > diff --git > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > index 1122c89b8b..5de62219b3 100644 > --- a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > +++ b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > @@ -143,10 +143,11 @@ UpdateOptionalData( > OriginalOptionDataSize += (*(UINT16*)(OriginalData + sizeof(UINT32))); > OriginalOptionDataSize -= OriginalSize; > NewSize = OriginalSize - OriginalOptionDataSize + DataSize; > - NewData = AllocateCopyPool(NewSize, OriginalData); > + NewData = AllocatePool(NewSize); > if (NewData == NULL) { > Status = EFI_OUT_OF_RESOURCES; > } else { > + CopyMem (NewData, OriginalData, OriginalSize - OriginalOptionDataSize); > CopyMem(NewData + OriginalSize - OriginalOptionDataSize, Data, > DataSize); > } > } > @@ -1120,11 +1121,12 @@ BcfgAddOpt( > // Now we know how many EFI_INPUT_KEY structs we need to attach to > the end of the EFI_KEY_OPTION struct. > // Re-allocate with the added information. > // > - KeyOptionBuffer = AllocateCopyPool(sizeof(EFI_KEY_OPTION) + > (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount), > &NewKeyOption); > + KeyOptionBuffer = AllocatePool (sizeof(EFI_KEY_OPTION) + > (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount)); > if (KeyOptionBuffer == NULL) { > ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM), > gShellBcfgHiiHandle, L"bcfg"); > ShellStatus = SHELL_OUT_OF_RESOURCES; > } > [Ray] 2. Should the above NULL check return? > + CopyMem (KeyOptionBuffer, &NewKeyOption, sizeof(EFI_KEY_OPTION)); > } > for (LoopCounter = 0 ; ShellStatus == SHELL_SUCCESS && LoopCounter < > NewKeyOption.KeyData.Options.InputKeyCount; LoopCounter++) { > // > -- > 2.14.1.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
