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: [email protected] <[email protected]>
Sent: Thursday, November 10, 2022 1:33 AM
To: [email protected]
Cc: Erich McMillan <[email protected]>; Kinney, Michael D
<[email protected]>; Michael Kubacki
<[email protected]>; Ni, Ray <[email protected]>; Gao, Zhichao
<[email protected]>
Subject: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables

From: Michael Kubacki <[email protected]>

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

Cc: Erich McMillan <[email protected]>
Cc: Michael D Kinney <[email protected]>
Cc: Michael Kubacki <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Zhichao Gao <[email protected]>
Co-authored-by: Erich McMillan <[email protected]>
Signed-off-by: Michael Kubacki <[email protected]>
---
  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







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96608): https://edk2.groups.io/g/devel/message/96608
Mute This Topic: https://groups.io/mt/94918102/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to