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

2018-07-04 Thread Ye, Ting 
Looks good to me.

Reviewed-by: Ye Ting  

-Original Message-
From: Wu, Jiaxin 
Sent: Wednesday, July 4, 2018 8:41 AM
To: edk2-devel@lists.01.org
Cc: Ye, Ting ; Fu, Siyuan ; Gary Lin 
; Wu, Jiaxin 
Subject: [Patch v3] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) 
message body.

*v2: Resolve the conflict commit.

*v3: Fixed the failure if BodyLength in HTTP token is less than the received 
size of HTTPS message.

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 
Cc: Fu Siyuan 
Cc: Gary Lin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin 
Tested-by: Gary Lin 
---
 NetworkPkg/HttpDxe/HttpImpl.c  | 112 +  
NetworkPkg/HttpDxe/HttpProto.c |  10 +++  NetworkPkg/HttpDxe/HttpProto.h |  10 
+++
 3 files changed, 78 insertions(+), 54 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c 
index f70e116f38..17deceb395 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 *) (>CallbackData),
  >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,
-

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

2018-07-03 Thread Wu, Jiaxin 
Great, thanks the verification. /Jiaxin


> -Original Message-
> From: Gary Lin [mailto:g...@suse.com]
> Sent: Wednesday, July 4, 2018 10:14 AM
> To: Wu, Jiaxin 
> Cc: edk2-devel@lists.01.org; Ye, Ting ; Fu, Siyuan
> 
> Subject: Re: [edk2] [Patch v3] NetworkPkg/HttpDxe: Fix the bug when
> parsing HTTP(S) message body.
> 
> On Wed, Jul 04, 2018 at 08:40:52AM +0800, Jiaxin Wu wrote:
> > *v2: Resolve the conflict commit.
> >
> > *v3: Fixed the failure if BodyLength in HTTP token is less than the received
> > size of HTTPS message.
> >
> > 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 
> > Cc: Fu Siyuan 
> > Cc: Gary Lin 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin 
> 
> > Tested-by: Gary Lin 
> Thanks for the patch. I've tested this patch with shim and grub2 from
> SLE15 GM, and they worked as expected. A crash in grub2 https connection is
> also gone after applying this patch.
> 
> Thanks,
> 
> Gary Lin
> 
> > ---
> >  NetworkPkg/HttpDxe/HttpImpl.c  | 112 +---
> -
> >  NetworkPkg/HttpDxe/HttpProto.c |  10 +++
> >  NetworkPkg/HttpDxe/HttpProto.h |  10 +++
> >  3 files changed, 78 insertions(+), 54 deletions(-)
> >
> > diff --git a/NetworkPkg/HttpDxe/HttpImpl.c
> b/NetworkPkg/HttpDxe/HttpImpl.c
> > index f70e116f38..17deceb395 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 *) (>CallbackData),
> >   >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);

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

2018-07-03 Thread Gary Lin 
On Wed, Jul 04, 2018 at 08:40:52AM +0800, Jiaxin Wu wrote:
> *v2: Resolve the conflict commit.
> 
> *v3: Fixed the failure if BodyLength in HTTP token is less than the received
> size of HTTPS message.
> 
> 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 
> Cc: Fu Siyuan 
> Cc: Gary Lin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin 

> Tested-by: Gary Lin 
Thanks for the patch. I've tested this patch with shim and grub2 from
SLE15 GM, and they worked as expected. A crash in grub2 https connection is
also gone after applying this patch.

Thanks,

Gary Lin

> ---
>  NetworkPkg/HttpDxe/HttpImpl.c  | 112 +
>  NetworkPkg/HttpDxe/HttpProto.c |  10 +++
>  NetworkPkg/HttpDxe/HttpProto.h |  10 +++
>  3 files changed, 78 insertions(+), 54 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index f70e116f38..17deceb395 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 *) (>CallbackData),
>   >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;
> +

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

2018-07-03 Thread Jiaxin Wu 
*v2: Resolve the conflict commit.

*v3: Fixed the failure if BodyLength in HTTP token is less than the received
size of HTTPS message.

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 
Cc: Fu Siyuan 
Cc: Gary Lin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin 
Tested-by: Gary Lin 
---
 NetworkPkg/HttpDxe/HttpImpl.c  | 112 +
 NetworkPkg/HttpDxe/HttpProto.c |  10 +++
 NetworkPkg/HttpDxe/HttpProto.h |  10 +++
 3 files changed, 78 insertions(+), 54 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index f70e116f38..17deceb395 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 *) (>CallbackData),
  >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,50 +1372,32 @@ HttpResponseWorker (
   HttpFreeMsgParser