[edk2-devel] [PATCH v7 09/12] ShellPkg: Fix conditionally uninitialized variables
Oliver Smith-Denny
osd at smith-denny.com
Tue Mar 28 18:49:23 UTC 2023
A couple comments below, thanks!
On 3/24/2023 3:30 PM, Michael Kubacki wrote:
> 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>
> Reviewed-by: Zhichao Gao <zhichao.gao at 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.
> - 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.
> + 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 (
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102062): https://edk2.groups.io/g/devel/message/102062
Mute This Topic: https://groups.io/mt/97834589/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