* [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
@ 2018-07-02 2:19 Jiaxin Wu
2018-07-02 4:17 ` Gary Lin
0 siblings, 1 reply; 7+ messages in thread
From: Jiaxin Wu @ 2018-07-02 2:19 UTC (permalink / raw)
To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wu Jiaxin
*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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
2018-07-02 2:19 [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body Jiaxin Wu
@ 2018-07-02 4:17 ` Gary Lin
2018-07-02 5:21 ` Wu, Jiaxin
0 siblings, 1 reply; 7+ messages in thread
From: Gary Lin @ 2018-07-02 4:17 UTC (permalink / raw)
To: Jiaxin Wu; +Cc: edk2-devel, Ye Ting, Fu Siyuan
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
2018-07-02 4:17 ` Gary Lin
@ 2018-07-02 5:21 ` Wu, Jiaxin
2018-07-02 5:34 ` Wu, Jiaxin
0 siblings, 1 reply; 7+ messages in thread
From: Wu, Jiaxin @ 2018-07-02 5:21 UTC (permalink / raw)
To: Gary Lin; +Cc: edk2-devel@lists.01.org, Ye, Ting, Fu, Siyuan
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
2018-07-02 5:21 ` Wu, Jiaxin
@ 2018-07-02 5:34 ` Wu, Jiaxin
2018-07-02 8:16 ` Gary Lin
0 siblings, 1 reply; 7+ messages in thread
From: Wu, Jiaxin @ 2018-07-02 5:34 UTC (permalink / raw)
To: Wu, Jiaxin, Gary Lin; +Cc: Ye, Ting, edk2-devel@lists.01.org, Fu, Siyuan
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
2018-07-02 5:34 ` Wu, Jiaxin
@ 2018-07-02 8:16 ` Gary Lin
2018-07-02 8:37 ` Wu, Jiaxin
0 siblings, 1 reply; 7+ messages in thread
From: Gary Lin @ 2018-07-02 8:16 UTC (permalink / raw)
To: Wu, Jiaxin; +Cc: Ye, Ting, edk2-devel@lists.01.org, Fu, Siyuan
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
2018-07-02 8:16 ` Gary Lin
@ 2018-07-02 8:37 ` Wu, Jiaxin
2018-07-02 9:34 ` Gary Lin
0 siblings, 1 reply; 7+ messages in thread
From: Wu, Jiaxin @ 2018-07-02 8:37 UTC (permalink / raw)
To: Gary Lin; +Cc: Ye, Ting, edk2-devel@lists.01.org, Fu, Siyuan
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body.
2018-07-02 8:37 ` Wu, Jiaxin
@ 2018-07-02 9:34 ` Gary Lin
0 siblings, 0 replies; 7+ messages in thread
From: Gary Lin @ 2018-07-02 9:34 UTC (permalink / raw)
To: Wu, Jiaxin; +Cc: Ye, Ting, edk2-devel@lists.01.org, Fu, Siyuan
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
> > >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-02 9:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-02 2:19 [Patch v2] NetworkPkg/HttpDxe: Fix the bug when parsing HTTP(S) message body Jiaxin Wu
2018-07-02 4:17 ` Gary Lin
2018-07-02 5:21 ` Wu, Jiaxin
2018-07-02 5:34 ` Wu, Jiaxin
2018-07-02 8:16 ` Gary Lin
2018-07-02 8:37 ` Wu, Jiaxin
2018-07-02 9:34 ` Gary Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox