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]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.