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

2018-07-02 Thread Gary Lin
On Mon, Jul 02, 2018 at 08:37:41AM +, 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:g...@suse.com]
> > Sent: Monday, July 2, 2018 4:17 PM
> > To: Wu, Jiaxin 
> > Cc: Ye, Ting ; edk2-devel@lists.01.org; Fu, Siyuan
> > 
> > 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 +, 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:g...@suse.com]
> > > > > Sent: Monday, July 2, 2018 12:17 PM
> > > > > To: Wu, Jiaxin 
> > > > > Cc: edk2-devel@lists.01.org; Ye, Ting ; Fu, Siyuan
> > > > > 
> > > > > 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 
> > > > > > Cc: Fu Siyuan 
> > > > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > > > Signed-off-by: Wu Jiaxin 
> > > > > > Reviewed-by: Fu Siyuan 
> > > > > > ---
> > > > > >  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 10

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

2018-07-02 Thread Wu, Jiaxin
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:g...@suse.com]
> Sent: Monday, July 2, 2018 4:17 PM
> To: Wu, Jiaxin 
> Cc: Ye, Ting ; edk2-devel@lists.01.org; Fu, Siyuan
> 
> 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 +, 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:g...@suse.com]
> > > > Sent: Monday, July 2, 2018 12:17 PM
> > > > To: Wu, Jiaxin 
> > > > Cc: edk2-devel@lists.01.org; Ye, Ting ; Fu, Siyuan
> > > > 
> > > > 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 
> > > > > Cc: Fu Siyuan 
> > > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > > Signed-off-by: Wu Jiaxin 
> > > > > Reviewed-by: Fu Siyuan 
> > > > > ---
> > > > >  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 *

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

2018-07-02 Thread Gary Lin
On Mon, Jul 02, 2018 at 05:34:29AM +, 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:g...@suse.com]
> > > Sent: Monday, July 2, 2018 12:17 PM
> > > To: Wu, Jiaxin 
> > > Cc: edk2-devel@lists.01.org; Ye, Ting ; Fu, Siyuan
> > > 
> > > 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 
> > > > Cc: Fu Siyuan 
> > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > Signed-off-by: Wu Jiaxin 
> > > > Reviewed-by: Fu Siyuan 
> > > > ---
> > > >  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;
> > > >}
> > > >
> > > > -
> > 

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

2018-07-01 Thread Wu, Jiaxin
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:g...@suse.com]
> > Sent: Monday, July 2, 2018 12:17 PM
> > To: Wu, Jiaxin 
> > Cc: edk2-devel@lists.01.org; Ye, Ting ; Fu, Siyuan
> > 
> > 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 
> > > Cc: Fu Siyuan 
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Wu Jiaxin 
> > > Reviewed-by: Fu Siyuan 
> > > ---
> > >  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 *) (>CallbackData),
> > >   >MsgParser
> > >   );
> > >if (EFI_ERROR (Status)) {
> > >  goto Error2;
> > >}
> >

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

2018-07-01 Thread Wu, Jiaxin
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:g...@suse.com]
> Sent: Monday, July 2, 2018 12:17 PM
> To: Wu, Jiaxin 
> Cc: edk2-devel@lists.01.org; Ye, Ting ; Fu, Siyuan
> 
> 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 
> > Cc: Fu Siyuan 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin 
> > Reviewed-by: Fu Siyuan 
> > ---
> >  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 *) (>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;
> > +
> > +//
> > +   

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

2018-07-01 Thread Gary Lin
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 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin 
> Reviewed-by: Fu Siyuan 
> ---
>  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 *) (>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.
> +//
> +

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

2018-07-01 Thread Jiaxin Wu
*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 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin 
Reviewed-by: Fu Siyuan 
---
 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 *) (>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,51 +1372,31 @@ HttpResponseWorker (
   HttpFreeMsgParser (HttpInstance->MsgParser);
   HttpInstance->MsgParser = NULL;
 }
 
 //
-// We receive part of header of next