From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.14465.1669257401010813052 for ; Wed, 23 Nov 2022 18:36:41 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=NacNDest; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id A622120B6C40; Wed, 23 Nov 2022 18:36:39 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A622120B6C40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1669257400; bh=nmGJDAY4wUG7OuZ9u65KmKEQzYt991XZufHwy1xDvUc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=NacNDestXxl+QfAy+TswDWNiOupupe9uZUzyMP/mNVnHm4xi6SY2z6DFO+5111wus v+6hYnJEznbk59FC407ooGgGWQgWqA7sJZIrrKiZqjHit8TUm3FbUCg/oFgyi8TOr6 ipEAeOcur27SeMC7kJVJx10AATE4eqvHH5A12O78= Message-ID: <04321f22-a029-a653-7df7-179e586c07bc@linux.microsoft.com> Date: Wed, 23 Nov 2022 21:36:37 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [edk2-devel] [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables To: devel@edk2.groups.io, zhichao.gao@intel.com Cc: Erich McMillan , "Kinney, Michael D" , "Ni, Ray" References: <20221109173246.174-1-mikuback@linux.microsoft.com> <20221109173246.174-10-mikuback@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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@linux.microsoft.com >> Sent: Thursday, November 10, 2022 1:33 AM >> To: devel@edk2.groups.io >> Cc: Erich McMillan ; Kinney, Michael D >> ; Michael Kubacki >> ; Ni, Ray ; Gao, Zhichao >> >> Subject: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables >> >> From: Michael Kubacki >> >> Fixes CodeQL alerts for CWE-457: >> https://cwe.mitre.org/data/definitions/457.html >> >> Cc: Erich McMillan >> Cc: Michael D Kinney >> Cc: Michael Kubacki >> Cc: Ray Ni >> Cc: Zhichao Gao >> Co-authored-by: Erich McMillan >> Signed-off-by: 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 > > > > >