Skip to content

Commit

Permalink
ShellPkg: CodeQL Fixes.
Browse files Browse the repository at this point in the history 
Includes changes across the repo for the following CodeQL rules:
- cpp/comparison-with-wider-type
- cpp/overflow-buffer
- cpp/redundant-null-check-param
- cpp/uselesstest

Some false positives are filtered in CodeQlFilters.yml.

Co-authored-by: Taylor Beebe <[email protected]>
  • Loading branch information
@makubacki @TaylorBeebe @apop5
2 people authored and apop5 committed Aug 23, 2024
1 parent b001cc9 commit 22ae568
Show file tree
Hide file tree
Showing 57 changed files with 1,177 additions and 235 deletions.
6 changes: 6 additions & 0 deletions ShellPkg/Application/Shell/FileHandleWrappers.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2144,6 +2144,12 @@ FileInterfaceFileWrite (
// Ascii
//
AsciiBuffer = AllocateZeroPool (*BufferSize);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (AsciiBuffer == NULL) {
return EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
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));
Expand Down
29 changes: 22 additions & 7 deletions ShellPkg/Application/Shell/Shell.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,13 @@ UefiMain (

Size = 100;
TempString = AllocateZeroPool (Size);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (TempString == NULL) {
ASSERT (TempString != NULL);
return EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference

UnicodeSPrint (TempString, Size, L"%d", PcdGet8 (PcdShellSupportLevel));
Status = InternalEfiShellSetEnv (L"uefishellsupport", TempString, TRUE);
Expand Down Expand Up @@ -1326,7 +1333,8 @@ DoStartupScript (
}

Status = RunShellCommand (FileStringPath, &CalleeStatus);
if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) {
if ((!EFI_ERROR (Status)) && (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) {
// MU_CHANGE - CodeQL Change - unguardednullreturndereference
ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (), (UINT64)CalleeStatus);
}

Expand Down Expand Up @@ -2609,11 +2617,18 @@ RunCommandOrFile (
CommandWithPath = ShellFindFilePathEx (FirstParameter, mExecutableExtensions);
}

//
// This should be impossible now.
//
ASSERT (CommandWithPath != NULL);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
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;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
//
// Make sure that path is not just a directory (or not found)
//
Expand Down Expand Up @@ -3330,8 +3345,8 @@ FindFirstCharacter (
IN CONST CHAR16 EscapeCharacter
)
{
UINT32 WalkChar;
UINT32 WalkStr;
UINTN WalkChar; // MU_CHANGE - CodeQL Change - comparison-with-wider-type
UINTN WalkStr; // MU_CHANGE - CodeQL Change - comparison-with-wider-type

for (WalkStr = 0; WalkStr < StrLen (String); WalkStr++) {
if (String[WalkStr] == EscapeCharacter) {
Expand Down
35 changes: 31 additions & 4 deletions ShellPkg/Application/Shell/ShellManParser.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. // MU_CHANGE - CodeQL change
@retval EFI_NOT_FOUND There is no help text available for Command.
**/
EFI_STATUS
Expand Down Expand Up @@ -600,7 +601,14 @@ ProcessManFile (
TempString = ShellCommandGetCommandHelp (Command);
if (TempString != NULL) {
FileHandle = ConvertEfiFileProtocolToShellHandle (CreateFileInterfaceMem (TRUE), NULL);
HelpSize = StrLen (TempString) * sizeof (CHAR16);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (FileHandle == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto Done;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
HelpSize = StrLen (TempString) * sizeof (CHAR16);
ShellWriteFile (FileHandle, &HelpSize, TempString);
ShellSetFilePosition (FileHandle, 0);
HelpSize = 0;
Expand All @@ -624,8 +632,20 @@ 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);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
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;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
Status = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0);
SHELL_FREE_NON_NULL (FileDevPath);
SHELL_FREE_NON_NULL (DevPath);
}
Expand Down Expand Up @@ -733,7 +753,14 @@ ProcessManFile (
}

FileHandle = ConvertEfiFileProtocolToShellHandle (CreateFileInterfaceMem (TRUE), NULL);
HelpSize = StrLen (TempString) * sizeof (CHAR16);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (FileHandle == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto Done;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
HelpSize = StrLen (TempString) * sizeof (CHAR16);
ShellWriteFile (FileHandle, &HelpSize, TempString);
ShellSetFilePosition (FileHandle, 0);
HelpSize = 0;
Expand Down
57 changes: 48 additions & 9 deletions ShellPkg/Application/Shell/ShellParametersProtocol.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,13 @@ 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);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (FullCommandLine == NULL) {
return EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
Status = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine);
}

if (Status == EFI_NOT_FOUND) {
Expand Down Expand Up @@ -738,6 +744,7 @@ UpdateStdInStdOutStdErr (
OutAppend = FALSE;
CommandLineCopy = NULL;
FirstLocation = NULL;
TempHandle = NULL; // MU_CHANGE - CodeQL change - conditionallyuninitializedvariable

if ((ShellParameters == NULL) || (SystemTableInfo == NULL) || (OldStdIn == NULL) || (OldStdOut == NULL) || (OldStdErr == NULL)) {
return (EFI_INVALID_PARAMETER);
Expand Down Expand Up @@ -1176,7 +1183,13 @@ UpdateStdInStdOutStdErr (

if (!ErrUnicode && !EFI_ERROR (Status)) {
TempHandle = CreateFileInterfaceFile (TempHandle, FALSE);
ASSERT (TempHandle != NULL);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (TempHandle == NULL) {
ASSERT (TempHandle != NULL);
Status = EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
}

if (!EFI_ERROR (Status)) {
Expand Down Expand Up @@ -1223,7 +1236,13 @@ UpdateStdInStdOutStdErr (

if (!OutUnicode && !EFI_ERROR (Status)) {
TempHandle = CreateFileInterfaceFile (TempHandle, FALSE);
ASSERT (TempHandle != NULL);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (TempHandle == NULL) {
ASSERT (TempHandle != NULL);
Status = EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
}

if (!EFI_ERROR (Status)) {
Expand All @@ -1245,9 +1264,16 @@ UpdateStdInStdOutStdErr (
}

TempHandle = CreateFileInterfaceEnv (StdOutVarName);
ASSERT (TempHandle != NULL);
ShellParameters->StdOut = TempHandle;
gST->ConOut = CreateSimpleTextOutOnFile (TempHandle, &gST->ConsoleOutHandle, gST->ConOut);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (TempHandle == NULL) {
ASSERT (TempHandle != NULL);
Status = EFI_OUT_OF_RESOURCES;
} else {
ShellParameters->StdOut = TempHandle;
gST->ConOut = CreateSimpleTextOutOnFile (TempHandle, &gST->ConsoleOutHandle, gST->ConOut);
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
}

//
Expand All @@ -1262,9 +1288,16 @@ UpdateStdInStdOutStdErr (
}

TempHandle = CreateFileInterfaceEnv (StdErrVarName);
ASSERT (TempHandle != NULL);
ShellParameters->StdErr = TempHandle;
gST->StdErr = CreateSimpleTextOutOnFile (TempHandle, &gST->StandardErrorHandle, gST->StdErr);
// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (TempHandle == NULL) {
ASSERT (TempHandle != NULL);
Status = EFI_OUT_OF_RESOURCES;
} else {
ShellParameters->StdErr = TempHandle;
gST->StdErr = CreateSimpleTextOutOnFile (TempHandle, &gST->StandardErrorHandle, gST->StdErr);
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
}

//
Expand Down Expand Up @@ -1307,6 +1340,12 @@ UpdateStdInStdOutStdErr (
TempHandle = CreateFileInterfaceFile (TempHandle, FALSE);
}

// MU_CHANGE Start - CodeQL Change - unguardednullreturndereference
if (TempHandle == NULL) {
return EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL Change - unguardednullreturndereference
ShellParameters->StdIn = TempHandle;
gST->ConIn = CreateSimpleTextInOnFile (TempHandle, &gST->ConsoleInHandle);
}
Expand Down
Loading

0 comments on commit 22ae568

Please sign in to comment.