From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout.perfora.net (mout.perfora.net [74.208.4.196]) by mx.groups.io with SMTP id smtpd.web11.11565.1680044814566962818 for ; Tue, 28 Mar 2023 16:06:54 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: smith-denny.com, ip: 74.208.4.196, mailfrom: osd@smith-denny.com) Received: from [10.137.194.171] ([131.107.8.107]) by mrelay.perfora.net (mreueus003 [74.208.5.2]) with ESMTPSA (Nemesis) id 0M7ZR3-1qcowD2Jq3-00xLF7; Wed, 29 Mar 2023 01:06:46 +0200 Message-ID: <4fe27a2b-e892-cc67-7ea7-762089641384@smith-denny.com> Date: Tue, 28 Mar 2023 16:06:45 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [edk2-devel] [PATCH v7 09/12] ShellPkg: Fix conditionally uninitialized variables To: Michael Kubacki , devel@edk2.groups.io Cc: Erich McMillan , Michael D Kinney , Ray Ni , Zhichao Gao References: <20230324223034.1560-1-mikuback@linux.microsoft.com> <20230324223034.1560-10-mikuback@linux.microsoft.com> <3f708733-a5dd-de89-acdc-d49b9d0eb6b0@smith-denny.com> <99becc09-aa31-aded-6d3e-b48d74b86c49@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: <99becc09-aa31-aded-6d3e-b48d74b86c49@linux.microsoft.com> X-Provags-ID: V03:K1:MoR8EG6jV4hO9vTH7usXKZtD/iuW+GPd1zSsLsg3WfDtW47hy5T 3S6yRCl71kvxqX59rNuhGfQNn9aXDYygD34fpH1X+AsgjDua+SoI5+zZ4pLdmx/ycjEwJy5 KY+AbPnV9zbR3leeJGNpTezrk6r3EtRMBrdZPkWCFG3CpSuLG6B4jTWKr69Iw19wMoJ+KnS Uvx6sMeQSChwAGi2KTtng== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:soxnlSSrOmw=;M6i6mPCHO7Js9rnS11gRlZZbtoO Co8YvdAco33Y4m7+u2BByI6mBNUv25el5V/bHaMcgKJSqp20PR1SQQjKXKtkwFiCb1WU6iL3k Y2j5Y1lpcfdKW22mokwzk7lL+5E6efSguj40M3sRAKL0YX1h0SEg62I8yBZOo7+JjY/xzjg/T lJnsonjp0pq5yzOdrp5O7KLbX2ioHuZY1CUbRWou3fClMVTpHdqVDwin9M5+y+kXvMI04e8RX nMc3MaNv8s3bAnXrFh1h6N3EDaEwyC9zRoUSxFnHNOIA79CixoEdcFkP5/mRQzfTSihcEtSlk hmOV6O5Sp+Ah4AFOSs4cNxa3t8nJIJAKpod8A6mqfpavCPuIsQtPpw+JG7UGA8lXMuXke+aBF +Dvbeds/62B1kt3jpx+gRNbPEb4jdvgMNSaQla0dzAKcNoJCR6XWqugjOZFPtYnPh4O4JUaxH qIAcHGUqjW2ECP5wNXiZkWCPuDjHvR7r5ysCsQM51JTRiGXMmzsfsZsNtwBuxmP+G6nWnz9N5 UJzsnt16VNYMsyuLNAtwWPYFriL1UO7JiLk8AB2srFO5DZ4SDXIJyp9l9WCSDAz+EMPzVQMDI z5Btnv1UXHBjTUE5AAtoZBgDXYG69Ewt9wTrI4gN+1CcUJxiZQnRIjuHFkPLYDM76LetzAtYK XpWX15H3FlXXX9RROoj26PpCCZfAG/6ofrK0iqt8Fw== Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 3/28/2023 12:24 PM, Michael Kubacki wrote: > 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 >>> >>> 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 >>> Reviewed-by: Zhichao Gao >>> --- >>>   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. Fair enough :) > > >>> -      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 ( >> >> >> >>