Hi Leif, I think you are raising a general question "can we trust the consumed API already return defined value", not specific to this patch. I don't have an answer to this. And I also would like to hear more opinions.
For this patch: I would accept the change made to core services such as LocateHandleBuffer/HandleProtocol, as I think these are trusted APIs; For others such as calling Aip->GetSupportedTypes/ GetInformation, I do prefer to keep original error handling since the AIP is provided by 3rd party drivers and some of them might produce a broken AIP. Adding assert() to check such returned parameter might not function when the system is burned with a release built tip. Thanks, Ting -----Original Message----- From: Wu, Jiaxin Sent: Monday, January 04, 2016 9:50 AM To: Leif Lindholm Cc: Ye, Ting; [email protected]; Fu, Siyuan Subject: RE: [edk2] [Patch] NetworkPkg: Fix suspicious dereference of pointer before NULL check Leif, As you mentioned you prefer clarifications instead of any testes (AipHandleCount and AipHandleBuffer) checking, I don't have strong opinion for adding more tests checking for API or adding the clarifications on the guaranteed behavior of the code. I think both of them good to me. If you want to create another patch for the clarifications, please also remove the untrusted checks for LocateHandleBuffer() thoroughly. Thanks. Jiaxin -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Leif Lindholm Sent: Thursday, December 31, 2015 7:00 PM To: Wu, Jiaxin Cc: Ye, Ting; [email protected]; Fu, Siyuan Subject: Re: [edk2] [Patch] NetworkPkg: Fix suspicious dereference of pointer before NULL check Hi Jiaxin, On Mon, Dec 28, 2015 at 05:41:56AM +0000, Wu, Jiaxin wrote: > Hi Lindholm, > Sorry that misunderstand you, this patch just make the code more > readable. We know it's impossible return NULL pointer after > LocateHandleBuffer returns EFI_SUCCESS according UEFI Spec or any code > logic, but if anyone doesn't have those knowledge(or unaware of > checking UEFI Spec/any code logic), he maybe thought AipHandleBuffer > is suspicious dereference of pointer especially with below piece > code: > > ... > Exit: > ... > if (AipHandleBuffer != NULL) { > FreePool (AipHandleBuffer); > } > ... Thank you. Ok, this makes me understand why the change was made, and would have been good to have covered in the commit message. I realise there is a bit of a language issue here as well. With your explanation, I can see what the commit message is intended to mean. But just reading the message on its own does not convey this meaning to me. If the message had contained more information, this would have been less of a problem. And I am not saying that the change is bad, but I will make an observation on that piece of code in general: It looks like it does not trust the APIs it is using. This applies both before and after this specific commit. We start out with: --- if (EFI_ERROR (Status) || AipHandleCount == 0) { return EFI_NOT_FOUND; } --- But we already know that LocateHandleBuffer returns EFI_NOT_FOUND if no handles were found. So the added test for AipHandleCount adds no added confidence to the program. Similarily, we know that if LocateHandleBuffer returns EFI_SUCCESS, AipHandleBuffer contains a valid pointer. Thus, both of these tests (AipHandleCount and AipHandleBuffer) are checking for API compliance rather than execution time success. This makes sense in a test suite, but not in core runtime code. I do agree with you that this behaviour is not obvious unless I read it with the UEFI specification next to me, but I would very much prefer clarifications on the guaranteed behaviour of the code to be made in the form of comments. So the way I would have written this code would have been: --- Status = gBS->LocateHandleBuffer ( ByProtocol, &gEfiAdapterInformationProtocolGuid, NULL, &AipHandleCount, &AipHandleBuffer ); if (EFI_ERROR(Status)) { return EFI_NOT_FOUND; } // Guaranteed that AipHandleCount > 0 and AipHandleBuffer != NULL --- Now, the compiler also does not know about the UEFI specification - and especially more recent GCC versions frequently complain in instances like this. That was why I was wondering if you were working around a compiler problem. The explicit initializations --- AipHandleCount = 0; AipHandleBuffer = NULL; --- resolve this problem. Ting: would you accept a patch that did the above - replacing the additional test and the ASSERT() with a comment? Regards, Leif > Thanks. > Jiaxin > > -----Original Message----- > From: Leif Lindholm [mailto:[email protected]] > Sent: Monday, December 28, 2015 12:48 AM > To: Wu, Jiaxin > Cc: [email protected]; Ye, Ting; Fu, Siyuan > Subject: Re: [edk2] [Patch] NetworkPkg: Fix suspicious dereference of > pointer before NULL check > > Hi Jiaxin, > > On Thu, Dec 24, 2015 at 02:16:53PM +0800, Jiaxin Wu wrote: > > This patch is used to fix suspicious dereference of pointer before > > NULL check in IScsiDxe driver. > > So, I realise this has already been committed, and I don't see anything wrong > with the code - but the commit message does not appear to be describing what > is actually being done. > > > Cc: Ye Ting <[email protected]> > > Cc: Fu Siyuan <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Jiaxin Wu <[email protected]> > > --- > > NetworkPkg/IScsiDxe/IScsiDriver.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/NetworkPkg/IScsiDxe/IScsiDriver.c > > b/NetworkPkg/IScsiDxe/IScsiDriver.c > > index a7031df..51ce169 100644 > > --- a/NetworkPkg/IScsiDxe/IScsiDriver.c > > +++ b/NetworkPkg/IScsiDxe/IScsiDriver.c > > @@ -103,11 +103,12 @@ IScsiCheckAip ( > > UINT8 NetworkBootPolicy; > > > > // > > // Check any AIP instances exist in system. > > // > > - AipHandleCount = 0; > > + AipHandleCount = 0; > > + AipHandleBuffer = NULL; > > Status = gBS->LocateHandleBuffer ( > > ByProtocol, > > &gEfiAdapterInformationProtocolGuid, > > NULL, > > &AipHandleCount, > > @@ -115,10 +116,12 @@ IScsiCheckAip ( > > ); > > if (EFI_ERROR (Status) || AipHandleCount == 0) { > > return EFI_NOT_FOUND; > > } > > > > + ASSERT (AipHandleBuffer != NULL); > > + > > The change is simply an ASSERT checking whether AipHandleBuffer contains a > valid pointer after successfully returning a non-empty set. > But surely this is a requirement if LocateHandleBuffer returns EFI_SUCCESS? > > So what error condition is this change resolving? > Is this to deal with a compiler warning? > > > InfoBlock = NULL; > > > > for (AipIndex = 0; AipIndex < AipHandleCount; AipIndex++) { > > Status = gBS->HandleProtocol ( > > AipHandleBuffer[AipIndex], > > -- > > 1.9.5.msysgit.1 > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
