You're right StrCpyS is better for this case. Since the patch has checked in, maybe we can refine those code in another patch. Thanks for the comment.
> -----Original Message----- > From: Carsey, Jaben > Sent: Wednesday, November 08, 2017 11:50 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Bi, Dandan <dandan...@intel.com> > Subject: RE: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool > > Why not use the StrCpy_s function to copy strings? CopyMem and StrSize feels > odd to me. > > > -----Original Message----- > > From: Wang, Jian J > > Sent: Tuesday, November 07, 2017 6:12 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 v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool > > Importance: High > > > > > v3: > > > No update > > > > > v2: > > > a. Use ReallocatePool instead of allocating then copying 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: 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 | 7 > > +++++-- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/ShellPkg/Application/Shell/Shell.c > > b/ShellPkg/Application/Shell/Shell.c > > index 5471930ba1..656206fdce 100644 > > --- a/ShellPkg/Application/Shell/Shell.c > > +++ b/ShellPkg/Application/Shell/Shell.c > > @@ -1646,7 +1646,7 @@ ShellConvertVariables ( > > // > > // now do the replacements... > > // > > - NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine); > > + NewCommandLine1 = AllocateZeroPool (NewSize); > > NewCommandLine2 = AllocateZeroPool(NewSize); > > ItemTemp = AllocateZeroPool(ItemSize+(2*sizeof(CHAR16))); > > if (NewCommandLine1 == NULL || NewCommandLine2 == NULL || > > ItemTemp == NULL) { > > @@ -1655,6 +1655,8 @@ ShellConvertVariables ( > > SHELL_FREE_NON_NULL(ItemTemp); > > return (NULL); > > } > > + CopyMem (NewCommandLine1, OriginalCommandLine, StrSize > > (OriginalCommandLine)); > > + > > for (MasterEnvList = EfiShellGetEnv(NULL) > > ; MasterEnvList != NULL && *MasterEnvList != CHAR_NULL > > ; MasterEnvList += StrLen(MasterEnvList) + 1 > > diff --git > > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > index 1122c89b8b..ee3db63358 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,13 @@ 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; > > + return ShellStatus; > > } > > + 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