* [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
@ 2019-09-12 2:54 Zhang, Shenglei
2019-09-12 18:54 ` Nate DeSimone
[not found] ` <15C3C5B9A4267C61.28831@groups.io>
0 siblings, 2 replies; 5+ messages in thread
From: Zhang, Shenglei @ 2019-09-12 2:54 UTC (permalink / raw)
To: devel; +Cc: Michael Kubacki, Chasel Chiu, Nate DeSimone, Liming Gao
In DxeCheckBootVariable.c, add check for BootOrder and Variable
that return EFI_NOT_FOUND when they are NULL.
In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL
when allocating memory to what it points to.
Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
v2: Update copyright
.../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c | 8 +++++++-
.../Test/Library/TestPointCheckLib/DxeCheckGcd.c | 6 ++++--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
index 85bd5b3d..98130683 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
@@ -1,6 +1,6 @@
/** @file
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
for (ListIndex = 0; ListIndex < sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]); ListIndex++) {
UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder", mLoadOptionVariableList[ListIndex]);
Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid, (VOID **)&BootOrder, &OrderSize);
+ if(BootOrder == NULL) {
+ return EFI_NOT_FOUND;;
+ }
if (EFI_ERROR(Status)) {
continue;
}
@@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
for (Index = 0; ; Index++) {
UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x", mKeyOptionVariableList[ListIndex], Index);
Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid, &Variable, &Size);
+ if(Variable == NULL) {
+ return EFI_NOT_FOUND;;
+ }
if (!EFI_ERROR(Status)) {
DumpKeyOption (KeyOptionName, Variable, Size);
} else {
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
index 82709d44..c90b37f2 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
@@ -1,6 +1,6 @@
/** @file
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -241,7 +241,9 @@ TestPointDumpGcd (
}
}
if (GcdMemoryMap != NULL) {
- *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+ if (GcdIoMap != NULL){
+ *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+ }
*GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
}
}
--
2.18.0.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
2019-09-12 2:54 [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers Zhang, Shenglei
@ 2019-09-12 18:54 ` Nate DeSimone
[not found] ` <15C3C5B9A4267C61.28831@groups.io>
1 sibling, 0 replies; 5+ messages in thread
From: Nate DeSimone @ 2019-09-12 18:54 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Chiu, Chasel, Gao, Liming
Your whitespace doesn't quite match the edk2 coding style guidelines, but we can fix that during commit.
Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
-----Original Message-----
From: Zhang, Shenglei
Sent: Wednesday, September 11, 2019 7:55 PM
To: devel@edk2.groups.io
Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
In DxeCheckBootVariable.c, add check for BootOrder and Variable that return EFI_NOT_FOUND when they are NULL.
In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL when allocating memory to what it points to.
Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
v2: Update copyright
.../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c | 8 +++++++-
.../Test/Library/TestPointCheckLib/DxeCheckGcd.c | 6 ++++--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
index 85bd5b3d..98130683 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
+++ eckBootVariable.c
@@ -1,6 +1,6 @@
/** @file
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
for (ListIndex = 0; ListIndex < sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]); ListIndex++) {
UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder", mLoadOptionVariableList[ListIndex]);
Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid, (VOID **)&BootOrder, &OrderSize);
+ if(BootOrder == NULL) {
+ return EFI_NOT_FOUND;;
+ }
if (EFI_ERROR(Status)) {
continue;
}
@@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
for (Index = 0; ; Index++) {
UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x", mKeyOptionVariableList[ListIndex], Index);
Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid, &Variable, &Size);
+ if(Variable == NULL) {
+ return EFI_NOT_FOUND;;
+ }
if (!EFI_ERROR(Status)) {
DumpKeyOption (KeyOptionName, Variable, Size);
} else {
diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
index 82709d44..c90b37f2 100644
--- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
+++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
+++ eckGcd.c
@@ -1,6 +1,6 @@
/** @file
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -241,7 +241,9 @@ TestPointDumpGcd (
}
}
if (GcdMemoryMap != NULL) {
- *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+ if (GcdIoMap != NULL){
+ *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
+ }
*GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
}
}
--
2.18.0.windows.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
[not found] ` <15C3C5B9A4267C61.28831@groups.io>
@ 2019-09-13 22:31 ` Nate DeSimone
2019-09-17 3:36 ` Zhang, Shenglei
0 siblings, 1 reply; 5+ messages in thread
From: Nate DeSimone @ 2019-09-13 22:31 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Chiu, Chasel, Gao, Liming
Hi Shenglei,
Looking at this patch more closely. There appear to be bugs... please
see below. Please fix this along with your poor use of semi-colons and
white-space.
Thanks,
Nate
On 9/12/2019 11:54 AM, Nate DeSimone wrote:
> Your whitespace doesn't quite match the edk2 coding style guidelines, but we can fix that during commit.
>
> Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
>
> -----Original Message-----
> From: Zhang, Shenglei
> Sent: Wednesday, September 11, 2019 7:55 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
>
> In DxeCheckBootVariable.c, add check for BootOrder and Variable that return EFI_NOT_FOUND when they are NULL.
> In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL when allocating memory to what it points to.
>
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>
> v2: Update copyright
>
> .../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c | 8 +++++++-
> .../Test/Library/TestPointCheckLib/DxeCheckGcd.c | 6 ++++--
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
> index 85bd5b3d..98130683 100644
> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckBootVariable.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> +++ eckBootVariable.c
> @@ -1,6 +1,6 @@
> /** @file
>
> -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
> for (ListIndex = 0; ListIndex < sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]); ListIndex++) {
> UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder", mLoadOptionVariableList[ListIndex]);
> Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid, (VOID **)&BootOrder, &OrderSize);
> + if(BootOrder == NULL) {
> + return EFI_NOT_FOUND;;
> + }
> if (EFI_ERROR(Status)) {
> continue;
> }
> @@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
> for (Index = 0; ; Index++) {
> UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x", mKeyOptionVariableList[ListIndex], Index);
> Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid, &Variable, &Size);
> + if(Variable == NULL) {
> + return EFI_NOT_FOUND;;
> + }
> if (!EFI_ERROR(Status)) {
> DumpKeyOption (KeyOptionName, Variable, Size);
> } else {
> diff --git a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
> index 82709d44..c90b37f2 100644
> --- a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheckGcd.c
> +++ b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> +++ eckGcd.c
> @@ -1,6 +1,6 @@
> /** @file
>
> -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -241,7 +241,9 @@ TestPointDumpGcd (
> }
> }
> if (GcdMemoryMap != NULL) {
> - *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> + if (GcdIoMap != NULL){
> + *GcdIoMap = AllocateCopyPool (NumberOfDescriptors * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> + }
GcdIoMap will always be NULL. Please see line 199 of this file. I
believe your patch is introducing a new bug.
> *GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
> }
> }
> --
> 2.18.0.windows.1
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
2019-09-13 22:31 ` [edk2-devel] " Nate DeSimone
@ 2019-09-17 3:36 ` Zhang, Shenglei
2019-09-17 21:42 ` Nate DeSimone
0 siblings, 1 reply; 5+ messages in thread
From: Zhang, Shenglei @ 2019-09-17 3:36 UTC (permalink / raw)
To: devel@edk2.groups.io, Desimone, Nathaniel L
Cc: Kubacki, Michael A, Chiu, Chasel, Gao, Liming
Hi Nathaniel,
> -----Original Message-----
> From: Desimone, Nathaniel L
> Sent: Saturday, September 14, 2019 6:31 AM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MinPlatformPkg/TestPointCheckLib: Add
> check for pointers
>
> Hi Shenglei,
>
> Looking at this patch more closely. There appear to be bugs... please
> see below. Please fix this along with your poor use of semi-colons and
> white-space.
>
> Thanks,
>
> Nate
>
> On 9/12/2019 11:54 AM, Nate DeSimone wrote:
> > Your whitespace doesn't quite match the edk2 coding style guidelines, but
> we can fix that during commit.
> >
> > Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> >
> > -----Original Message-----
> > From: Zhang, Shenglei
> > Sent: Wednesday, September 11, 2019 7:55 PM
> > To: devel@edk2.groups.io
> > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for
> pointers
> >
> > In DxeCheckBootVariable.c, add check for BootOrder and Variable that
> return EFI_NOT_FOUND when they are NULL.
> > In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL when
> allocating memory to what it points to.
> >
> > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> >
> > v2: Update copyright
> >
> > .../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c | 8 +++++++-
> > .../Test/Library/TestPointCheckLib/DxeCheckGcd.c | 6 ++++--
> > 2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheck
> BootVariable.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheck
> BootVariable.c
> > index 85bd5b3d..98130683 100644
> > ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheck
> BootVariable.c
> > +++
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> > +++ eckBootVariable.c
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
> > for (ListIndex = 0; ListIndex <
> sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]);
> ListIndex++) {
> > UnicodeSPrint (BootOrderName, sizeof(BootOrderName), L"%sOrder",
> mLoadOptionVariableList[ListIndex]);
> > Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid,
> (VOID **)&BootOrder, &OrderSize);
> > + if(BootOrder == NULL) {
> > + return EFI_NOT_FOUND;;
> > + }
> > if (EFI_ERROR(Status)) {
> > continue;
> > }
> > @@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
> > for (Index = 0; ; Index++) {
> > UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName), L"%s%04x",
> mKeyOptionVariableList[ListIndex], Index);
> > Status = GetVariable2 (KeyOptionName, &gEfiGlobalVariableGuid,
> &Variable, &Size);
> > + if(Variable == NULL) {
> > + return EFI_NOT_FOUND;;
> > + }
> > if (!EFI_ERROR(Status)) {
> > DumpKeyOption (KeyOptionName, Variable, Size);
> > } else {
> > diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheck
> Gcd.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheck
> Gcd.c
> > index 82709d44..c90b37f2 100644
> > ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCheck
> Gcd.c
> > +++
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> > +++ eckGcd.c
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2017-2019, Intel Corporation. All rights reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -241,7 +241,9 @@ TestPointDumpGcd (
> > }
> > }
> > if (GcdMemoryMap != NULL) {
> > - *GcdIoMap = AllocateCopyPool (NumberOfDescriptors *
> sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> > + if (GcdIoMap != NULL){
> > + *GcdIoMap = AllocateCopyPool (NumberOfDescriptors *
> sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> > + }
>
> GcdIoMap will always be NULL. Please see line 199 of this file. I
> believe your patch is introducing a new bug.
I fail to get your point. Would you explain why GcdIoMap will always be NULL? It looks like an input
parameter from other place.
Thanks,
Shenglei
>
> > *GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
> > }
> > }
> > --
> > 2.18.0.windows.1
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
2019-09-17 3:36 ` Zhang, Shenglei
@ 2019-09-17 21:42 ` Nate DeSimone
0 siblings, 0 replies; 5+ messages in thread
From: Nate DeSimone @ 2019-09-17 21:42 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io
Cc: Kubacki, Michael A, Chiu, Chasel, Gao, Liming
Hi Shenglei,
You are right, *GcdIoMap will always be NULL but GcdIoMap may not be as the function does not exit early in the case of GcdIoMap being == NULL, sorry I misread. Please make sure you put a space between your closing parenthesis and your opening curly:
if (GcdIoMap != NULL) {
not...
if (GcdIoMap != NULL){
Also, please fix the double semicolon and if statement whitespace:
If (BootOrder == NULL) {
return EFI_NOT_FOUND;
}
not...
if(BootOrder == NULL) {
return EFI_NOT_FOUND;;
}
Then you will get a reviewed-by.
Thanks,
Nate
-----Original Message-----
From: Zhang, Shenglei <shenglei.zhang@intel.com>
Sent: Monday, September 16, 2019 8:36 PM
To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers
Hi Nathaniel,
> -----Original Message-----
> From: Desimone, Nathaniel L
> Sent: Saturday, September 14, 2019 6:31 AM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MinPlatformPkg/TestPointCheckLib:
> Add check for pointers
>
> Hi Shenglei,
>
> Looking at this patch more closely. There appear to be bugs... please
> see below. Please fix this along with your poor use of semi-colons and
> white-space.
>
> Thanks,
>
> Nate
>
> On 9/12/2019 11:54 AM, Nate DeSimone wrote:
> > Your whitespace doesn't quite match the edk2 coding style
> > guidelines, but
> we can fix that during commit.
> >
> > Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> >
> > -----Original Message-----
> > From: Zhang, Shenglei
> > Sent: Wednesday, September 11, 2019 7:55 PM
> > To: devel@edk2.groups.io
> > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for
> pointers
> >
> > In DxeCheckBootVariable.c, add check for BootOrder and Variable that
> return EFI_NOT_FOUND when they are NULL.
> > In DxeCheckGcd.c, add check for GcdIoMap to ensure it not NULL when
> allocating memory to what it points to.
> >
> > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> >
> > v2: Update copyright
> >
> > .../Test/Library/TestPointCheckLib/DxeCheckBootVariable.c | 8 +++++++-
> > .../Test/Library/TestPointCheckLib/DxeCheckGcd.c | 6 ++++--
> > 2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec
> k
> BootVariable.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec
> k
> BootVariable.c
> > index 85bd5b3d..98130683 100644
> > ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec
> k
> BootVariable.c
> > +++
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> > +++ eckBootVariable.c
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2017-2019, Intel Corporation. All rights
> > +reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -130,6 +130,9 @@ TestPointCheckLoadOptionVariable (
> > for (ListIndex = 0; ListIndex <
> sizeof(mLoadOptionVariableList)/sizeof(mLoadOptionVariableList[0]);
> ListIndex++) {
> > UnicodeSPrint (BootOrderName, sizeof(BootOrderName),
> > L"%sOrder",
> mLoadOptionVariableList[ListIndex]);
> > Status = GetVariable2 (BootOrderName, &gEfiGlobalVariableGuid,
> (VOID **)&BootOrder, &OrderSize);
> > + if(BootOrder == NULL) {
> > + return EFI_NOT_FOUND;;
> > + }
> > if (EFI_ERROR(Status)) {
> > continue;
> > }
> > @@ -222,6 +225,9 @@ TestPointCheckKeyOptionVariable (
> > for (Index = 0; ; Index++) {
> > UnicodeSPrint (KeyOptionName, sizeof(KeyOptionName),
> > L"%s%04x",
> mKeyOptionVariableList[ListIndex], Index);
> > Status = GetVariable2 (KeyOptionName,
> > &gEfiGlobalVariableGuid,
> &Variable, &Size);
> > + if(Variable == NULL) {
> > + return EFI_NOT_FOUND;;
> > + }
> > if (!EFI_ERROR(Status)) {
> > DumpKeyOption (KeyOptionName, Variable, Size);
> > } else {
> > diff --git
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec
> k
> Gcd.c
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec
> k
> Gcd.c
> > index 82709d44..c90b37f2 100644
> > ---
> a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec
> k
> Gcd.c
> > +++
> b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh
> > +++ eckGcd.c
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2017-2019, Intel Corporation. All rights
> > +reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -241,7 +241,9 @@ TestPointDumpGcd (
> > }
> > }
> > if (GcdMemoryMap != NULL) {
> > - *GcdIoMap = AllocateCopyPool (NumberOfDescriptors *
> sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> > + if (GcdIoMap != NULL){
> > + *GcdIoMap = AllocateCopyPool (NumberOfDescriptors *
> sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR), IoMap);
> > + }
>
> GcdIoMap will always be NULL. Please see line 199 of this file. I
> believe your patch is introducing a new bug.
I fail to get your point. Would you explain why GcdIoMap will always be NULL? It looks like an input parameter from other place.
Thanks,
Shenglei
>
> > *GcdIoMapNumberOfDescriptors = NumberOfDescriptors;
> > }
> > }
> > --
> > 2.18.0.windows.1
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-17 21:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-12 2:54 [PATCH] MinPlatformPkg/TestPointCheckLib: Add check for pointers Zhang, Shenglei
2019-09-12 18:54 ` Nate DeSimone
[not found] ` <15C3C5B9A4267C61.28831@groups.io>
2019-09-13 22:31 ` [edk2-devel] " Nate DeSimone
2019-09-17 3:36 ` Zhang, Shenglei
2019-09-17 21:42 ` Nate DeSimone
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox