Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables

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

The ASSERT() was added to aid debugging by bringing attention to the 
point where the fallback assignment occurs in the error status check block.

Are you suggesting the ASSERT() be removed because of a known case where 
it might trigger but the case is expected to return an error? Or, that 
is is not necessary in general?

Thanks,
Michael

On 11/23/2022 9:04 PM, Michael D Kinney wrote:
> Hi Michael,
> 
> comments below.
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Wednesday, November 9, 2022 9:33 AM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
>> Ni, Ray <ray.ni@intel.com>
>> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>   UefiCpuPkg/CpuMpPei/CpuBist.c   | 8 +++++++-
>>   UefiCpuPkg/CpuMpPei/CpuMpPei.c  | 8 +++++++-
>>   UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
>>   3 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
>> index 7dc93cd784d4..122808139b87 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
>> @@ -175,7 +175,13 @@ CollectBistDataFromPpi (
>>     EFI_SEC_PLATFORM_INFORMATION_RECORD2  *PlatformInformationRecord2;
>>     EFI_SEC_PLATFORM_INFORMATION_CPU      *CpuInstanceInHob;
>>
>> -  MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
>> +  Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
>> +  ASSERT_EFI_ERROR (Status);
> 
> I think this ASSERT() should be removed.  The added error status check looks correct.
> 
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    NumberOfProcessors        = 1u;
>> +    NumberOfEnabledProcessors = 1u;
>> +  }
>>
>>     BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
>>                           sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>> index e7f1fe9f426c..a84304273168 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
>>       return;
>>     }
>>
>> -  MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> +  Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> +  ASSERT_EFI_ERROR (Status);
> 
> I think this ASSERT() should be removed.  The added error status check looks correct.
> 
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    NumberOfProcessors = 1u;
>> +  }
>> +
>>     SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
>>     ASSERT (SwitchStackData != NULL);
>>     ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> index 135422225340..1322fcb77f28 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> @@ -538,6 +538,7 @@ SetupStackGuardPage (
>>     UINTN                 NumberOfProcessors;
>>     UINTN                 Bsp;
>>     UINTN                 Index;
>> +  EFI_STATUS            Status;
>>
>>     //
>>     // One extra page at the bottom of the stack is needed for Guard page.
>> @@ -547,7 +548,13 @@ SetupStackGuardPage (
>>       ASSERT (FALSE);
>>     }
>>
>> -  MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> +  Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> +  ASSERT_EFI_ERROR (Status);
> 
> I think this ASSERT() should be removed.  The added error status check looks correct.
> 
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    NumberOfProcessors = 1u;
>> +  }
>> +
>>     MpInitLibWhoAmI (&Bsp);
>>     for (Index = 0; Index < NumberOfProcessors; ++Index) {
>>       StackBase = 0;
>> --
>> 2.28.0.windows.1
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156
>> Mute This Topic: https://groups.io/mt/94918104/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 
>