Re: [staging/HTTPS-TLS][PATCH 1/2] CryptoPkg/Library/TlsLib: Refine the coding style

Re: [staging/HTTPS-TLS][PATCH 1/2] CryptoPkg/Library/TlsLib: Refine the coding style

[Palmer, Thomas]

Reviewed by Thomas Palmer <thomas.palmer@hpe.com>

-----Original Message-----
From: Jiaxin Wu [mailto:jiaxin.wu@intel.com] 
Sent: Monday, July 11, 2016 12:43 AM
To: edk2-devel@lists.01.org
Cc: Palmer, Thomas <thomas.palmer@hpe.com>; Samer El-Haj-Mahmoud <Smahmoud@lenovo.com>; Long Qin <qin.long@intel.com>; Ye Ting <ting.ye@intel.com>
Subject: [staging/HTTPS-TLS][PATCH 1/2] CryptoPkg/Library/TlsLib: Refine the coding style

Cc: Palmer Thomas <thomas.palmer@hpe.com>
Cc: Samer El-Haj-Mahmoud <Smahmoud@lenovo.com>
Cc: Long Qin <qin.long@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 CryptoPkg/Library/TlsLib/TlsLib.c | 52 +++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.c b/CryptoPkg/Library/TlsLib/TlsLib.c
index 814c358..f007882 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.c
+++ b/CryptoPkg/Library/TlsLib/TlsLib.c
@@ -614,65 +614,75 @@ TlsDoHandshake (
   IN OUT UINTN                    *BufferOutSize
   )
 {
   TLS_CONNECTION  *TlsConn;
   UINTN           PendingBufferSize;
-  int             ret;
-  unsigned long   e;
+  INTN            Ret;
+  unsigned long   ErrorCode;
 
   TlsConn           = (TLS_CONNECTION *) Tls;
   PendingBufferSize = 0;
+  Ret               = 1;
 
   if (TlsConn == NULL || \
     TlsConn->Ssl == NULL || TlsConn->InBio == NULL || TlsConn->OutBio == NULL || \
     BufferOutSize == NULL || \
     (BufferIn == NULL && BufferInSize != 0) || \
     (BufferIn != NULL && BufferInSize == 0) || \
     (BufferOut == NULL && *BufferOutSize != 0)) {
     return EFI_INVALID_PARAMETER;
   }
   
-  ret = 1;
   if(BufferIn == NULL && BufferInSize == 0) {
     //
     // If RequestBuffer is NULL and RequestSize is 0, and TLS session 
     // status is EfiTlsSessionNotStarted, the TLS session will be initiated 
     // and the response packet needs to be ClientHello.
     //
     PendingBufferSize = (UINTN) BIO_ctrl_pending (TlsConn->OutBio);
     if (PendingBufferSize == 0) {
       SSL_set_connect_state (TlsConn->Ssl);
-      ret = SSL_do_handshake (TlsConn->Ssl);
+      Ret = SSL_do_handshake (TlsConn->Ssl);
       PendingBufferSize = (UINTN) BIO_ctrl_pending (TlsConn->OutBio);
     }
   } else {
     PendingBufferSize = (UINTN) BIO_ctrl_pending (TlsConn->OutBio);
     if (PendingBufferSize == 0) {
       BIO_write (TlsConn->InBio, BufferIn, (UINT32) BufferInSize);
-      ret = SSL_do_handshake (TlsConn->Ssl);
+      Ret = SSL_do_handshake (TlsConn->Ssl);
       PendingBufferSize = (UINTN) BIO_ctrl_pending (TlsConn->OutBio);
     }
   }
 
-  if (ret < 1) {
-    ret = SSL_get_error (TlsConn->Ssl, ret);
-    if (ret == SSL_ERROR_SSL ||
-        ret == SSL_ERROR_SYSCALL ||
-        ret == SSL_ERROR_ZERO_RETURN) {
-      DEBUG ((DEBUG_ERROR, "%a SSL_HANDSHAKE_ERROR State=0x%x SSL_ERROR_%a\n", __FUNCTION__, SSL_state (TlsConn->Ssl),
-            ret == SSL_ERROR_SSL ? "SSL":
-            ret == SSL_ERROR_SYSCALL ? "SYSCALL":
-            "ZERO_RETURN"
-            ));
+  if (Ret < 1) {
+    Ret = SSL_get_error (TlsConn->Ssl, Ret);
+    if (Ret == SSL_ERROR_SSL ||
+        Ret == SSL_ERROR_SYSCALL ||
+        Ret == SSL_ERROR_ZERO_RETURN) {
+      DEBUG ((
+        DEBUG_ERROR, 
+        "%a SSL_HANDSHAKE_ERROR State=0x%x SSL_ERROR_%a\n",
+        __FUNCTION__, 
+        SSL_state (TlsConn->Ssl),
+        Ret == SSL_ERROR_SSL ? "SSL" : Ret == SSL_ERROR_SYSCALL ? "SYSCALL" : "ZERO_RETURN"
+        ));
       DEBUG_CODE_BEGIN ();
-      while (1) {
-        e = ERR_get_error ();
-        if (e == 0) {
-          break;
+        while (TRUE) {
+          ErrorCode = ERR_get_error ();
+          if (ErrorCode == 0) {
+            break;
+          }
+          DEBUG ((
+            DEBUG_ERROR, 
+            "%a ERROR 0x%x=L%x:F%x:R%x\n",
+            __FUNCTION__, 
+            ErrorCode, 
+            ERR_GET_LIB (ErrorCode), 
+            ERR_GET_FUNC (ErrorCode), 
+            ERR_GET_REASON (ErrorCode)
+            ));
         }
-        DEBUG ((DEBUG_ERROR, "%a ERROR 0x%x=L%x:F%x:R%x\n", __FUNCTION__, e, ERR_GET_LIB (e), ERR_GET_FUNC (e), ERR_GET_REASON (e)));
-      }
       DEBUG_CODE_END ();
       return EFI_PROTOCOL_ERROR;
     }
   }
 
-- 
1.9.5.msysgit.1

Re: [staging/HTTPS-TLS][PATCH 2/2] NetworkPkg: Fix potential assert issue

[Palmer, Thomas]

Reviewed by Thomas Palmer <thomas.palmer@hpe.com>

-----Original Message-----
From: Jiaxin Wu [mailto:jiaxin.wu@intel.com] 
Sent: Monday, July 11, 2016 12:43 AM
To: edk2-devel@lists.01.org
Cc: Palmer, Thomas <thomas.palmer@hpe.com>; Samer El-Haj-Mahmoud <Smahmoud@lenovo.com>; Long Qin <qin.long@intel.com>; Ye Ting <ting.ye@intel.com>; Fu Siyuan <siyuan.fu@intel.com>
Subject: [staging/HTTPS-TLS][PATCH 2/2] NetworkPkg: Fix potential assert issue

This patch is used to fix potential assert issue when connection failed or buffer allocation failed.

Cc: Palmer Thomas <thomas.palmer@hpe.com>
Cc: Samer El-Haj-Mahmoud <Smahmoud@lenovo.com>
Cc: Long Qin <qin.long@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 NetworkPkg/HttpDxe/HttpsSupport.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index 36f658c..d746972 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -874,11 +874,14 @@ TlsReceiveOnePdu (
     Status = EFI_OUT_OF_RESOURCES;
     goto ON_EXIT;
   }
 
   Header = NetbufAllocSpace (PduHdr, Len, NET_BUF_TAIL);
-  ASSERT (Header != NULL);
+  if (Header == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto ON_EXIT;
+  }
 
   //
   // First step, receive one TLS header.
   //
   Status = TlsCommonReceive (HttpInstance, PduHdr, Timeout); @@ -899,12 +902,10 @@ TlsReceiveOnePdu (
     InsertTailList (NbufList, &PduHdr->List);
   } else {
     Status = EFI_PROTOCOL_ERROR;
     goto ON_EXIT;
   }
-
-  ASSERT(Header != NULL);
     
   Len = SwapBytes16(RecordHeader.Length);
   if (Len == 0) {
     //
     // No TLS playload.
@@ -1184,11 +1185,13 @@ TlsConnectSession (
     if(HttpInstance->TlsSessionState == EfiTlsSessionError) {  
       return EFI_ABORTED;    
     }
   }
 
-  ASSERT(HttpInstance->TlsSessionState == EfiTlsSessionDataTransferring);
+  if (HttpInstance->TlsSessionState != EfiTlsSessionDataTransferring) {
+    Status = EFI_ABORTED;
+  }
 
   return Status;
 }
 
 /**
@@ -1572,11 +1575,14 @@ HttpsReceive (
     //
     ASSERT (((TLSRecordHeader *) (TempFragment.Bulk))->ContentType == TLS_CONTENT_TYPE_APPLICATION_DATA);
     
     BufferInSize = ((TLSRecordHeader *) (TempFragment.Bulk))->Length;
     BufferIn = AllocateZeroPool (BufferInSize);
-    ASSERT (BufferIn != NULL);
+    if (BufferIn == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
+      return Status;
+    }
 
     CopyMem (BufferIn, TempFragment.Bulk + sizeof (TLSRecordHeader), BufferInSize);
 
     //
     // Free the buffer in TempFragment.
--
1.9.5.msysgit.1