* [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized
@ 2021-01-21 9:39 Zeng, Star
2021-01-21 13:47 ` [edk2-devel] " Laszlo Ersek
2021-01-21 14:38 ` Laszlo Ersek
0 siblings, 2 replies; 6+ messages in thread
From: Zeng, Star @ 2021-01-21 9:39 UTC (permalink / raw)
To: devel; +Cc: Star Zeng, Eric Dong, Ray Ni, Laszlo Ersek
NumberOfCpus local variable in GetAcpiCpuData will be uninitialized
when CpuS3DataDxe runs before DxeRegisterCpuFeaturesLib (linked by
CpuFeaturesDxe) because there is no code to initialize it at
(AcpiCpuData != NULL) execution path.
The issue is exposed after cefad282fb31aff3e1a6dcbd368cbbffc3fce900
and 38ee7bafa72f58982f99ac6f61eef160f80bad69.
There was negligence in that code review.
One further topic may be "Could EDK2 CI be enhanced to catch this kind
of uninitialized local variable case?". :)
This patch fixes this regression issue.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
.../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 7bb92404027f..60daa5cc87f0 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -957,6 +957,7 @@ GetAcpiCpuData (
//
// Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
//
+ NumberOfCpus = AcpiCpuData->NumberOfCpus;
TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
ASSERT (RegisterTable != NULL);
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized
[not found] <20210121093825.1571-1-star.zeng@intel.com>
@ 2021-01-21 9:47 ` Ni, Ray
0 siblings, 0 replies; 6+ messages in thread
From: Ni, Ray @ 2021-01-21 9:47 UTC (permalink / raw)
To: Zeng, Star; +Cc: devel@edk2.groups.io, Dong, Eric, Laszlo Ersek
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Thursday, January 21, 2021 5:38 PM
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be
> uninitialized
>
> NumberOfCpus local variable in GetAcpiCpuData will be uninitialized
> when CpuS3DataDxe runs before DxeRegisterCpuFeaturesLib (linked by
> CpuFeaturesDxe) because there is no code to initialize it at
> (AcpiCpuData != NULL) execution path.
>
> The issue is exposed after cefad282fb31aff3e1a6dcbd368cbbffc3fce900
> and 38ee7bafa72f58982f99ac6f61eef160f80bad69.
> There was negligence in that code review.
> One further topic may be "Could EDK2 CI be enhanced to catch this kind
> of uninitialized local variable case?". :)
>
> This patch fixes this regression issue.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> .../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 7bb92404027f..60daa5cc87f0 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -957,6 +957,7 @@ GetAcpiCpuData (
> //
> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for
> all CPUs
> //
> + NumberOfCpus = AcpiCpuData->NumberOfCpus;
> TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> ASSERT (RegisterTable != NULL);
> --
> 2.27.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized
2021-01-21 9:39 [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized Zeng, Star
@ 2021-01-21 13:47 ` Laszlo Ersek
2021-01-21 14:32 ` Zeng, Star
2021-01-21 14:38 ` Laszlo Ersek
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-01-21 13:47 UTC (permalink / raw)
To: devel, star.zeng; +Cc: Eric Dong, Ray Ni
On 01/21/21 10:39, Zeng, Star wrote:
> NumberOfCpus local variable in GetAcpiCpuData will be uninitialized
> when CpuS3DataDxe runs before DxeRegisterCpuFeaturesLib (linked by
> CpuFeaturesDxe) because there is no code to initialize it at
> (AcpiCpuData != NULL) execution path.
>
> The issue is exposed after cefad282fb31aff3e1a6dcbd368cbbffc3fce900
> and 38ee7bafa72f58982f99ac6f61eef160f80bad69.
Oops, sorry!
> There was negligence in that code review.
> One further topic may be "Could EDK2 CI be enhanced to catch this kind
> of uninitialized local variable case?". :)
Indeed I don't know why none of the toolchains caught this issue in CI.
:/ GCC is otherwise known for its "variable may be used without
initialization" warnings.
>
> This patch fixes this regression issue.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> .../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 7bb92404027f..60daa5cc87f0 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -957,6 +957,7 @@ GetAcpiCpuData (
> //
> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> //
> + NumberOfCpus = AcpiCpuData->NumberOfCpus;
> TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> ASSERT (RegisterTable != NULL);
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(Also, I think Ray's R-b from
<https://edk2.groups.io/g/devel/message/70615> applies here, so I'm
going to pick that up.)
Thanks & sorry again
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized
2021-01-21 13:47 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-21 14:32 ` Zeng, Star
0 siblings, 0 replies; 6+ messages in thread
From: Zeng, Star @ 2021-01-21 14:32 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray, Zeng, Star
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 21, 2021 9:48 PM
> To: devel@edk2.groups.io; Zeng, Star <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg RegisterCpuFeaturesLib:
> NumberOfCpus may be uninitialized
>
> On 01/21/21 10:39, Zeng, Star wrote:
> > NumberOfCpus local variable in GetAcpiCpuData will be uninitialized
> > when CpuS3DataDxe runs before DxeRegisterCpuFeaturesLib (linked by
> > CpuFeaturesDxe) because there is no code to initialize it at
> > (AcpiCpuData != NULL) execution path.
> >
> > The issue is exposed after cefad282fb31aff3e1a6dcbd368cbbffc3fce900
> > and 38ee7bafa72f58982f99ac6f61eef160f80bad69.
>
> Oops, sorry!
>
> > There was negligence in that code review.
> > One further topic may be "Could EDK2 CI be enhanced to catch this kind
> > of uninitialized local variable case?". :)
>
> Indeed I don't know why none of the toolchains caught this issue in CI.
> :/ GCC is otherwise known for its "variable may be used without
> initialization" warnings.
>
> >
> > This patch fixes this regression issue.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> > .../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index 7bb92404027f..60daa5cc87f0 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib
> > +++ .c
> > @@ -957,6 +957,7 @@ GetAcpiCpuData (
> > //
> > // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> > //
> > + NumberOfCpus = AcpiCpuData->NumberOfCpus;
> > TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> > ASSERT (RegisterTable != NULL);
> >
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> (Also, I think Ray's R-b from
> <https://edk2.groups.io/g/devel/message/70615> applies here, so I'm going
> to pick that up.)
Laszlo and Ray, thanks for the R-b.
Please help push it by pull request if no other feedback is received.
Thanks,
Star
>
> Thanks & sorry again
> Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized
2021-01-21 9:39 [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized Zeng, Star
2021-01-21 13:47 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-21 14:38 ` Laszlo Ersek
2021-01-21 14:44 ` Zeng, Star
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-01-21 14:38 UTC (permalink / raw)
To: devel, star.zeng; +Cc: Eric Dong, Ray Ni
On 01/21/21 10:39, Zeng, Star wrote:
> NumberOfCpus local variable in GetAcpiCpuData will be uninitialized
> when CpuS3DataDxe runs before DxeRegisterCpuFeaturesLib (linked by
> CpuFeaturesDxe) because there is no code to initialize it at
> (AcpiCpuData != NULL) execution path.
>
> The issue is exposed after cefad282fb31aff3e1a6dcbd368cbbffc3fce900
> and 38ee7bafa72f58982f99ac6f61eef160f80bad69.
> There was negligence in that code review.
> One further topic may be "Could EDK2 CI be enhanced to catch this kind
> of uninitialized local variable case?". :)
>
> This patch fixes this regression issue.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> .../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 7bb92404027f..60daa5cc87f0 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -957,6 +957,7 @@ GetAcpiCpuData (
> //
> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> //
> + NumberOfCpus = AcpiCpuData->NumberOfCpus;
> TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> ASSERT (RegisterTable != NULL);
>
Merged as commit 6c5801be6ef3, via
<https://github.com/tianocore/edk2/pull/1380>.
I've picked up Ray's R-b from the detached message
<https://edk2.groups.io/g/devel/message/70615>.
I've also updated the original ticket:
<https://bugzilla.tianocore.org/show_bug.cgi?id=3159>.
Thanks & sorry about the regression,
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized
2021-01-21 14:38 ` Laszlo Ersek
@ 2021-01-21 14:44 ` Zeng, Star
0 siblings, 0 replies; 6+ messages in thread
From: Zeng, Star @ 2021-01-21 14:44 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray, Zeng, Star
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 21, 2021 10:38 PM
> To: devel@edk2.groups.io; Zeng, Star <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg RegisterCpuFeaturesLib:
> NumberOfCpus may be uninitialized
>
> On 01/21/21 10:39, Zeng, Star wrote:
> > NumberOfCpus local variable in GetAcpiCpuData will be uninitialized
> > when CpuS3DataDxe runs before DxeRegisterCpuFeaturesLib (linked by
> > CpuFeaturesDxe) because there is no code to initialize it at
> > (AcpiCpuData != NULL) execution path.
> >
> > The issue is exposed after cefad282fb31aff3e1a6dcbd368cbbffc3fce900
> > and 38ee7bafa72f58982f99ac6f61eef160f80bad69.
> > There was negligence in that code review.
> > One further topic may be "Could EDK2 CI be enhanced to catch this kind
> > of uninitialized local variable case?". :)
> >
> > This patch fixes this regression issue.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> > .../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index 7bb92404027f..60daa5cc87f0 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib
> > +++ .c
> > @@ -957,6 +957,7 @@ GetAcpiCpuData (
> > //
> > // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> > //
> > + NumberOfCpus = AcpiCpuData->NumberOfCpus;
> > TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> > ASSERT (RegisterTable != NULL);
> >
>
> Merged as commit 6c5801be6ef3, via
> <https://github.com/tianocore/edk2/pull/1380>.
>
> I've picked up Ray's R-b from the detached message
> <https://edk2.groups.io/g/devel/message/70615>.
>
> I've also updated the original ticket:
> <https://bugzilla.tianocore.org/show_bug.cgi?id=3159>.
>
> Thanks & sorry about the regression,
> Laszlo
No worry, it was my negligence for the original code review series.
Fortunately, we catch it quickly. 😊
Star
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-21 14:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-21 9:39 [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: NumberOfCpus may be uninitialized Zeng, Star
2021-01-21 13:47 ` [edk2-devel] " Laszlo Ersek
2021-01-21 14:32 ` Zeng, Star
2021-01-21 14:38 ` Laszlo Ersek
2021-01-21 14:44 ` Zeng, Star
[not found] <20210121093825.1571-1-star.zeng@intel.com>
2021-01-21 9:47 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox