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);
}
...
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
