public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Jiaxin Wu <Jiaxin.wu@intel.com>
To: edk2-devel@lists.01.org
Cc: Ye Ting <ting.ye@intel.com>, Fu Siyuan <siyuan.fu@intel.com>,
	Wu Jiaxin <jiaxin.wu@intel.com>
Subject: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
Date: Mon,  2 Jul 2018 10:19:30 +0800	[thread overview]
Message-ID: <20180702021930.11488-1-Jiaxin.wu@intel.com> (raw)

*v2: Resolve the conflict commit.

HttpBodyParserCallback function is to parse the HTTP(S) message body so as to
confirm whether there is the next message header. But it doesn't record the
parsing message data/length correctly.

This patch is refine the parsing logic so as to fix the potential failure.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
---
 NetworkPkg/HttpDxe/HttpImpl.c  | 112 +++++++++++++++++----------------
 NetworkPkg/HttpDxe/HttpProto.c |  10 +++
 NetworkPkg/HttpDxe/HttpProto.h |  10 +++
 3 files changed, 77 insertions(+), 55 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index f70e116f38..57a3712c23 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -914,10 +914,11 @@ HttpBodyParserCallback (
   IN CHAR8                      *Data,
   IN UINTN                      Length,
   IN VOID                       *Context
   )
 {
+  HTTP_CALLBACK_DATA            *CallbackData;
   HTTP_TOKEN_WRAP               *Wrap;
   UINTN                         BodyLength;
   CHAR8                         *Body;
 
   if (EventType != BodyParseEventOnComplete) {
@@ -926,25 +927,22 @@ HttpBodyParserCallback (
 
   if (Data == NULL || Length != 0 || Context == NULL) {
     return EFI_SUCCESS;
   }
 
-  Wrap = (HTTP_TOKEN_WRAP *) Context;
-  Body = Wrap->HttpToken->Message->Body;
-  BodyLength = Wrap->HttpToken->Message->BodyLength;
+  CallbackData = (HTTP_CALLBACK_DATA *) Context;
+
+  Wrap       = (HTTP_TOKEN_WRAP *) (CallbackData->Wrap);
+  Body       = CallbackData->ParseData;
+  BodyLength = CallbackData->ParseDataLength;
+
   if (Data < Body + BodyLength) {
     Wrap->HttpInstance->NextMsg = Data;
   } else {
     Wrap->HttpInstance->NextMsg = NULL;
   }
 
-
-  //
-  // Free Tx4Token or Tx6Token since already received corrsponding HTTP response.
-  //
-  FreePool (Wrap);
-
   return EFI_SUCCESS;
 }
 
 /**
   The work function of EfiHttpResponse().
@@ -1189,33 +1187,43 @@ HttpResponseWorker (
                  HttpInstance->Method,
                  HttpMsg->Data.Response->StatusCode,
                  HttpMsg->HeaderCount,
                  HttpMsg->Headers,
                  HttpBodyParserCallback,
-                 (VOID *) ValueInItem,
+                 (VOID *) (&HttpInstance->CallbackData),
                  &HttpInstance->MsgParser
                  );
       if (EFI_ERROR (Status)) {
         goto Error2;
       }
 
       //
       // Check whether we received a complete HTTP message.
       //
       if (HttpInstance->CacheBody != NULL) {
+        //
+        // Record the CallbackData data.
+        //
+        HttpInstance->CallbackData.Wrap = (VOID *) Wrap;
+        HttpInstance->CallbackData.ParseData = (VOID *) HttpInstance->CacheBody;
+        HttpInstance->CallbackData.ParseDataLength = HttpInstance->CacheLen;
+
+        //
+        // Parse message with CallbackData data.
+        //
         Status = HttpParseMessageBody (HttpInstance->MsgParser, HttpInstance->CacheLen, HttpInstance->CacheBody);
         if (EFI_ERROR (Status)) {
           goto Error2;
         }
+      }
 
-        if (HttpIsMessageComplete (HttpInstance->MsgParser)) {
-          //
-          // Free the MsgParse since we already have a full HTTP message.
-          //
-          HttpFreeMsgParser (HttpInstance->MsgParser);
-          HttpInstance->MsgParser = NULL;
-        }
+      if (HttpIsMessageComplete (HttpInstance->MsgParser)) {
+        //
+        // Free the MsgParse since we already have a full HTTP message.
+        //
+        HttpFreeMsgParser (HttpInstance->MsgParser);
+        HttpInstance->MsgParser = NULL;
       }
     }
 
     if ((HttpMsg->Body == NULL) || (HttpMsg->BodyLength == 0)) {
       Status = EFI_SUCCESS;
@@ -1330,16 +1338,30 @@ HttpResponseWorker (
     if (EFI_ERROR (Status)) {
       goto Error2;
     }
 
     //
-    // Check whether we receive a complete HTTP message.
+    // Process the received the body packet.
+    //
+    HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
+
+    CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
+
+    //
+    // Record the CallbackData data.
+    //
+    HttpInstance->CallbackData.Wrap = (VOID *) Wrap;
+    HttpInstance->CallbackData.ParseData = HttpMsg->Body;
+    HttpInstance->CallbackData.ParseDataLength = HttpMsg->BodyLength;
+
+    //
+    // Parse Body with CallbackData data.
     //
     Status = HttpParseMessageBody (
                HttpInstance->MsgParser,
-               (UINTN) Fragment.Len,
-               (CHAR8 *) Fragment.Bulk
+               HttpMsg->BodyLength,
+               HttpMsg->Body
                );
     if (EFI_ERROR (Status)) {
       goto Error2;
     }
 
@@ -1350,51 +1372,31 @@ HttpResponseWorker (
       HttpFreeMsgParser (HttpInstance->MsgParser);
       HttpInstance->MsgParser = NULL;
     }
 
     //
-    // We receive part of header of next HTTP msg.
+    // Check whether there is the next message header in the HttpMsg->Body.
     //
     if (HttpInstance->NextMsg != NULL) {
-      HttpMsg->BodyLength = MIN ((UINTN) HttpInstance->NextMsg - (UINTN) Fragment.Bulk, HttpMsg->BodyLength);
-      CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
-
-      HttpInstance->CacheLen = Fragment.Len - HttpMsg->BodyLength;
-      if (HttpInstance->CacheLen != 0) {
-        if (HttpInstance->CacheBody != NULL) {
-          FreePool (HttpInstance->CacheBody);
-        }
-
-        HttpInstance->CacheBody = AllocateZeroPool (HttpInstance->CacheLen);
-        if (HttpInstance->CacheBody == NULL) {
-          Status = EFI_OUT_OF_RESOURCES;
-          goto Error2;
-        }
-
-        CopyMem (HttpInstance->CacheBody, Fragment.Bulk + HttpMsg->BodyLength, HttpInstance->CacheLen);
-        HttpInstance->CacheOffset = 0;
+      HttpMsg->BodyLength = HttpInstance->NextMsg - (CHAR8 *) HttpMsg->Body;
+    }
 
-        HttpInstance->NextMsg = HttpInstance->CacheBody + ((UINTN) HttpInstance->NextMsg - (UINTN) (Fragment.Bulk + HttpMsg->BodyLength));
+    HttpInstance->CacheLen = Fragment.Len - HttpMsg->BodyLength;
+    if (HttpInstance->CacheLen != 0) {
+      if (HttpInstance->CacheBody != NULL) {
+        FreePool (HttpInstance->CacheBody);
       }
-    } else {
-      HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
-      CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
-      HttpInstance->CacheLen = Fragment.Len - HttpMsg->BodyLength;
-      if (HttpInstance->CacheLen != 0) {
-        if (HttpInstance->CacheBody != NULL) {
-          FreePool (HttpInstance->CacheBody);
-        }
-
-        HttpInstance->CacheBody = AllocateZeroPool (HttpInstance->CacheLen);
-        if (HttpInstance->CacheBody == NULL) {
-          Status = EFI_OUT_OF_RESOURCES;
-          goto Error2;
-        }
 
-        CopyMem (HttpInstance->CacheBody, Fragment.Bulk + HttpMsg->BodyLength, HttpInstance->CacheLen);
-        HttpInstance->CacheOffset = 0;
+      HttpInstance->CacheBody = AllocateZeroPool (HttpInstance->CacheLen);
+      if (HttpInstance->CacheBody == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Error2;
       }
+
+      CopyMem (HttpInstance->CacheBody, Fragment.Bulk + HttpMsg->BodyLength, HttpInstance->CacheLen);
+      HttpInstance->CacheOffset = 0;
+      HttpInstance->NextMsg = HttpInstance->CacheBody;
     }
 
     if (Fragment.Bulk != NULL) {
       FreePool (Fragment.Bulk);
       Fragment.Bulk = NULL;
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 5356cd35c0..94f89f5665 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -195,10 +195,20 @@ HttpTcpReceiveNotifyDpc (
     Length = (UINTN) Wrap->TcpWrap.Rx6Data.FragmentTable[0].FragmentLength;
   } else {
     Length = (UINTN) Wrap->TcpWrap.Rx4Data.FragmentTable[0].FragmentLength;
   }
 
+  //
+  // Record the CallbackData data.
+  //
+  HttpInstance->CallbackData.Wrap = (VOID *) Wrap;
+  HttpInstance->CallbackData.ParseData = Wrap->HttpToken->Message->Body;
+  HttpInstance->CallbackData.ParseDataLength = Length;
+
+  //
+  // Parse Body with CallbackData data.
+  //
   Status = HttpParseMessageBody (
              HttpInstance->MsgParser,
              Length,
              Wrap->HttpToken->Message->Body
              );
diff --git a/NetworkPkg/HttpDxe/HttpProto.h b/NetworkPkg/HttpDxe/HttpProto.h
index cc6c1eb566..fa57dbfd39 100644
--- a/NetworkPkg/HttpDxe/HttpProto.h
+++ b/NetworkPkg/HttpDxe/HttpProto.h
@@ -89,10 +89,19 @@ typedef struct {
   EFI_TLS_CONNECTION_END        ConnectionEnd;
   EFI_TLS_VERIFY                VerifyMethod;
   EFI_TLS_SESSION_STATE         SessionState;
 } TLS_CONFIG_DATA;
 
+//
+// Callback data for HTTP_PARSER_CALLBACK()
+//
+typedef struct {
+  UINTN                         ParseDataLength;
+  VOID                          *ParseData;
+  VOID                          *Wrap;
+} HTTP_CALLBACK_DATA;
+
 typedef struct _HTTP_PROTOCOL {
   UINT32                        Signature;
   EFI_HTTP_PROTOCOL             Http;
   EFI_HANDLE                    Handle;
   HTTP_SERVICE                  *Service;
@@ -147,10 +156,11 @@ typedef struct _HTTP_PROTOCOL {
 
   //
   // HTTP message-body parser.
   //
   VOID                          *MsgParser;
+  HTTP_CALLBACK_DATA            CallbackData;
 
   EFI_HTTP_VERSION              HttpVersion;
   UINT32                        TimeOutMillisec;
   BOOLEAN                       LocalAddressIsIPv6;
 
-- 
2.17.1.windows.2



             reply	other threads:[~2018-07-02  2:19 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-02  2:19 Jiaxin Wu [this message]
2018-07-02  4:17 ` [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body Gary Lin
2018-07-02  5:21   ` Wu, Jiaxin
2018-07-02  5:34     ` Wu, Jiaxin
2018-07-02  8:16       ` Gary Lin
2018-07-02  8:37         ` Wu, Jiaxin
2018-07-02  9:34           ` Gary Lin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180702021930.11488-1-Jiaxin.wu@intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox