Hi Nathaniel, > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Saturday, September 14, 2019 6:31 AM > To: Zhang, Shenglei <shenglei.zh...@intel.com>; devel@edk2.groups.io > Cc: Kubacki, Michael A <michael.a.kuba...@intel.com>; Chiu, Chasel > <chasel.c...@intel.com>; Gao, Liming <liming....@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.desim...@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.kuba...@intel.com>; Chiu, Chasel > <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; Gao, Liming <liming....@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.kuba...@intel.com> > > Cc: Chasel Chiu <chasel.c...@intel.com> > > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Signed-off-by: Shenglei Zhang <shenglei.zh...@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 > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47336): https://edk2.groups.io/g/devel/message/47336 Mute This Topic: https://groups.io/mt/33110621/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-