[v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

[edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Jiaxin Wu 5 years, 9 months ago

*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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Gary Lin 5 years, 9 months ago

On Mon, Jul 02, 2018 at 10:19:30AM +0800, Jiaxin Wu wrote:
> *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.
> 
Hi Jiaxin,

After applying this patch, shim failed to fetch grub.efi through the
HTTPS connection. It got EFI_BAD_BUFFER_SIZE from HTTP->Response() and I
found several error messages in the OVMF log:

TcpInput: Discard a packet
TcpSendIpPacket: No appropriate IpSender.

This only happened with HTTPS. If I replace the HTTPS URL in dhcpd.conf with
the HTTP URL, it works as expected.

Gary Lin

> 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
> 
> _______________________________________________
> 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

Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Wu, Jiaxin 5 years, 9 months ago

Hi Gary,

Thanks the verification, in my part, can't HTTPS works well. Can you send me the failure wireshark packet/debug message to me?

Thanks,
Jiaxin  

> -----Original Message-----
> From: Gary Lin [mailto:glin@suse.com]
> Sent: Monday, July 2, 2018 12:17 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> parsing HTTP(S) message body.
> 
> On Mon, Jul 02, 2018 at 10:19:30AM +0800, Jiaxin Wu wrote:
> > *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.
> >
> Hi Jiaxin,
> 
> After applying this patch, shim failed to fetch grub.efi through the
> HTTPS connection. It got EFI_BAD_BUFFER_SIZE from HTTP->Response() and
> I
> found several error messages in the OVMF log:
> 
> TcpInput: Discard a packet
> TcpSendIpPacket: No appropriate IpSender.
> 
> This only happened with HTTPS. If I replace the HTTPS URL in dhcpd.conf with
> the HTTP URL, it works as expected.
> 
> Gary Lin
> 
> > 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
> >
> > _______________________________________________
> > 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

Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Wu, Jiaxin 5 years, 9 months ago

Hi Gary,

Thanks the verification, in my part, HTTPS works well. Can you send me
the failure wireshark packet/full debug message?

(correct the typo)

Thanks,
Jiaxin


> 
> > -----Original Message-----
> > From: Gary Lin [mailto:glin@suse.com]
> > Sent: Monday, July 2, 2018 12:17 PM
> > To: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> > parsing HTTP(S) message body.
> >
> > On Mon, Jul 02, 2018 at 10:19:30AM +0800, Jiaxin Wu wrote:
> > > *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.
> > >
> > Hi Jiaxin,
> >
> > After applying this patch, shim failed to fetch grub.efi through the
> > HTTPS connection. It got EFI_BAD_BUFFER_SIZE from HTTP->Response()
> and
> > I
> > found several error messages in the OVMF log:
> >
> > TcpInput: Discard a packet
> > TcpSendIpPacket: No appropriate IpSender.
> >
> > This only happened with HTTPS. If I replace the HTTPS URL in dhcpd.conf
> with
> > the HTTP URL, it works as expected.
> >
> > Gary Lin
> >
> > > 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
> > >
> > > _______________________________________________
> > > 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Gary Lin 5 years, 9 months ago

On Mon, Jul 02, 2018 at 05:34:29AM +0000, Wu, Jiaxin wrote:
> Hi Gary,
> 
> Thanks the verification, in my part, HTTPS works well. Can you send me
> the failure wireshark packet/full debug message?
> 
> (correct the typo)
> 
Per my test, HTTPS worked for firmware -> bootx64.efi(shim.efi), and
shim failed to fetch grub2.

Here is the packets captured by wireshark:
https://drive.google.com/open?id=11UMRtkq521My3VNY75MeKDL8AFI12RzE

From the log, the second connection did start but end suddenly after a
few packet transmissions.

For reference, here are the sizes of bootx64.efi and grub.efi:

bootx64.efi: 1208528 bytes
grub.efi: 1057792 bytes

Gary Lin

> Thanks,
> Jiaxin
> 
> 
> > 
> > > -----Original Message-----
> > > From: Gary Lin [mailto:glin@suse.com]
> > > Sent: Monday, July 2, 2018 12:17 PM
> > > To: Wu, Jiaxin <jiaxin.wu@intel.com>
> > > Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> > > <siyuan.fu@intel.com>
> > > Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> > > parsing HTTP(S) message body.
> > >
> > > On Mon, Jul 02, 2018 at 10:19:30AM +0800, Jiaxin Wu wrote:
> > > > *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.
> > > >
> > > Hi Jiaxin,
> > >
> > > After applying this patch, shim failed to fetch grub.efi through the
> > > HTTPS connection. It got EFI_BAD_BUFFER_SIZE from HTTP->Response()
> > and
> > > I
> > > found several error messages in the OVMF log:
> > >
> > > TcpInput: Discard a packet
> > > TcpSendIpPacket: No appropriate IpSender.
> > >
> > > This only happened with HTTPS. If I replace the HTTPS URL in dhcpd.conf
> > with
> > > the HTTP URL, it works as expected.
> > >
> > > Gary Lin
> > >
> > > > 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
> > > >
> > > > _______________________________________________
> > > > 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
> _______________________________________________
> 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

Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Wu, Jiaxin 5 years, 9 months ago

Sorry, I can't access below google link:(. It will be better to share us the steps to reproduce the issue. I don't know whether the below two files are enough to reproduce the issue, but you can share it to me first, then I will try that.

 > bootx64.efi: 1208528 bytes
> grub.efi: 1057792 bytes

BTW, the issue only happened after applying this patches? 

Thanks,
Jiaxin
 

> -----Original Message-----
> From: Gary Lin [mailto:glin@suse.com]
> Sent: Monday, July 2, 2018 4:17 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org; Fu, Siyuan
> <siyuan.fu@intel.com>
> Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> parsing HTTP(S) message body.
> 
> On Mon, Jul 02, 2018 at 05:34:29AM +0000, Wu, Jiaxin wrote:
> > Hi Gary,
> >
> > Thanks the verification, in my part, HTTPS works well. Can you send me
> > the failure wireshark packet/full debug message?
> >
> > (correct the typo)
> >
> Per my test, HTTPS worked for firmware -> bootx64.efi(shim.efi), and
> shim failed to fetch grub2.
> 
> Here is the packets captured by wireshark:
> https://drive.google.com/open?id=11UMRtkq521My3VNY75MeKDL8AFI12Rz
> E
> 
> From the log, the second connection did start but end suddenly after a
> few packet transmissions.
> 
> For reference, here are the sizes of bootx64.efi and grub.efi:
> 
> bootx64.efi: 1208528 bytes
> grub.efi: 1057792 bytes
> 
> Gary Lin
> 
> > Thanks,
> > Jiaxin
> >
> >
> > >
> > > > -----Original Message-----
> > > > From: Gary Lin [mailto:glin@suse.com]
> > > > Sent: Monday, July 2, 2018 12:17 PM
> > > > To: Wu, Jiaxin <jiaxin.wu@intel.com>
> > > > Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> > > > <siyuan.fu@intel.com>
> > > > Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> > > > parsing HTTP(S) message body.
> > > >
> > > > On Mon, Jul 02, 2018 at 10:19:30AM +0800, Jiaxin Wu wrote:
> > > > > *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.
> > > > >
> > > > Hi Jiaxin,
> > > >
> > > > After applying this patch, shim failed to fetch grub.efi through the
> > > > HTTPS connection. It got EFI_BAD_BUFFER_SIZE from HTTP->Response()
> > > and
> > > > I
> > > > found several error messages in the OVMF log:
> > > >
> > > > TcpInput: Discard a packet
> > > > TcpSendIpPacket: No appropriate IpSender.
> > > >
> > > > This only happened with HTTPS. If I replace the HTTPS URL in
> dhcpd.conf
> > > with
> > > > the HTTP URL, it works as expected.
> > > >
> > > > Gary Lin
> > > >
> > > > > 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
> > > > >
> > > > > _______________________________________________
> > > > > 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
> > _______________________________________________
> > 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

Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.

Posted by Gary Lin 5 years, 9 months ago

On Mon, Jul 02, 2018 at 08:37:41AM +0000, Wu, Jiaxin wrote:
> Sorry, I can't access below google link:(. It will be better to share us the steps to reproduce the issue. I don't know whether the below two files are enough to reproduce the issue, but you can share it to me first, then I will try that.
> 
Ah, ok. I'll send you the files privately.

>  > bootx64.efi: 1208528 bytes
> > grub.efi: 1057792 bytes
> 
> BTW, the issue only happened after applying this patches? 
> 
Yes. Without the patch, shim can download grub2 successfully.

Thanks,

Gary Lin

> Thanks,
> Jiaxin
>  
> 
> > -----Original Message-----
> > From: Gary Lin [mailto:glin@suse.com]
> > Sent: Monday, July 2, 2018 4:17 PM
> > To: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Cc: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> > parsing HTTP(S) message body.
> > 
> > On Mon, Jul 02, 2018 at 05:34:29AM +0000, Wu, Jiaxin wrote:
> > > Hi Gary,
> > >
> > > Thanks the verification, in my part, HTTPS works well. Can you send me
> > > the failure wireshark packet/full debug message?
> > >
> > > (correct the typo)
> > >
> > Per my test, HTTPS worked for firmware -> bootx64.efi(shim.efi), and
> > shim failed to fetch grub2.
> > 
> > Here is the packets captured by wireshark:
> > https://drive.google.com/open?id=11UMRtkq521My3VNY75MeKDL8AFI12Rz
> > E
> > 
> > From the log, the second connection did start but end suddenly after a
> > few packet transmissions.
> > 
> > For reference, here are the sizes of bootx64.efi and grub.efi:
> > 
> > bootx64.efi: 1208528 bytes
> > grub.efi: 1057792 bytes
> > 
> > Gary Lin
> > 
> > > Thanks,
> > > Jiaxin
> > >
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Gary Lin [mailto:glin@suse.com]
> > > > > Sent: Monday, July 2, 2018 12:17 PM
> > > > > To: Wu, Jiaxin <jiaxin.wu@intel.com>
> > > > > Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> > > > > <siyuan.fu@intel.com>
> > > > > Subject: Re: [edk2] [Patch v2] NetworkPkg/HttpDxe: Fix the bug when
> > > > > parsing HTTP(S) message body.
> > > > >
> > > > > On Mon, Jul 02, 2018 at 10:19:30AM +0800, Jiaxin Wu wrote:
> > > > > > *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.
> > > > > >
> > > > > Hi Jiaxin,
> > > > >
> > > > > After applying this patch, shim failed to fetch grub.efi through the
> > > > > HTTPS connection. It got EFI_BAD_BUFFER_SIZE from HTTP->Response()
> > > > and
> > > > > I
> > > > > found several error messages in the OVMF log:
> > > > >
> > > > > TcpInput: Discard a packet
> > > > > TcpSendIpPacket: No appropriate IpSender.
> > > > >
> > > > > This only happened with HTTPS. If I replace the HTTPS URL in
> > dhcpd.conf
> > > > with
> > > > > the HTTP URL, it works as expected.
> > > > >
> > > > > Gary Lin
> > > > >
> > > > > > 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
> > > > > >
> > > > > > _______________________________________________
> > > > > > 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
> > > _______________________________________________
> > > 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