Looks good.

Reviewed-by: Wu Jiaxin <jiaxin...@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

Reply via email to