Looks good.
Reviewed-by: Ye Ting <ting...@intel.com>
-----Original Message-----
From: Zhang, Lubo
Sent: Thursday, November 26, 2015 10:19 AM
To: Fu, Siyuan; edk2-devel@lists.01.org
Cc: Ye, Ting; Wu, Jiaxin; Qiu, Shumin
Subject: RE: [edk2] [PATCH v2] NetworkPkg:Fix NULL pointer dereference issues.
Yes, the HttpHeaders always not be a NULL pointer when a success call from
HttpTcpReceiveHeader function, I will modify when I commit.
Thanks
-----Original Message-----
From: Fu, Siyuan
Sent: Thursday, November 26, 2015 10:09 AM
To: Zhang, Lubo; edk2-devel@lists.01.org
Cc: Ye, Ting; Wu, Jiaxin; Qiu, Shumin
Subject: RE: [edk2] [PATCH v2] NetworkPkg:Fix NULL pointer dereference issues.
Hi, Lubo
Should HttpHeaders always not be a NULL pointer upon a success call of
HttpTcpReceiveHeader?
If yes, you should use ASSERT here, otherwise the patch is ok with me?
Status = HttpTcpReceiveHeader (HttpInstance, &SizeofHeaders, &BufferSize);
- if (EFI_ERROR (Status)) {
+ if (EFI_ERROR (Status) || HttpHeaders == NULL) {
goto Error;
}
Reviewed-by: Fu Siyuan <siyuan...@intel.com>
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang
Lubo
Sent: Thursday, November 26, 2015 10:01 AM
To: edk2-devel@lists.01.org
Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Wu, Jiaxin
<jiaxin...@intel.com>; Qiu, Shumin <shumin....@intel.com>
Subject: [edk2] [PATCH v2] NetworkPkg:Fix NULL pointer dereference issues.
v2:
*Revise some codes according to the comments.
1.In HttpResponseWorker, check the HttpHeaders in the first used place.
2.In EfiHttpPoll(), check the HttpInstance state outside of if condition.
Cc: Ye Ting <ting...@intel.com>
Cc: Fu Siyuan <siyuan...@intel.com>
Cc: Wu Jiaxin <jiaxin...@intel.com>
Cc: Qiu Shumin <shumin....@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo <lubo.zh...@intel.com>
---
NetworkPkg/HttpDxe/HttpImpl.c | 18 ++++++++++++---
NetworkPkg/HttpDxe/HttpImpl.h | 1 + NetworkPkg/HttpDxe/HttpProto.c | 52
+++++++++++++++++++-----------------------
3 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 4ad07d4..2cda5a7 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -38,10 +38,11 @@ EFI_HTTP_PROTOCOL mEfiHttpTemplate = {
@retval EFI_SUCCESS Operation succeeded.
@retval EFI_INVALID_PARAMETER One or more of the following conditions is
TRUE:
This is NULL.
HttpConfigData is NULL.
HttpConfigData->AccessPoint is NULL.
+ @retval EFI_OUT_OF_RESOURCES Could not allocate enough system resources.
@retval EFI_NOT_STARTED The HTTP instance is not configured.
**/
EFI_STATUS
EFIAPI
@@ -69,18 +70,24 @@ EfiHttpGetModeData (
HttpConfigData->TimeOutMillisec = HttpInstance->TimeOutMillisec;
HttpConfigData->LocalAddressIsIPv6 = HttpInstance->LocalAddressIsIPv6;
if (HttpInstance->LocalAddressIsIPv6) {
Http6AccessPoint = AllocateZeroPool (sizeof (EFI_HTTPv6_ACCESS_POINT));
+ if (Http6AccessPoint == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
CopyMem (
Http6AccessPoint,
&HttpInstance->Ipv6Node,
sizeof (HttpInstance->Ipv6Node)
);
HttpConfigData->AccessPoint.IPv6Node = Http6AccessPoint;
} else {
Http4AccessPoint = AllocateZeroPool (sizeof (EFI_HTTPv4_ACCESS_POINT));
+ if (Http4AccessPoint == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
CopyMem (
Http4AccessPoint,
&HttpInstance->IPv4Node,
sizeof (HttpInstance->IPv4Node)
);
@@ -879,11 +886,11 @@ HttpResponseWorker (
HttpInstance->EndofHeader = &EndofHeader;
HttpInstance->HttpHeaders = &HttpHeaders;
Status = HttpTcpReceiveHeader (HttpInstance, &SizeofHeaders, &BufferSize);
- if (EFI_ERROR (Status)) {
+ if (EFI_ERROR (Status) || HttpHeaders == NULL) {
goto Error;
}
//
// Cache the part of body.
@@ -1285,18 +1292,23 @@ EfiHttpPoll (
}
HttpInstance = HTTP_INSTANCE_FROM_PROTOCOL (This);
ASSERT (HttpInstance != NULL);
- if (HttpInstance->State != HTTP_STATE_TCP_CONNECTED || (HttpInstance->Tcp4
== NULL &&
- HttpInstance->Tcp6
== NULL)) {
+ if (HttpInstance->State != HTTP_STATE_TCP_CONNECTED) {
return EFI_NOT_STARTED;
}
if (HttpInstance->LocalAddressIsIPv6) {
+ if (HttpInstance->Tcp6 == NULL) {
+ return EFI_NOT_STARTED;
+ }
Status = HttpInstance->Tcp6->Poll (HttpInstance->Tcp6);
} else {
+ if (HttpInstance->Tcp4 == NULL) {
+ return EFI_NOT_STARTED;
+ }
Status = HttpInstance->Tcp4->Poll (HttpInstance->Tcp4);
}
DispatchDpc ();
diff --git a/NetworkPkg/HttpDxe/HttpImpl.h b/NetworkPkg/HttpDxe/HttpImpl.h
index 49c8af1..afbe982 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.h
+++ b/NetworkPkg/HttpDxe/HttpImpl.h
@@ -42,10 +42,11 @@
@retval EFI_SUCCESS Operation succeeded.
@retval EFI_INVALID_PARAMETER One or more of the following conditions is
TRUE:
This is NULL.
HttpConfigData is NULL.
HttpConfigData->AccessPoint is NULL.
+ @retval EFI_OUT_OF_RESOURCES Could not allocate enough system resources.
@retval EFI_NOT_STARTED The HTTP instance is not configured.
**/
EFI_STATUS
EFIAPI
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 9996951..85f8401 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -563,30 +563,27 @@ HttpCloseTcpRxEvent ( {
HTTP_PROTOCOL *HttpInstance;
EFI_TCP4_IO_TOKEN *Rx4Token;
EFI_TCP6_IO_TOKEN *Rx6Token;
+ ASSERT (Wrap != NULL);
HttpInstance = Wrap->HttpInstance;
Rx4Token = NULL;
Rx6Token = NULL;
if (HttpInstance->LocalAddressIsIPv6) {
- if (Wrap != NULL) {
- if (Wrap->TcpWrap.Rx6Token.CompletionToken.Event != NULL) {
- gBS->CloseEvent (Wrap->TcpWrap.Rx6Token.CompletionToken.Event);
- }
+ if (Wrap->TcpWrap.Rx6Token.CompletionToken.Event != NULL) {
+ gBS->CloseEvent (Wrap->TcpWrap.Rx6Token.CompletionToken.Event);
}
if (HttpInstance->Rx6Token.CompletionToken.Event != NULL) {
gBS->CloseEvent (HttpInstance->Rx6Token.CompletionToken.Event);
HttpInstance->Rx6Token.CompletionToken.Event = NULL;
}
} else {
- if (Wrap != NULL) {
- if (Wrap->TcpWrap.Rx4Token.CompletionToken.Event != NULL) {
- gBS->CloseEvent (Wrap->TcpWrap.Rx4Token.CompletionToken.Event);
- }
+ if (Wrap->TcpWrap.Rx4Token.CompletionToken.Event != NULL) {
+ gBS->CloseEvent (Wrap->TcpWrap.Rx4Token.CompletionToken.Event);
}
if (HttpInstance->Rx4Token.CompletionToken.Event != NULL) {
gBS->CloseEvent (HttpInstance->Rx4Token.CompletionToken.Event);
HttpInstance->Rx4Token.CompletionToken.Event = NULL; @@ -1889,27
+1886,26 @@ HttpTcpTokenCleanup ( {
HTTP_PROTOCOL *HttpInstance;
EFI_TCP4_IO_TOKEN *Rx4Token;
EFI_TCP6_IO_TOKEN *Rx6Token;
+ ASSERT (Wrap != NULL);
HttpInstance = Wrap->HttpInstance;
Rx4Token = NULL;
Rx6Token = NULL;
if (HttpInstance->LocalAddressIsIPv6) {
- if (Wrap != NULL) {
- if (Wrap->TcpWrap.Rx6Token.CompletionToken.Event != NULL) {
- gBS->CloseEvent (Wrap->TcpWrap.Rx6Token.CompletionToken.Event);
- }
+ if (Wrap->TcpWrap.Rx6Token.CompletionToken.Event != NULL) {
+ gBS->CloseEvent (Wrap->TcpWrap.Rx6Token.CompletionToken.Event);
+ }
- Rx6Token = &Wrap->TcpWrap.Rx6Token;
- if (Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer != NULL) {
- FreePool (Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer);
- Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer = NULL;
- }
- FreePool (Wrap);
+ Rx6Token = &Wrap->TcpWrap.Rx6Token;
+ if (Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer != NULL) {
+ FreePool (Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer);
+ Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer = NULL;
}
+ FreePool (Wrap);
if (HttpInstance->Rx6Token.CompletionToken.Event != NULL) {
gBS->CloseEvent (HttpInstance->Rx6Token.CompletionToken.Event);
HttpInstance->Rx6Token.CompletionToken.Event = NULL;
}
@@ -1919,22 +1915,20 @@ HttpTcpTokenCleanup (
FreePool (Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer);
Rx6Token->Packet.RxData->FragmentTable[0].FragmentBuffer = NULL;
}
} else {
- if (Wrap != NULL) {
- if (Wrap->TcpWrap.Rx4Token.CompletionToken.Event != NULL) {
- gBS->CloseEvent (Wrap->TcpWrap.Rx4Token.CompletionToken.Event);
- }
- Rx4Token = &Wrap->TcpWrap.Rx4Token;
- if (Rx4Token->Packet.RxData->FragmentTable[0].FragmentBuffer != NULL) {
- FreePool (Rx4Token->Packet.RxData->FragmentTable[0].FragmentBuffer);
- Rx4Token->Packet.RxData->FragmentTable[0].FragmentBuffer = NULL;
- }
- FreePool (Wrap);
+ if (Wrap->TcpWrap.Rx4Token.CompletionToken.Event != NULL) {
+ gBS->CloseEvent (Wrap->TcpWrap.Rx4Token.CompletionToken.Event);
}
-
+ Rx4Token = &Wrap->TcpWrap.Rx4Token;
+ if (Rx4Token->Packet.RxData->FragmentTable[0].FragmentBuffer != NULL) {
+ FreePool (Rx4Token->Packet.RxData->FragmentTable[0].FragmentBuffer);
+ Rx4Token->Packet.RxData->FragmentTable[0].FragmentBuffer = NULL;
+ }
+ FreePool (Wrap);
+
if (HttpInstance->Rx4Token.CompletionToken.Event != NULL) {
gBS->CloseEvent (HttpInstance->Rx4Token.CompletionToken.Event);
HttpInstance->Rx4Token.CompletionToken.Event = NULL;
}
--
1.9.5.msysgit.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
