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
