Re: [edk2-devel] [PATCH v7 09/12] ShellPkg: Fix conditionally uninitialized variables

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Hi Oliver,

Thanks for taking time to look through the series. I responded inline.

Thanks,
Michael

On 3/28/2023 2:49 PM, Oliver Smith-Denny wrote:
> A couple comments below, thanks!
> 
> On 3/24/2023 3:30 PM, Michael Kubacki wrote:
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
>> ---
>>   ShellPkg/Application/Shell/Shell.c                          |  1 +
>>   ShellPkg/Application/Shell/ShellProtocol.c                  | 60 
>> ++++++++++----------
>>   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, 107 insertions(+), 89 deletions(-)
>>
>> diff --git a/ShellPkg/Application/Shell/Shell.c 
>> b/ShellPkg/Application/Shell/Shell.c
>> index 0ae6e14a34bf..f95c799bb2a4 100644
>> --- a/ShellPkg/Application/Shell/Shell.c
>> +++ b/ShellPkg/Application/Shell/Shell.c
>> @@ -1300,6 +1300,7 @@ DoStartupScript (
>>     CHAR16         *FullFileStringPath;
>>     UINTN          NewSize;
>> +  CalleeStatus    = EFI_SUCCESS;
>>     Key.UnicodeChar = CHAR_NULL;
>>     Key.ScanCode    = 0;
>> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c 
>> b/ShellPkg/Application/Shell/ShellProtocol.c
>> index e6d20ab16479..da8c31cb038a 100644
>> --- a/ShellPkg/Application/Shell/ShellProtocol.c
>> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
>> @@ -735,50 +735,52 @@ 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);
>> -        for (HandleCount = 0; HandleCount < ParentDriverCount; 
>> HandleCount++) {
>> -          //
>> -          // try using that driver's component name with controller 
>> and our driver as the child.
>> -          //
>> -          Status = gBS->OpenProtocol (
>> -                          ParentDriverBuffer[HandleCount],
>> -                          &gEfiComponentName2ProtocolGuid,
>> -                          (VOID **)&CompName2,
>> -                          gImageHandle,
>> -                          NULL,
>> -                          EFI_OPEN_PROTOCOL_GET_PROTOCOL
>> -                          );
>> -          if (EFI_ERROR (Status)) {
>> +        Status = PARSE_HANDLE_DATABASE_UEFI_DRIVERS 
>> (ParentControllerBuffer[LoopVar], &ParentDriverCount, 
>> &ParentDriverBuffer);
>> +        if (!EFI_ERROR (Status)) {
>> +          for (HandleCount = 0; HandleCount < ParentDriverCount; 
>> HandleCount++) {
>> +            //
>> +            // try using that driver's component name with controller 
>> and our driver as the child.
>> +            //
>>               Status = gBS->OpenProtocol (
>>                               ParentDriverBuffer[HandleCount],
>> -                            &gEfiComponentNameProtocolGuid,
>> +                            &gEfiComponentName2ProtocolGuid,
>>                               (VOID **)&CompName2,
>>                               gImageHandle,
>>                               NULL,
>>                               EFI_OPEN_PROTOCOL_GET_PROTOCOL
>>                               );
>> -          }
>> +            if (EFI_ERROR (Status)) {
>> +              Status = gBS->OpenProtocol (
>> +                              ParentDriverBuffer[HandleCount],
>> +                              &gEfiComponentNameProtocolGuid,
>> +                              (VOID **)&CompName2,
>> +                              gImageHandle,
>> +                              NULL,
>> +                              EFI_OPEN_PROTOCOL_GET_PROTOCOL
>> +                              );
>> +            }
>> +
>> +            if (EFI_ERROR (Status)) {
>> +              continue;
>> +            }
>> -          if (EFI_ERROR (Status)) {
>> -            continue;
>> +            Lang   = GetBestLanguageForDriver 
>> (CompName2->SupportedLanguages, Language, FALSE);
>> +            Status = CompName2->GetControllerName (CompName2, 
>> ParentControllerBuffer[LoopVar], DeviceHandle, Lang, 
>> &DeviceNameToReturn);
>> +            FreePool (Lang);
>> +            Lang = NULL;
>> +            if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) {
>> +              break;
>> +            }
>>             }
>> -          Lang   = GetBestLanguageForDriver 
>> (CompName2->SupportedLanguages, Language, FALSE);
>> -          Status = CompName2->GetControllerName (CompName2, 
>> ParentControllerBuffer[LoopVar], DeviceHandle, Lang, 
>> &DeviceNameToReturn);
>> -          FreePool (Lang);
>> -          Lang = NULL;
>> +          SHELL_FREE_NON_NULL (ParentDriverBuffer);
>>             if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) {
>>               break;
>>             }
>>           }
>> -
>> -        SHELL_FREE_NON_NULL (ParentDriverBuffer);
>> -        if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) {
>> -          break;
>> -        }
>>         }
>>         SHELL_FREE_NON_NULL (ParentControllerBuffer);
>> 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);
> 
> Should we be checking for NewDefaultName to not be NULL before doing the 
> below function call? Looks like ShellCommandAddMapItemAndUpdatePath does 
> not do NULL checking and directly uses the Name (where it would fail in 
> StrSize()). Similarly we then call FreePool on it, even if it is NULL.
> 
Yes, but that's outside the scope of this patch. If someone were to fix 
that, they should also address the other ~20 places in the file that use 
asserts in place of checking null properly.


>> -      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)) {
> 
> nit: If we got an EFI_ERROR from FileHandleGetSize, we will return 
> EFI_OUT_OF_RESOURCES when we hit InBuffer == NULL below, which is not 
> the real reason we failed. Perhaps we don't care, though.
> 
True. The goal here was not to change that behavior.

>> +            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 (
> 
> 
> 
>