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?
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 (#96606): https://edk2.groups.io/g/devel/message/96606
Mute This Topic: https://groups.io/mt/94918102/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
