Re: [edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

2018-09-12 Thread Ye, Ting 
Reviewed-by: Ye Ting  

-Original Message-
From: Wu, Jiaxin 
Sent: Tuesday, September 4, 2018 3:17 PM
To: edk2-devel@lists.01.org
Cc: Stephen Benjamin ; Laszlo Ersek ; 
Ye, Ting ; Fu, Siyuan ; Wu, Jiaxin 

Subject: [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in 
HTTP header.

This patch is to resolve the lock-up issue if the value of HTTP header is 
blank.  The issue is recorded @ 
https://bugzilla.tianocore.org/show_bug.cgi?id=1102.

Cc: Stephen Benjamin 
Cc: Laszlo Ersek 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin 
---
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 5fbb50d03a..2fc3da8a2d 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -1595,63 +1595,94 @@ HttpGetFieldNameAndValue (
 return NULL;
   }
 
   //
   // Each header field consists of a name followed by a colon (":") and the 
field value.
+  // The field value MAY be preceded by any amount of LWS, though a single SP 
is preferred.
+  //
+  // message-header = field-name ":" [ field-value ]  // field-name = 
+ token  // field-value = *( field-content | LWS )  //  // Note: 
+ "*(element)" allows any number element, including zero; "1*(element)" 
requires at least one element.
+  //   [element] means element is optional.
+  //   LWS  = [CRLF] 1*(SP|HT), it can be ' ' or '\t' or '\r\n ' or 
'\r\n\t'.
+  //   CRLF = '\r\n'.
+  //   SP   = ' '.
+  //   HT   = '\t' (Tab).
   //
   FieldNameStr = String;
   FieldValueStr = AsciiStrGetNextToken (FieldNameStr, ':');
   if (FieldValueStr == NULL) {
 return NULL;
   }
 
   //
-  // Replace ':' with 0
+  // Replace ':' with 0, then FieldName has been retrived from String.
   //
   *(FieldValueStr - 1) = 0;
 
   //
-  // The field value MAY be preceded by any amount of LWS, though a single SP 
is preferred.
-  // Note: LWS  = [CRLF] 1*(SP|HT), it can be '\r\n ' or '\r\n\t' or ' ' or 
'\t'.
-  //   CRLF = '\r\n'.
-  //   SP   = ' '.
-  //   HT   = '\t' (Tab).
+  // Handle FieldValueStr, skip all the preceded LWS.
   //
   while (TRUE) {
 if (*FieldValueStr == ' ' || *FieldValueStr == '\t') {
   //
   // Boundary condition check.
   //
   if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 1) {
+//
+// Wrong String format!
+//
 return NULL;
   }
 
   FieldValueStr ++;
 } else if (*FieldValueStr == '\r') {
   //
   // Boundary condition check.
   //
   if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 3) {
-return NULL;
+//
+// No more preceded LWS, so break here.
+//
+break;
   }
 
-  if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' || 
*(FieldValueStr + 2) == '\t')) {
-FieldValueStr = FieldValueStr + 3;
+  if (*(FieldValueStr + 1) == '\n' ) {
+if (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t') {
+  FieldValueStr = FieldValueStr + 3;
+} else {
+  //
+  // No more preceded LWS, so break here.
+  //
+  break;
+}
+  } else {
+//
+// Wrong String format!
+//
+return NULL;
   }
 } else {
+  //
+  // No more preceded LWS, so break here.
+  //
   break;
 }
   }
 
-  //
-  // Header fields can be extended over multiple lines by preceding each extra
-  // line with at least one SP or HT.
-  //
   StrPtr = FieldValueStr;
   do {
+//
+// Handle the LWS within the field value.
+//
 StrPtr = AsciiStrGetNextToken (StrPtr, '\r');
 if (StrPtr == NULL || *StrPtr != '\n') {
+  //
+  // Wrong String format!
+  //
   return NULL;
 }
 
 StrPtr++;
   } while (*StrPtr == ' ' || *StrPtr == '\t');
--
2.17.1.windows.2

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

Re: [edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

2018-09-12 Thread Fu, Siyuan 
Reviewed-by: Fu Siyuan 



> -Original Message-
> From: Wu, Jiaxin
> Sent: Tuesday, September 4, 2018 3:17 PM
> To: edk2-devel@lists.01.org
> Cc: Stephen Benjamin ; Laszlo Ersek
> ; Ye, Ting ; Fu, Siyuan
> ; Wu, Jiaxin 
> Subject: [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value
> in HTTP header.
> 
> This patch is to resolve the lock-up issue if the value of HTTP header
> is blank.  The issue is recorded @
> https://bugzilla.tianocore.org/show_bug.cgi?id=1102.
> 
> Cc: Stephen Benjamin 
> Cc: Laszlo Ersek 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin 
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
>  1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 5fbb50d03a..2fc3da8a2d 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1595,63 +1595,94 @@ HttpGetFieldNameAndValue (
>  return NULL;
>}
> 
>//
>// Each header field consists of a name followed by a colon (":") and
> the field value.
> +  // The field value MAY be preceded by any amount of LWS, though a
> single SP is preferred.
> +  //
> +  // message-header = field-name ":" [ field-value ]
> +  // field-name = token
> +  // field-value = *( field-content | LWS )
> +  //
> +  // Note: "*(element)" allows any number element, including zero;
> "1*(element)" requires at least one element.
> +  //   [element] means element is optional.
> +  //   LWS  = [CRLF] 1*(SP|HT), it can be ' ' or '\t' or '\r\n ' or
> '\r\n\t'.
> +  //   CRLF = '\r\n'.
> +  //   SP   = ' '.
> +  //   HT   = '\t' (Tab).
>//
>FieldNameStr = String;
>FieldValueStr = AsciiStrGetNextToken (FieldNameStr, ':');
>if (FieldValueStr == NULL) {
>  return NULL;
>}
> 
>//
> -  // Replace ':' with 0
> +  // Replace ':' with 0, then FieldName has been retrived from String.
>//
>*(FieldValueStr - 1) = 0;
> 
>//
> -  // The field value MAY be preceded by any amount of LWS, though a
> single SP is preferred.
> -  // Note: LWS  = [CRLF] 1*(SP|HT), it can be '\r\n ' or '\r\n\t' or ' '
> or '\t'.
> -  //   CRLF = '\r\n'.
> -  //   SP   = ' '.
> -  //   HT   = '\t' (Tab).
> +  // Handle FieldValueStr, skip all the preceded LWS.
>//
>while (TRUE) {
>  if (*FieldValueStr == ' ' || *FieldValueStr == '\t') {
>//
>// Boundary condition check.
>//
>if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 1) {
> +//
> +// Wrong String format!
> +//
>  return NULL;
>}
> 
>FieldValueStr ++;
>  } else if (*FieldValueStr == '\r') {
>//
>// Boundary condition check.
>//
>if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 3) {
> -return NULL;
> +//
> +// No more preceded LWS, so break here.
> +//
> +break;
>}
> 
> -  if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' ||
> *(FieldValueStr + 2) == '\t')) {
> -FieldValueStr = FieldValueStr + 3;
> +  if (*(FieldValueStr + 1) == '\n' ) {
> +if (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t')
> {
> +  FieldValueStr = FieldValueStr + 3;
> +} else {
> +  //
> +  // No more preceded LWS, so break here.
> +  //
> +  break;
> +}
> +  } else {
> +//
> +// Wrong String format!
> +//
> +return NULL;
>}
>  } else {
> +  //
> +  // No more preceded LWS, so break here.
> +  //
>break;
>  }
>}
> 
> -  //
> -  // Header fields can be extended over multiple lines by preceding each
> extra
> -  // line with at least one SP or HT.
> -  //
>StrPtr = FieldValueStr;
>do {
> +//
> +// Handle the LWS within the field value.
> +//
>  StrPtr = AsciiStrGetNextToken (StrPtr, '\r');
>  if (StrPtr == NULL || *StrPtr != '\n') {
> +  //
> +  // Wrong String format!
> +  //
>return NULL;
>  }
> 
>  StrPtr++;
>} while (*StrPtr == ' ' || *StrPtr == '\t');
> --
> 2.17.1.windows.2

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

Re: [edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

2018-09-11 Thread Laszlo Ersek 
On 09/04/18 15:43, Stephen Benjamin wrote:
> On Tue, Sep 4, 2018 at 7:02 AM Laszlo Ersek  wrote:
>>
>> On 09/04/18 09:17, Jiaxin Wu wrote:
>>> This patch is to resolve the lock-up issue if the value of HTTP header
>>> is blank.  The issue is recorded @
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1102.
>>>
>>> Cc: Stephen Benjamin 
>>> Cc: Laszlo Ersek 
>>> Cc: Ye Ting 
>>> Cc: Fu Siyuan 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Wu Jiaxin 
>>> ---
>>>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
>>>  1 file changed, 44 insertions(+), 13 deletions(-)
>>
>> Thank you, Jiaxin, I'll let Stephen test this :)
>>
>> Stephen, can you please respond with your Tested-by, if the patch solves
>> the problem for you?
>>
>> Also, would you prefer a remote repo+branch that you could pull (over
>> applying this patch with "git-am") for testing? The edk2 project uses
>> CRLF source files, so "git-am" is a bit quirky.
> 
> No need, the patch applied fine. Thanks for the quick turnaround! It
> resolved my issue.
> 
> Tested-by: Stephen Benjamin 

Siyuan, Ting, can you guys please review Jiaxin's patch?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

2018-09-04 Thread Stephen Benjamin 
On Tue, Sep 4, 2018 at 7:02 AM Laszlo Ersek  wrote:
>
> On 09/04/18 09:17, Jiaxin Wu wrote:
> > This patch is to resolve the lock-up issue if the value of HTTP header
> > is blank.  The issue is recorded @
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1102.
> >
> > Cc: Stephen Benjamin 
> > Cc: Laszlo Ersek 
> > Cc: Ye Ting 
> > Cc: Fu Siyuan 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Wu Jiaxin 
> > ---
> >  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
> >  1 file changed, 44 insertions(+), 13 deletions(-)
>
> Thank you, Jiaxin, I'll let Stephen test this :)
>
> Stephen, can you please respond with your Tested-by, if the patch solves
> the problem for you?
>
> Also, would you prefer a remote repo+branch that you could pull (over
> applying this patch with "git-am") for testing? The edk2 project uses
> CRLF source files, so "git-am" is a bit quirky.

No need, the patch applied fine. Thanks for the quick turnaround! It
resolved my issue.

Tested-by: Stephen Benjamin 

> Thanks,
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

2018-09-04 Thread Laszlo Ersek 
On 09/04/18 09:17, Jiaxin Wu wrote:
> This patch is to resolve the lock-up issue if the value of HTTP header
> is blank.  The issue is recorded @
> https://bugzilla.tianocore.org/show_bug.cgi?id=1102.
> 
> Cc: Stephen Benjamin 
> Cc: Laszlo Ersek 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin 
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
>  1 file changed, 44 insertions(+), 13 deletions(-)

Thank you, Jiaxin, I'll let Stephen test this :)

Stephen, can you please respond with your Tested-by, if the patch solves
the problem for you?

Also, would you prefer a remote repo+branch that you could pull (over
applying this patch with "git-am") for testing? The edk2 project uses
CRLF source files, so "git-am" is a bit quirky.

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] MdeModulePkg/Library/DxeHttpLib: Handle the blank value in HTTP header.

2018-09-04 Thread Jiaxin Wu 
This patch is to resolve the lock-up issue if the value of HTTP header
is blank.  The issue is recorded @
https://bugzilla.tianocore.org/show_bug.cgi?id=1102.

Cc: Stephen Benjamin 
Cc: Laszlo Ersek 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin 
---
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 57 +++-
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 5fbb50d03a..2fc3da8a2d 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -1595,63 +1595,94 @@ HttpGetFieldNameAndValue (
 return NULL;
   }
 
   //
   // Each header field consists of a name followed by a colon (":") and the 
field value.
+  // The field value MAY be preceded by any amount of LWS, though a single SP 
is preferred.
+  //
+  // message-header = field-name ":" [ field-value ]
+  // field-name = token
+  // field-value = *( field-content | LWS )
+  //
+  // Note: "*(element)" allows any number element, including zero; 
"1*(element)" requires at least one element.
+  //   [element] means element is optional.
+  //   LWS  = [CRLF] 1*(SP|HT), it can be ' ' or '\t' or '\r\n ' or 
'\r\n\t'.
+  //   CRLF = '\r\n'.
+  //   SP   = ' '.
+  //   HT   = '\t' (Tab).
   //
   FieldNameStr = String;
   FieldValueStr = AsciiStrGetNextToken (FieldNameStr, ':');
   if (FieldValueStr == NULL) {
 return NULL;
   }
 
   //
-  // Replace ':' with 0
+  // Replace ':' with 0, then FieldName has been retrived from String.
   //
   *(FieldValueStr - 1) = 0;
 
   //
-  // The field value MAY be preceded by any amount of LWS, though a single SP 
is preferred.
-  // Note: LWS  = [CRLF] 1*(SP|HT), it can be '\r\n ' or '\r\n\t' or ' ' or 
'\t'.
-  //   CRLF = '\r\n'.
-  //   SP   = ' '.
-  //   HT   = '\t' (Tab).
+  // Handle FieldValueStr, skip all the preceded LWS.
   //
   while (TRUE) {
 if (*FieldValueStr == ' ' || *FieldValueStr == '\t') {
   //
   // Boundary condition check.
   //
   if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 1) {
+//
+// Wrong String format!
+//
 return NULL;
   }
 
   FieldValueStr ++;
 } else if (*FieldValueStr == '\r') {
   //
   // Boundary condition check.
   //
   if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 3) {
-return NULL;
+//
+// No more preceded LWS, so break here.
+//
+break;
   }
 
-  if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' || 
*(FieldValueStr + 2) == '\t')) {
-FieldValueStr = FieldValueStr + 3;
+  if (*(FieldValueStr + 1) == '\n' ) {
+if (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t') {
+  FieldValueStr = FieldValueStr + 3;
+} else {
+  //
+  // No more preceded LWS, so break here.
+  //
+  break;
+}
+  } else {
+//
+// Wrong String format!
+//
+return NULL;
   }
 } else {
+  //
+  // No more preceded LWS, so break here.
+  //
   break;
 }
   }
 
-  //
-  // Header fields can be extended over multiple lines by preceding each extra
-  // line with at least one SP or HT.
-  //
   StrPtr = FieldValueStr;
   do {
+//
+// Handle the LWS within the field value.
+//
 StrPtr = AsciiStrGetNextToken (StrPtr, '\r');
 if (StrPtr == NULL || *StrPtr != '\n') {
+  //
+  // Wrong String format!
+  //
   return NULL;
 }
 
 StrPtr++;
   } while (*StrPtr == ' ' || *StrPtr == '\t');
-- 
2.17.1.windows.2

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