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

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

I spoke to Erich offline and he mentioned that a previous coding 
practice he used specified unsigned integer literals when intended so he 
applied that here.

Thanks,
Michael

On 3/10/2023 5:59 PM, Michael Kubacki wrote:
> Erich introduced this particular change, he may be able to provide more 
> info.
> 
> On 3/10/2023 3:03 PM, Michael D Kinney wrote:
>>
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>>> Michael Kubacki
>>> Sent: Friday, March 10, 2023 10:43 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 v4 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);
>>> +
>>> +  if (EFI_ERROR (Status)) {
>>> +    NumberOfProcessors        = 1u;
>>> +    NumberOfEnabledProcessors = 1u;
>>
>> Why is '1u' used instead of '1'?  I don't see a lot of usage of the 
>> postfix
>> unless the const needs to be forced to a larger bitwidth than default 
>> int type.
>>
>>> +  }
>>>
>>>     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);
>>> +
>>> +  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);
>>> +
>>> +  if (EFI_ERROR (Status)) {
>>> +    NumberOfProcessors = 1u;
>>> +  }
>>> +
>>>     MpInitLibWhoAmI (&Bsp);
>>>     for (Index = 0; Index < NumberOfProcessors; ++Index) {
>>>       StackBase = 0;
>>> -- 
>>> 2.39.2.windows.1
>>>
>>>
>>>
>>> -=-=-=-=-=-=
>>> Groups.io Links: You receive all messages sent to this group.
>>> View/Reply Online (#101030): 
>>> https://edk2.groups.io/g/devel/message/101030
>>> Mute This Topic: https://groups.io/mt/97526805/1643496
>>> Group Owner: devel+owner@edk2.groups.io
>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub 
>>> [michael.d.kinney@intel.com]
>>> -=-=-=-=-=-=
>>>
>>
>>
>>
>> 
>>
>>