Re: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables


Michael Kubacki
 

Thanks. I will include this in the v2 series.

On 11/23/2022 9:19 PM, Gao, Zhichao wrote:
See comments below:

-----Original Message-----
From: mikuback@... <mikuback@...>
Sent: Thursday, November 10, 2022 1:33 AM
To: devel@edk2.groups.io
Cc: Erich McMillan <emcmillan@...>; Kinney, Michael D
<michael.d.kinney@...>; Michael Kubacki
<mikuback@...>; Ni, Ray <ray.ni@...>; Gao, Zhichao
<zhichao.gao@...>
Subject: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables

From: Michael Kubacki <michael.kubacki@...>

Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html

Cc: Erich McMillan <emcmillan@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
ShellPkg/Application/Shell/Shell.c | 2 +-
ShellPkg/Application/Shell/ShellProtocol.c | 4 +-
ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 56
+++++++++++---------
ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c | 18 ++++---
ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c | 9 ++--
ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 14 ++---
ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c | 17 +++---
ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c | 21 ++++----
8 files changed, 78 insertions(+), 63 deletions(-)

diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index df00adfdfa5b..86db2f4ebb6e 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1324,7 +1324,7 @@ DoStartupScript (
}

Status = RunShellCommand (FileStringPath, &CalleeStatus);
- if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) {
+ if (!EFI_ERROR (Status) &&
+ (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) {
Incorrect here. Cannot handle the unsuccess condition. Better to assign the success initial value to Calleestatus and keep the org logic.

ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (),
(UINT64)CalleeStatus);
}

diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
b/ShellPkg/Application/Shell/ShellProtocol.c
index 509eb60e40f4..9183da284fff 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -729,8 +729,8 @@ EfiShellGetDeviceName (
//
// Now check the parent controller using this as the child.
//
- if (DeviceNameToReturn == NULL) {
- PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
&ParentControllerCount, &ParentControllerBuffer);
+ Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
&ParentControllerCount, &ParentControllerBuffer);
+ if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) {
for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
PARSE_HANDLE_DATABASE_UEFI_DRIVERS
(ParentControllerBuffer[LoopVar], &ParentDriverCount,
&ParentDriverBuffer);
Should we cover above function as well?
It was not identified by the query results, but I can add it if we like.

Others looks good to me.
Thanks,
Zhichao

for (HandleCount = 0; HandleCount < ParentDriverCount;
HandleCount++) { diff --git
a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
index 36cf46fb2c38..4549cbde9b9a 100644
--- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
+++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
@@ -1399,10 +1399,11 @@ ShellCommandCreateInitialMappingsAndPaths (
CHAR16 *MapName;
SHELL_MAP_LIST *MapListItem;

- SplitCurDir = NULL;
- MapName = NULL;
- MapListItem = NULL;
- HandleList = NULL;
+ ConsistMappingTable = NULL;
+ SplitCurDir = NULL;
+ MapName = NULL;
+ MapListItem = NULL;
+ HandleList = NULL;

//
// Reset the static members back to zero @@ -1458,32 +1459,35 @@
ShellCommandCreateInitialMappingsAndPaths (
//
PerformQuickSort (DevicePathList, Count, sizeof
(EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);

- ShellCommandConsistMappingInitialize (&ConsistMappingTable);
- //
- // Assign new Mappings to all...
- //
- for (Count = 0; HandleList[Count] != NULL; Count++) {
+ if (!EFI_ERROR (ShellCommandConsistMappingInitialize
+ (&ConsistMappingTable))) {
//
- // Get default name first
+ // Assign new Mappings to all...
//
- NewDefaultName = ShellCommandCreateNewMappingName
(MappingTypeFileSystem);
- ASSERT (NewDefaultName != NULL);
- Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName,
DevicePathList[Count], 0, TRUE);
- ASSERT_EFI_ERROR (Status);
- FreePool (NewDefaultName);
-
- //
- // Now do consistent name
- //
- NewConsistName = ShellCommandConsistMappingGenMappingName
(DevicePathList[Count], ConsistMappingTable);
- if (NewConsistName != NULL) {
- Status = ShellCommandAddMapItemAndUpdatePath
(NewConsistName, DevicePathList[Count], 0, FALSE);
+ for (Count = 0; HandleList[Count] != NULL; Count++) {
+ //
+ // Get default name first
+ //
+ NewDefaultName = ShellCommandCreateNewMappingName
(MappingTypeFileSystem);
+ ASSERT (NewDefaultName != NULL);
+ Status = ShellCommandAddMapItemAndUpdatePath
(NewDefaultName,
+ DevicePathList[Count], 0, TRUE);
ASSERT_EFI_ERROR (Status);
- FreePool (NewConsistName);
+ FreePool (NewDefaultName);
+
+ //
+ // Now do consistent name
+ //
+ NewConsistName = ShellCommandConsistMappingGenMappingName
(DevicePathList[Count], ConsistMappingTable);
+ if (NewConsistName != NULL) {
+ Status = ShellCommandAddMapItemAndUpdatePath
(NewConsistName, DevicePathList[Count], 0, FALSE);
+ ASSERT_EFI_ERROR (Status);
+ FreePool (NewConsistName);
+ }
}
}

- ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
+ if (ConsistMappingTable != NULL) {
+ ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
+ }

SHELL_FREE_NON_NULL (HandleList);
SHELL_FREE_NON_NULL (DevicePathList); @@ -1626,12 +1630,12 @@
ShellCommandUpdateMapping (
//
PerformQuickSort (DevicePathList, Count, sizeof
(EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);

- ShellCommandConsistMappingInitialize (&ConsistMappingTable);
+ Status = ShellCommandConsistMappingInitialize
+ (&ConsistMappingTable);

//
// Assign new Mappings to remainders
//
- for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL
&& !EFI_ERROR (Status); Count++) {
+ for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL;
+ Count++) {
//
// Skip ones that already have
//
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
index 97a4b57a932f..5329b559ba46 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
@@ -158,7 +158,10 @@ ShellCommandRunDblk (
ShellStatus = SHELL_INVALID_PARAMETER;
}

- ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE);
+ if (EFI_ERROR (ShellConvertStringToUint64 (LbaString, &Lba, TRUE,
FALSE))) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
gShellDebug1HiiHandle, L"dblk", LbaString);
+ ShellStatus = SHELL_INVALID_PARAMETER;
+ }
}

if (BlockCountString == NULL) {
@@ -169,12 +172,13 @@ ShellCommandRunDblk (
ShellStatus = SHELL_INVALID_PARAMETER;
}

- ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE,
FALSE);
- if (BlockCount > 0x10) {
- BlockCount = 0x10;
- } else if (BlockCount == 0) {
- ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
gShellDebug1HiiHandle, L"dblk", BlockCountString);
- ShellStatus = SHELL_INVALID_PARAMETER;
+ if (!EFI_ERROR (ShellConvertStringToUint64 (BlockCountString,
&BlockCount, TRUE, FALSE))) {
+ if (BlockCount > 0x10) {
+ BlockCount = 0x10;
+ } else if (BlockCount == 0) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
gShellDebug1HiiHandle, L"dblk", BlockCountString);
+ ShellStatus = SHELL_INVALID_PARAMETER;
+ }
}
}

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
index 8bf23a2076a1..72f8c087cb69 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
@@ -112,10 +112,13 @@ ShellCommandRunEfiDecompress (

if (ShellStatus == SHELL_SUCCESS) {
Status = FileHandleGetSize (InFileHandle, &Temp64Bit);
- ASSERT (Temp64Bit <= (UINT32)(-1));
- InSize = (UINTN)Temp64Bit;
ASSERT_EFI_ERROR (Status);
- InBuffer = AllocateZeroPool (InSize);
+ if (!EFI_ERROR (Status)) {
+ ASSERT (Temp64Bit <= (UINT32)(-1));
+ InSize = (UINTN)Temp64Bit;
+ InBuffer = AllocateZeroPool (InSize);
+ }
+
if (InBuffer == NULL) {
Status = EFI_OUT_OF_RESOURCES;
} else {
diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
index d7a133c0c5b4..870c5b0d1da7 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
@@ -508,9 +508,10 @@ ShellCommandRunConnect (
Count = ShellCommandLineGetCount (Package);

if (Param1 != NULL) {
- Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE,
FALSE);
- Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
- if (EFI_ERROR (Status)) {
+ Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE,
FALSE);
+ if (!EFI_ERROR (Status)) {
+ Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
gShellDriver1HiiHandle, L"connect", Param1);
ShellStatus = SHELL_INVALID_PARAMETER;
}
@@ -519,9 +520,10 @@ ShellCommandRunConnect (
}

if (Param2 != NULL) {
- Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE,
FALSE);
- Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
- if (EFI_ERROR (Status)) {
+ Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE,
FALSE);
+ if (!EFI_ERROR (Status)) {
+ Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
gShellDriver1HiiHandle, L"connect", Param2);
ShellStatus = SHELL_INVALID_PARAMETER;
}
diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
index 009ae5282b27..fd49d1f7ceb4 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
@@ -160,12 +160,17 @@ ShellCommandRunDisconnect (
Param1 = ShellCommandLineGetRawValue (Package, 1);
Param2 = ShellCommandLineGetRawValue (Package, 2);
Param3 = ShellCommandLineGetRawValue (Package, 3);
- ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE);
- Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
((UINTN)Intermediate1) : NULL;
- ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE);
- Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
((UINTN)Intermediate2) : NULL;
- ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE);
- Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
((UINTN)Intermediate3) : NULL;
+ if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1,
TRUE, FALSE))) {
+ Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
((UINTN)Intermediate1) : NULL;
+ }
+
+ if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2,
TRUE, FALSE))) {
+ Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
((UINTN)Intermediate2) : NULL;
+ }
+
+ if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3,
TRUE, FALSE))) {
+ Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
((UINTN)Intermediate3) : NULL;
+ }

if ((Param1 != NULL) && (Handle1 == NULL)) {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
gShellDriver1HiiHandle, L"disconnect", Param1); diff --git
a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
index c645c9fd6882..8f70d6b6af39 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
@@ -438,25 +438,22 @@ ShellCommandRunDrvDiag (
ControllerHandleStr = ShellCommandLineGetRawValue (Package, 2);
ChildHandleStr = ShellCommandLineGetRawValue (Package, 3);

- if (DriverHandleStr == NULL) {
- Handle1 = NULL;
- } else {
- ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE,
FALSE);
+ if ((DriverHandleStr != NULL) && ShellConvertStringToUint64
+ (DriverHandleStr, &Intermediate, TRUE, FALSE)) {
Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
+ Handle1 = NULL;
}

- if (ControllerHandleStr == NULL) {
- Handle2 = NULL;
- } else {
- ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE,
FALSE);
+ if ((ControllerHandleStr != NULL) && ShellConvertStringToUint64
+ (ControllerHandleStr, &Intermediate, TRUE, FALSE)) {
Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
+ Handle2 = NULL;
}

- if (ChildHandleStr == NULL) {
- Handle3 = NULL;
- } else {
- ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE,
FALSE);
+ if ((ChildHandleStr != NULL) && ShellConvertStringToUint64
+ (ChildHandleStr, &Intermediate, TRUE, FALSE)) {
Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
+ Handle3 = NULL;
}

Status = DoDiagnostics (
--
2.28.0.windows.1

Join devel@edk2.groups.io to automatically receive all group messages.