[v4] Enable New CodeQL Queries

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

Posted by Michael Kubacki 2 years, 11 months ago

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;
+  }
 
   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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Michael D Kinney 2 years, 11 months ago


> -----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]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101035): https://edk2.groups.io/g/devel/message/101035
Mute This Topic: https://groups.io/mt/97526805/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Michael Kubacki 2 years, 11 months ago

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]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101038): https://edk2.groups.io/g/devel/message/101038
Mute This Topic: https://groups.io/mt/97526805/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Michael Kubacki 2 years, 10 months ago

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]
>>> -=-=-=-=-=-=
>>>
>>
>>
>>
>> 
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101493): https://edk2.groups.io/g/devel/message/101493
Mute This Topic: https://groups.io/mt/97526805/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Michael Kubacki 2 years, 10 months ago

Hi Mike,

Would you like a change here?

Thanks,
Michael

On 3/21/2023 10:25 AM, Michael Kubacki wrote:
> 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]
>>>> -=-=-=-=-=-=
>>>>
>>>
>>>
>>>
>>> 
>>>
>>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101798): https://edk2.groups.io/g/devel/message/101798
Mute This Topic: https://groups.io/mt/97526805/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Michael D Kinney 2 years, 10 months ago

I would prefer to follow convention of rest of edk2 sources.
which is to not use the postfix 'u' for small integers.

Mike

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Friday, March 24, 2023 8:51 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Erich McMillan <emcmillan@microsoft.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v4 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
> 
> Hi Mike,
> 
> Would you like a change here?
> 
> Thanks,
> Michael
> 
> On 3/21/2023 10:25 AM, Michael Kubacki wrote:
> > 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]
> >>>> -=-=-=-=-=-=
> >>>>
> >>>
> >>>
> >>>
> >>> 
> >>>
> >>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101799): https://edk2.groups.io/g/devel/message/101799
Mute This Topic: https://groups.io/mt/97526805/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-