Reviewed-by : Tapan Shah <<tapands...@hp.com>> Make sure you merge ifconfig changes with EDKII trunk before check-in as the code is modified substantially compare to the patch file provided.
-----Original Message----- From: Qiu, Shumin [mailto:shumin....@intel.com] Sent: Tuesday, July 07, 2015 7:58 PM To: Shah, Tapan Cc: Ni, Ruiyu; Heyi Guo Subject: RE: [edk2][PATCH] ShellPkg: Refine code to use Strn**S safe functions instead of Str**S ones in some cases. Patch attached. -Shumin -----Original Message----- From: Shah, Tapan [mailto:tapands...@hp.com] Sent: Wednesday, July 08, 2015 12:41 AM To: Qiu, Shumin; edk2-devel@lists.sourceforge.net Cc: Ni, Ruiyu; Heyi Guo Subject: RE: [edk2][PATCH] ShellPkg: Refine code to use Strn**S safe functions instead of Str**S ones in some cases. Shumin, Can you send an actual patch file instead of diff posted in email for the review? I can't apply this change on top of EDKII trunk code. Thanks, Tapan -----Original Message----- From: Qiu Shumin [mailto:shumin....@intel.com] Sent: Tuesday, July 07, 2015 2:13 AM To: edk2-devel@lists.sourceforge.net Cc: Qiu Shumin; Ruiyu Ni; Heyi Guo; Shah, Tapan Subject: [edk2][PATCH] ShellPkg: Refine code to use Strn**S safe functions instead of Str**S ones in some cases. Safe string functions may ASSERT when the source length is larger than the MaxDest. This patch use Strn**S to indicate the copy length. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Qiu Shumin <shumin....@intel.com> CC: Ruiyu Ni <ruiyu...@intel.com> CC: Heyi Guo <heyi....@linaro.org> CC: Shah, Tapan <tapands...@hp.com> --- ShellPkg/Application/Shell/FileHandleWrappers.c | 4 +- ShellPkg/Application/Shell/Shell.c | 67 ++++++++++++---------- ShellPkg/Library/UefiDpLib/DpUtilities.c | 8 +-- .../Library/UefiShellDebug1CommandsLib/DmpStore.c | 2 +- .../SmbiosView/QueryTable.c | 4 +- .../UefiShellNetwork1CommandsLib/Ifconfig.c | 2 +- 6 files changed, 46 insertions(+), 41 deletions(-) diff --git a/ShellPkg/Application/Shell/FileHandleWrappers.c b/ShellPkg/Application/Shell/FileHandleWrappers.c index 8a71502..18b6c3e 100644 --- a/ShellPkg/Application/Shell/FileHandleWrappers.c +++ b/ShellPkg/Application/Shell/FileHandleWrappers.c @@ -509,7 +509,7 @@ FileInterfaceStdInRead( if (StrStr(CurrentString + TabPos, L":") == NULL) { Cwd = ShellInfoObject.NewEfiShellProtocol->GetCurDir(NULL); if (Cwd != NULL) { - StrCpyS(TabStr, (*BufferSize)/sizeof(CHAR16), Cwd); + StrnCpyS(TabStr, (*BufferSize)/sizeof(CHAR16), Cwd, + (*BufferSize)/sizeof(CHAR16) - 1); if (TabStr[StrLen(TabStr)-1] == L'\\' && *(CurrentString + TabPos) == L'\\' ) { TabStr[StrLen(TabStr)-1] = CHAR_NULL; } @@ -523,7 +523,7 @@ FileInterfaceStdInRead( StrnCatS(TabStr, (*BufferSize)/sizeof(CHAR16), CurrentString + TabPos, StringLen - TabPos); } } else { - StrCpyS(TabStr, (*BufferSize)/sizeof(CHAR16), CurrentString + TabPos); + StrnCpyS(TabStr, (*BufferSize)/sizeof(CHAR16), CurrentString + + TabPos, (*BufferSize)/sizeof(CHAR16) - 1); } StrnCatS(TabStr, (*BufferSize)/sizeof(CHAR16), L"*", (*BufferSize)/sizeof(CHAR16) - 1 - StrLen(TabStr)); FoundFileList = NULL; diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c index 779bb53..78a64de 100644 --- a/ShellPkg/Application/Shell/Shell.c +++ b/ShellPkg/Application/Shell/Shell.c @@ -2564,6 +2564,7 @@ RunScriptFileHandle ( EFI_STATUS Status; SCRIPT_FILE *NewScriptFile; UINTN LoopVar; + UINTN PrintBuffSize; CHAR16 *CommandLine; CHAR16 *CommandLine2; CHAR16 *CommandLine3; @@ -2578,6 +2579,7 @@ RunScriptFileHandle ( ASSERT(!ShellCommandGetScriptExit()); PreScriptEchoState = ShellCommandGetEchoState(); + PrintBuffSize = PcdGet16(PcdShellPrintBufferSize); NewScriptFile = (SCRIPT_FILE*)AllocateZeroPool(sizeof(SCRIPT_FILE)); if (NewScriptFile == NULL) { @@ -2652,12 +2654,12 @@ RunScriptFileHandle ( // // Now enumerate through the commands and run each one. // - CommandLine = AllocateZeroPool(PcdGet16(PcdShellPrintBufferSize)); + CommandLine = AllocateZeroPool(PrintBuffSize); if (CommandLine == NULL) { DeleteScriptFileStruct(NewScriptFile); return (EFI_OUT_OF_RESOURCES); } - CommandLine2 = AllocateZeroPool(PcdGet16(PcdShellPrintBufferSize)); + CommandLine2 = AllocateZeroPool(PrintBuffSize); if (CommandLine2 == NULL) { FreePool(CommandLine); DeleteScriptFileStruct(NewScriptFile); @@ -2669,9 +2671,10 @@ RunScriptFileHandle ( ; // conditional increment in the body of the loop ){ ASSERT(CommandLine2 != NULL); - StrCpyS( CommandLine2, - PcdGet16(PcdShellPrintBufferSize)/sizeof(CHAR16), - NewScriptFile->CurrentCommand->Cl + StrnCpyS( CommandLine2, + PrintBuffSize/sizeof(CHAR16), + NewScriptFile->CurrentCommand->Cl, + PrintBuffSize/sizeof(CHAR16) - 1 ); // @@ -2693,9 +2696,10 @@ RunScriptFileHandle ( // // Due to variability in starting the find and replace action we need to have both buffers the same. // - StrCpyS( CommandLine, - PcdGet16(PcdShellPrintBufferSize)/sizeof(CHAR16), - CommandLine2 + StrnCpyS( CommandLine, + PrintBuffSize/sizeof(CHAR16), + CommandLine2, + PrintBuffSize/sizeof(CHAR16) - 1 ); // @@ -2704,53 +2708,54 @@ RunScriptFileHandle ( if (NewScriptFile->Argv != NULL) { switch (NewScriptFile->Argc) { default: - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%9", NewScriptFile->Argv[9], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine2, + CommandLine, PrintBuffSize, L"%9", NewScriptFile->Argv[9], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 9: - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%8", NewScriptFile->Argv[8], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine, + CommandLine2, PrintBuffSize, L"%8", NewScriptFile->Argv[8], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 8: - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%7", NewScriptFile->Argv[7], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine2, + CommandLine, PrintBuffSize, L"%7", NewScriptFile->Argv[7], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 7: - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%6", NewScriptFile->Argv[6], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine, + CommandLine2, PrintBuffSize, L"%6", NewScriptFile->Argv[6], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 6: - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%5", NewScriptFile->Argv[5], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine2, + CommandLine, PrintBuffSize, L"%5", NewScriptFile->Argv[5], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 5: - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%4", NewScriptFile->Argv[4], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine, + CommandLine2, PrintBuffSize, L"%4", NewScriptFile->Argv[4], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 4: - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%3", NewScriptFile->Argv[3], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine2, + CommandLine, PrintBuffSize, L"%3", NewScriptFile->Argv[3], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 3: - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%2", NewScriptFile->Argv[2], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine, + CommandLine2, PrintBuffSize, L"%2", NewScriptFile->Argv[2], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 2: - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%1", NewScriptFile->Argv[1], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine2, + CommandLine, PrintBuffSize, L"%1", NewScriptFile->Argv[1], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); case 1: - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%0", NewScriptFile->Argv[0], FALSE, TRUE); + Status = ShellCopySearchAndReplace(CommandLine, + CommandLine2, PrintBuffSize, L"%0", NewScriptFile->Argv[0], FALSE, + TRUE); ASSERT_EFI_ERROR(Status); break; case 0: break; } } - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%1", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%2", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%3", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%4", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%5", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%6", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%7", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PcdGet16 (PcdShellPrintBufferSize), L"%8", L"\"\"", FALSE, FALSE); - Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PcdGet16 (PcdShellPrintBufferSize), L"%9", L"\"\"", FALSE, FALSE); - - StrCpyS( CommandLine2, - PcdGet16(PcdShellPrintBufferSize)/sizeof(CHAR16), - CommandLine + Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PrintBuffSize, L"%1", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PrintBuffSize, L"%2", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PrintBuffSize, L"%3", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PrintBuffSize, L"%4", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PrintBuffSize, L"%5", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PrintBuffSize, L"%6", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, PrintBuffSize, L"%7", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine, CommandLine2, PrintBuffSize, L"%8", L"\"\"", FALSE, FALSE); + Status = ShellCopySearchAndReplace(CommandLine2, CommandLine, + PrintBuffSize, L"%9", L"\"\"", FALSE, FALSE); + + StrnCpyS( CommandLine2, + PrintBuffSize/sizeof(CHAR16), + CommandLine, + PrintBuffSize/sizeof(CHAR16) - 1 ); LastCommand = NewScriptFile->CurrentCommand; diff --git a/ShellPkg/Library/UefiDpLib/DpUtilities.c b/ShellPkg/Library/UefiDpLib/DpUtilities.c index 7290285..063eb65 100644 --- a/ShellPkg/Library/UefiDpLib/DpUtilities.c +++ b/ShellPkg/Library/UefiDpLib/DpUtilities.c @@ -261,7 +261,7 @@ GetNameFromHandle ( ); if (!EFI_ERROR (Status)) { SHELL_FREE_NON_NULL (PlatformLanguage); - StrCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, StringPtr); + StrnCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, StringPtr, + DP_GAUGE_STRING_LENGTH); mGaugeString[DP_GAUGE_STRING_LENGTH] = 0; return; } @@ -305,7 +305,7 @@ GetNameFromHandle ( // // Method 3. Get the name string from FFS UI section // - StrCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, NameString); + StrnCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, NameString, + DP_GAUGE_STRING_LENGTH); mGaugeString[DP_GAUGE_STRING_LENGTH] = 0; FreePool (NameString); } else { @@ -321,7 +321,7 @@ GetNameFromHandle ( // NameString = ConvertDevicePathToText (LoadedImageDevicePath, TRUE, FALSE); if (NameString != NULL) { - StrCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, NameString); + StrnCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, NameString, + DP_GAUGE_STRING_LENGTH); mGaugeString[DP_GAUGE_STRING_LENGTH] = 0; FreePool (NameString); return; @@ -334,7 +334,7 @@ GetNameFromHandle ( // StringPtr = HiiGetString (gDpHiiHandle, STRING_TOKEN (STR_DP_ERROR_NAME), NULL); ASSERT (StringPtr != NULL); - StrCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, StringPtr); + StrnCpyS (mGaugeString, DP_GAUGE_STRING_LENGTH + 1, StringPtr, + DP_GAUGE_STRING_LENGTH); FreePool (StringPtr); } diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c index d818b9b..ac6d0bd 100644 --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c @@ -406,7 +406,7 @@ CascadeProcessVariables ( FoundVarName = AllocateZeroPool (NameSize); if (FoundVarName != NULL) { if (PrevName != NULL) { - StrCpyS(FoundVarName, NameSize/sizeof(CHAR16), PrevName); + StrnCpyS(FoundVarName, NameSize/sizeof(CHAR16), PrevName, + NameSize/sizeof(CHAR16) - 1); } Status = gRT->GetNextVariableName (&NameSize, FoundVarName, &FoundVarGuid); diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c index dd878c4..759f486 100644 --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable. +++ c @@ -3229,8 +3229,8 @@ QueryTable ( // if ((High > Low && Key >= Low && Key <= High) || (Table[Index].Key == Key)) { - StrCpyS (Info, InfoLen, Table[Index].Info); - StrCatS (Info, InfoLen, L"\n"); + StrnCpyS (Info, InfoLen, Table[Index].Info, InfoLen - 1); + StrnCatS (Info, InfoLen, L"\n", InfoLen - 1 - StrLen(Info)); return Key; } } diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c index fa00d6c..4f5eaf8 100644 --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c @@ -625,7 +625,7 @@ IfconfigGetAllNicInfoByHii ( goto ON_ERROR; } if (ConfigHdr != NULL) { - StrnCpyS (ConfigResp, ConfigRespBufferSize/sizeof(CHAR16), ConfigHdr, Length + NIC_ITEM_CONFIG_SIZE * 2 + 100 - 1); + StrnCpyS (ConfigResp, ConfigRespBufferSize/sizeof(CHAR16), + ConfigHdr, ConfigRespBufferSize/sizeof(CHAR16) - 1); } // -- 1.9.5.msysgit.1 ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
