[edk2-devel] [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
Michael Kubacki
mikuback at linux.microsoft.com
Thu Nov 24 02:36:37 UTC 2022
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 at linux.microsoft.com <mikuback at linux.microsoft.com>
>> Sent: Thursday, November 10, 2022 1:33 AM
>> To: devel at edk2.groups.io
>> Cc: Erich McMillan <emcmillan at microsoft.com>; Kinney, Michael D
>> <michael.d.kinney at intel.com>; Michael Kubacki
>> <mikuback at linux.microsoft.com>; Ni, Ray <ray.ni at intel.com>; Gao, Zhichao
>> <zhichao.gao at intel.com>
>> Subject: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
>>
>> From: Michael Kubacki <michael.kubacki at microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Cc: Erich McMillan <emcmillan at microsoft.com>
>> Cc: Michael D Kinney <michael.d.kinney at intel.com>
>> Cc: Michael Kubacki <mikuback at linux.microsoft.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Cc: Zhichao Gao <zhichao.gao at intel.com>
>> Co-authored-by: Erich McMillan <emcmillan at microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki at microsoft.com>
>> ---
>> 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list