From 875202bf85d528fb7c63c4827cfe013b58f2870f Mon Sep 17 00:00:00 2001 From: Oliver Smith-Denny Date: Thu, 3 Oct 2024 10:30:15 -0700 Subject: ShellPkg: Shell: CodeQL Fixes Includes changes across the module for the following CodeQL rules: - cpp/comparison-with-wider-type - cpp/overflow-buffer - cpp/redundant-null-check-param - cpp/uselesstest Co-authored-by: Taylor Beebe Signed-off-by: Oliver Smith-Denny --- ShellPkg/Application/Shell/FileHandleWrappers.c | 4 + ShellPkg/Application/Shell/Shell.c | 24 ++++-- ShellPkg/Application/Shell/ShellManParser.c | 29 ++++++- .../Application/Shell/ShellParametersProtocol.c | 45 ++++++++--- ShellPkg/Application/Shell/ShellProtocol.c | 88 +++++++++++++++++----- 5 files changed, 151 insertions(+), 39 deletions(-) diff --git a/ShellPkg/Application/Shell/FileHandleWrappers.c b/ShellPkg/Application/Shell/FileHandleWrappers.c index 2ba9b3401a..f98d86a16f 100644 --- a/ShellPkg/Application/Shell/FileHandleWrappers.c +++ b/ShellPkg/Application/Shell/FileHandleWrappers.c @@ -2144,6 +2144,10 @@ FileInterfaceFileWrite ( // Ascii // AsciiBuffer = AllocateZeroPool (*BufferSize); + if (AsciiBuffer == NULL) { + return EFI_OUT_OF_RESOURCES; + } + AsciiSPrint (AsciiBuffer, *BufferSize, "%S", Buffer); Size = AsciiStrSize (AsciiBuffer) - 1; // (we dont need the null terminator) Status = (((EFI_FILE_PROTOCOL_FILE *)This)->Orig->Write (((EFI_FILE_PROTOCOL_FILE *)This)->Orig, &Size, AsciiBuffer)); diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c index 01b4e37871..173ec8c5d4 100644 --- a/ShellPkg/Application/Shell/Shell.c +++ b/ShellPkg/Application/Shell/Shell.c @@ -576,6 +576,11 @@ UefiMain ( Size = 100; TempString = AllocateZeroPool (Size); + if (TempString == NULL) { + ASSERT (TempString != NULL); + Status = EFI_OUT_OF_RESOURCES; + goto FreeResources; + } UnicodeSPrint (TempString, Size, L"%d", PcdGet8 (PcdShellSupportLevel)); Status = InternalEfiShellSetEnv (L"uefishellsupport", TempString, TRUE); @@ -1326,7 +1331,7 @@ DoStartupScript ( } Status = RunShellCommand (FileStringPath, &CalleeStatus); - if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) { + if ((!EFI_ERROR (Status)) && (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) { ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (), (UINT64)CalleeStatus); } @@ -2609,10 +2614,15 @@ RunCommandOrFile ( CommandWithPath = ShellFindFilePathEx (FirstParameter, mExecutableExtensions); } - // - // This should be impossible now. - // - ASSERT (CommandWithPath != NULL); + if (CommandWithPath == NULL) { + // + // This should be impossible now. + // + ASSERT (CommandWithPath != NULL); + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_SHELL_NOT_FOUND), ShellInfoObject.HiiHandle, FirstParameter); + SetLastError (SHELL_NOT_FOUND); + return EFI_NOT_FOUND; + } // // Make sure that path is not just a directory (or not found) @@ -3330,8 +3340,8 @@ FindFirstCharacter ( IN CONST CHAR16 EscapeCharacter ) { - UINT32 WalkChar; - UINT32 WalkStr; + UINTN WalkChar; + UINTN WalkStr; for (WalkStr = 0; WalkStr < StrLen (String); WalkStr++) { if (String[WalkStr] == EscapeCharacter) { diff --git a/ShellPkg/Application/Shell/ShellManParser.c b/ShellPkg/Application/Shell/ShellManParser.c index 5c823cd7f5..f7082467c7 100644 --- a/ShellPkg/Application/Shell/ShellManParser.c +++ b/ShellPkg/Application/Shell/ShellManParser.c @@ -549,6 +549,7 @@ ManFileFindTitleSection ( returned help text. @retval EFI_INVALID_PARAMETER HelpText is NULL. @retval EFI_INVALID_PARAMETER ManFileName is invalid. + @retval EFI_INVALID_PARAMETER Command is invalid. @retval EFI_NOT_FOUND There is no help text available for Command. **/ EFI_STATUS @@ -600,7 +601,12 @@ ProcessManFile ( TempString = ShellCommandGetCommandHelp (Command); if (TempString != NULL) { FileHandle = ConvertEfiFileProtocolToShellHandle (CreateFileInterfaceMem (TRUE), NULL); - HelpSize = StrLen (TempString) * sizeof (CHAR16); + if (FileHandle == NULL) { + Status = EFI_OUT_OF_RESOURCES; + goto Done; + } + + HelpSize = StrLen (TempString) * sizeof (CHAR16); ShellWriteFile (FileHandle, &HelpSize, TempString); ShellSetFilePosition (FileHandle, 0); HelpSize = 0; @@ -624,8 +630,18 @@ ProcessManFile ( Status = SearchPathForFile (TempString, &FileHandle); if (EFI_ERROR (Status)) { FileDevPath = FileDevicePath (NULL, TempString); - DevPath = AppendDevicePath (ShellInfoObject.ImageDevPath, FileDevPath); - Status = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0); + if (FileDevPath == NULL) { + Status = EFI_OUT_OF_RESOURCES; + goto Done; + } + + DevPath = AppendDevicePath (ShellInfoObject.ImageDevPath, FileDevPath); + if (DevPath == NULL) { + Status = EFI_OUT_OF_RESOURCES; + goto Done; + } + + Status = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0); SHELL_FREE_NON_NULL (FileDevPath); SHELL_FREE_NON_NULL (DevPath); } @@ -733,7 +749,12 @@ ProcessManFile ( } FileHandle = ConvertEfiFileProtocolToShellHandle (CreateFileInterfaceMem (TRUE), NULL); - HelpSize = StrLen (TempString) * sizeof (CHAR16); + if (FileHandle == NULL) { + Status = EFI_OUT_OF_RESOURCES; + goto Done; + } + + HelpSize = StrLen (TempString) * sizeof (CHAR16); ShellWriteFile (FileHandle, &HelpSize, TempString); ShellSetFilePosition (FileHandle, 0); HelpSize = 0; diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c index 64d67d9f13..8464cbf616 100644 --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c @@ -354,7 +354,11 @@ CreatePopulateInstallShellParametersProtocol ( Status = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine); if (Status == EFI_BUFFER_TOO_SMALL) { FullCommandLine = AllocateZeroPool (Size + LoadedImage->LoadOptionsSize); - Status = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine); + if (FullCommandLine == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + Status = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine); } if (Status == EFI_NOT_FOUND) { @@ -738,6 +742,7 @@ UpdateStdInStdOutStdErr ( OutAppend = FALSE; CommandLineCopy = NULL; FirstLocation = NULL; + TempHandle = NULL; if ((ShellParameters == NULL) || (SystemTableInfo == NULL) || (OldStdIn == NULL) || (OldStdOut == NULL) || (OldStdErr == NULL)) { return (EFI_INVALID_PARAMETER); @@ -1176,7 +1181,10 @@ UpdateStdInStdOutStdErr ( if (!ErrUnicode && !EFI_ERROR (Status)) { TempHandle = CreateFileInterfaceFile (TempHandle, FALSE); - ASSERT (TempHandle != NULL); + if (TempHandle == NULL) { + ASSERT (TempHandle != NULL); + Status = EFI_OUT_OF_RESOURCES; + } } if (!EFI_ERROR (Status)) { @@ -1223,7 +1231,10 @@ UpdateStdInStdOutStdErr ( if (!OutUnicode && !EFI_ERROR (Status)) { TempHandle = CreateFileInterfaceFile (TempHandle, FALSE); - ASSERT (TempHandle != NULL); + if (TempHandle == NULL) { + ASSERT (TempHandle != NULL); + Status = EFI_OUT_OF_RESOURCES; + } } if (!EFI_ERROR (Status)) { @@ -1245,9 +1256,13 @@ UpdateStdInStdOutStdErr ( } TempHandle = CreateFileInterfaceEnv (StdOutVarName); - ASSERT (TempHandle != NULL); - ShellParameters->StdOut = TempHandle; - gST->ConOut = CreateSimpleTextOutOnFile (TempHandle, &gST->ConsoleOutHandle, gST->ConOut); + if (TempHandle == NULL) { + ASSERT (TempHandle != NULL); + Status = EFI_OUT_OF_RESOURCES; + } else { + ShellParameters->StdOut = TempHandle; + gST->ConOut = CreateSimpleTextOutOnFile (TempHandle, &gST->ConsoleOutHandle, gST->ConOut); + } } // @@ -1262,9 +1277,13 @@ UpdateStdInStdOutStdErr ( } TempHandle = CreateFileInterfaceEnv (StdErrVarName); - ASSERT (TempHandle != NULL); - ShellParameters->StdErr = TempHandle; - gST->StdErr = CreateSimpleTextOutOnFile (TempHandle, &gST->StandardErrorHandle, gST->StdErr); + if (TempHandle == NULL) { + ASSERT (TempHandle != NULL); + Status = EFI_OUT_OF_RESOURCES; + } else { + ShellParameters->StdErr = TempHandle; + gST->StdErr = CreateSimpleTextOutOnFile (TempHandle, &gST->StandardErrorHandle, gST->StdErr); + } } // @@ -1307,8 +1326,12 @@ UpdateStdInStdOutStdErr ( TempHandle = CreateFileInterfaceFile (TempHandle, FALSE); } - ShellParameters->StdIn = TempHandle; - gST->ConIn = CreateSimpleTextInOnFile (TempHandle, &gST->ConsoleInHandle); + if (TempHandle == NULL) { + Status = EFI_OUT_OF_RESOURCES; + } else { + ShellParameters->StdIn = TempHandle; + gST->ConIn = CreateSimpleTextInOnFile (TempHandle, &gST->ConsoleInHandle); + } } } } diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c index da8c31cb03..ff53f6ba4f 100644 --- a/ShellPkg/Application/Shell/ShellProtocol.c +++ b/ShellPkg/Application/Shell/ShellProtocol.c @@ -436,7 +436,10 @@ EfiShellGetFilePathFromDevicePath ( if ((DevicePathType (&FilePath->Header) != MEDIA_DEVICE_PATH) || (DevicePathSubType (&FilePath->Header) != MEDIA_FILEPATH_DP)) { - FreePool (PathForReturn); + if (PathForReturn != NULL) { + FreePool (PathForReturn); + } + return NULL; } @@ -447,7 +450,10 @@ EfiShellGetFilePathFromDevicePath ( AlignedNode = AllocateCopyPool (DevicePathNodeLength (FilePath), FilePath); if (AlignedNode == NULL) { - FreePool (PathForReturn); + if (PathForReturn != NULL) { + FreePool (PathForReturn); + } + return NULL; } @@ -719,7 +725,11 @@ EfiShellGetDeviceName ( continue; } - Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE); + Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE); + if (Lang == NULL) { + continue; + } + Status = CompName2->GetControllerName (CompName2, DeviceHandle, NULL, Lang, &DeviceNameToReturn); FreePool (Lang); Lang = NULL; @@ -767,7 +777,11 @@ EfiShellGetDeviceName ( continue; } - Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE); + Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE); + if (Lang == NULL) { + continue; + } + Status = CompName2->GetControllerName (CompName2, ParentControllerBuffer[LoopVar], DeviceHandle, Lang, &DeviceNameToReturn); FreePool (Lang); Lang = NULL; @@ -1817,16 +1831,32 @@ EfiShellExecute ( return (EFI_UNSUPPORTED); } + Temp = NULL; if (NestingEnabled ()) { DevPath = AppendDevicePath (ShellInfoObject.ImageDevPath, ShellInfoObject.FileDevPath); + if (DevPath == NULL) { + return EFI_OUT_OF_RESOURCES; + } DEBUG_CODE_BEGIN (); Temp = ConvertDevicePathToText (ShellInfoObject.FileDevPath, TRUE, TRUE); - FreePool (Temp); + if (Temp != NULL) { + FreePool (Temp); + } + Temp = ConvertDevicePathToText (ShellInfoObject.ImageDevPath, TRUE, TRUE); - FreePool (Temp); - Temp = ConvertDevicePathToText (DevPath, TRUE, TRUE); - FreePool (Temp); + if (Temp != NULL) { + FreePool (Temp); + } + + if (DevPath != NULL) { + Temp = ConvertDevicePathToText (DevPath, TRUE, TRUE); + } + + if (Temp != NULL) { + FreePool (Temp); + } + DEBUG_CODE_END (); Temp = NULL; @@ -2395,6 +2425,8 @@ ShellSearchHandle ( CHAR16 *NewFullName; UINTN Size; + NewShellNode = NULL; + FileInfo = NULL; if ( (FilePattern == NULL) || (UnicodeCollation == NULL) || (FileList == NULL) @@ -2434,14 +2466,17 @@ ShellSearchHandle ( // // We want the root node. create the node. // - FileInfo = FileHandleGetInfo (FileHandle); - NewShellNode = CreateAndPopulateShellFileInfo ( - MapName, - EFI_SUCCESS, - L"\\", - FileHandle, - FileInfo - ); + FileInfo = FileHandleGetInfo (FileHandle); + if (FileInfo != NULL) { + NewShellNode = CreateAndPopulateShellFileInfo ( + MapName, + EFI_SUCCESS, + L"\\", + FileHandle, + FileInfo + ); + } + SHELL_FREE_NON_NULL (FileInfo); } else { // @@ -2631,6 +2666,9 @@ EfiShellFindFiles ( } PatternCopy = PathCleanUpDirectories (PatternCopy); + if (PatternCopy == NULL) { + return (EFI_OUT_OF_RESOURCES); + } Count = StrStr (PatternCopy, L":") - PatternCopy + 1; ASSERT (Count <= StrLen (PatternCopy)); @@ -2715,6 +2753,10 @@ EfiShellOpenFileList ( // if (StrStr (Path, L":") == NULL) { CurDir = EfiShellGetCurDir (NULL); + if (CurDir == NULL) { + return EFI_NOT_FOUND; + } + ASSERT ((Path2 == NULL && Path2Size == 0) || (Path2 != NULL)); StrnCatGrow (&Path2, &Path2Size, CurDir, 0); StrnCatGrow (&Path2, &Path2Size, L"\\", 0); @@ -2855,6 +2897,10 @@ EfiShellGetEnvEx ( // Allocate the space and recall the get function // Buffer = AllocateZeroPool (Size); + if (Buffer == NULL) { + return NULL; + } + Status = SHELL_GET_ENVIRONMENT_VARIABLE_AND_ATTRIBUTES (Name, Attributes, &Size, Buffer); } @@ -3122,7 +3168,10 @@ EfiShellSetCurDir ( } DirectoryName = StrnCatGrow (&DirectoryName, NULL, Dir, 0); - ASSERT (DirectoryName != NULL); + if (DirectoryName == NULL) { + ASSERT (DirectoryName != NULL); + return (EFI_OUT_OF_RESOURCES); + } PathCleanUpDirectories (DirectoryName); @@ -3500,6 +3549,11 @@ EfiShellGetAlias ( Status = gRT->GetVariable (AliasLower, &gShellAliasGuid, &Attribs, &RetSize, RetVal); if (Status == EFI_BUFFER_TOO_SMALL) { RetVal = AllocateZeroPool (RetSize); + if (RetVal == NULL) { + FreePool (AliasLower); + return NULL; + } + Status = gRT->GetVariable (AliasLower, &gShellAliasGuid, &Attribs, &RetSize, RetVal); } -- cgit