Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

[Fu, Siyuan]

Nagaraj,

The check is correct but incomplete, the Request and HeaderCount should both 
zero or both not zero.

Best Regards,
Siyuan


From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Hegde, 
Nagaraj P
Sent: Thursday, May 5, 2016 2:13 PM
To: Fu, Siyuan <siyuan...@intel.com>; Wu, Jiaxin <jiaxin...@intel.com>; 
edk2-devel@lists.01.org
Cc: Ye, Ting <ting...@intel.com>
Subject: Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

As Jiaxin pointed out in his reply,


if ((Message->Data.Request != NULL && Url == NULL) || (Message->Data.Request != 
NULL && Message->HeaderCount == 0)) {

return EFI_INVALID_PARAMETER;
}

looks to be a meaningful check to be in place. I will send a v2 patch.

Regards,
Nagaraj.

From: Fu, Siyuan [mailto:siyuan...@intel.com]
Sent: Thursday, May 05, 2016 11:16 AM
To: Hegde, Nagaraj P <nagaraj-p.he...@hpe.com<mailto:nagaraj-p.he...@hpe.com>>; 
Wu, Jiaxin <jiaxin...@intel.com<mailto:jiaxin...@intel.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ye, Ting <ting...@intel.com<mailto:ting...@intel.com>>; El-Haj-Mahmoud, 
Samer <samer.el-haj-mahm...@hpe.com<mailto:samer.el-haj-mahm...@hpe.com>>
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Nagaraj,

You are right, the HTTP 1.1 requests the client to put at least a Host header 
in all request messages, that means the Request and Header should be either 
both non-zero for a new request message, or bother zero for a subsequent 
message body. So I vote for the EFI_INVALID_PARAMETER, if the input parameters 
not follow this rule.

Best Regards,
Siyuan



From: Hegde, Nagaraj P [mailto:nagaraj-p.he...@hpe.com]
Sent: Thursday, May 5, 2016 11:48 AM
To: Wu, Jiaxin 
<jiaxin...@intel.com<mailto:jiaxin...@intel.com<mailto:jiaxin...@intel.com%3cmailto:jiaxin...@intel.com>>>;
 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
Cc: Fu, Siyuan 
<siyuan...@intel.com<mailto:siyuan...@intel.com<mailto:siyuan...@intel.com%3cmailto:siyuan...@intel.com>>>;
 Ye, Ting 
<ting...@intel.com<mailto:ting...@intel.com<mailto:ting...@intel.com%3cmailto:ting...@intel.com>>>;
 El-Haj-Mahmoud, Samer 
<samer.el-haj-mahm...@hpe.com<mailto:samer.el-haj-mahm...@hpe.com<mailto:samer.el-haj-mahm...@hpe.com%3cmailto:samer.el-haj-mahm...@hpe.com>>>
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi Jiaxin,

I believe in HTTP 1.1, servers do not accept Request methods without any 
header. In other words, "Host:" header is a must for all request methods. That 
was the reason I planned to put the construction of header parts inside the "if 
(Message->Data.Request != NULL" condition. Now, I wonder if we need to add this 
condition in the entry of this function itself and return a 
EFI_INVALID_PARAMETER or allow it to go to HTTP server and return back as "400 
Bad Request". In case we allow, I see an issue. We would miss sending the 
additional "\r\n" which would mark the end of request and headers (if any). 
Adding additional "\r\n" is embedded into HttpUtilities Build function.

Regards,
Nagaraj.

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
Sent: Thursday, May 05, 2016 8:10 AM
To: Hegde, Nagaraj P 
<nagaraj-p.he...@hpe.com<mailto:nagaraj-p.he...@hpe.com<mailto:nagaraj-p.he...@hpe.com%3cmailto:nagaraj-p.he...@hpe.com>>>;
 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
Cc: Fu, Siyuan 
<siyuan...@intel.com<mailto:siyuan...@intel.com<mailto:siyuan...@intel.com%3cmailto:siyuan...@intel.com>>>;
 Ye, Ting 
<ting...@intel.com<mailto:ting...@intel.com<mailto:ting...@intel.com%3cmailto:ting...@intel.com>>>;
 El-Haj-Mahmoud, Samer 
<samer.el-haj-mahm...@hpe.com<mailto:samer.el-haj-mahm...@hpe.com<mailto:samer.el-haj-mahm...@hpe.com%3cmailto:samer.el-haj-mahm...@hpe.com>>>
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi  Nagaraj,

Constructing header parts seems should be outside the conditional of "if 
(Message->Data.Request != NULL)".

if (HttpHdr != NULL) {
  //
  // Construct header
  //
  CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
  RequestPtr += HttpHdrSize;
}

Others is good to me.

Reviewed-By: Wu Jiaxin 
<jiaxin...@intel.com<mailto:jiaxin...@intel.com<mailto:jiaxin...@intel.com%3cmailto:jiaxin...@intel.com>>>

Thanks.
Jiaxin

> -Original Message-
> From: Nagaraj Hegde [mailto:nagaraj-p.he...@h

Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

[Hegde, Nagaraj P]

As Jiaxin pointed out in his reply,


if ((Message->Data.Request != NULL && Url == NULL) || (Message->Data.Request != 
NULL && Message->HeaderCount == 0)) {

return EFI_INVALID_PARAMETER;
}

looks to be a meaningful check to be in place. I will send a v2 patch.

Regards,
Nagaraj.

From: Fu, Siyuan [mailto:siyuan...@intel.com]
Sent: Thursday, May 05, 2016 11:16 AM
To: Hegde, Nagaraj P ; Wu, Jiaxin 
; edk2-devel@lists.01.org
Cc: Ye, Ting ; El-Haj-Mahmoud, Samer 

Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Nagaraj,

You are right, the HTTP 1.1 requests the client to put at least a Host header 
in all request messages, that means the Request and Header should be either 
both non-zero for a new request message, or bother zero for a subsequent 
message body. So I vote for the EFI_INVALID_PARAMETER, if the input parameters 
not follow this rule.

Best Regards,
Siyuan



From: Hegde, Nagaraj P [mailto:nagaraj-p.he...@hpe.com]
Sent: Thursday, May 5, 2016 11:48 AM
To: Wu, Jiaxin >; 
edk2-devel@lists.01.org
Cc: Fu, Siyuan >; Ye, Ting 
>; El-Haj-Mahmoud, Samer 
>
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi Jiaxin,

I believe in HTTP 1.1, servers do not accept Request methods without any 
header. In other words, "Host:" header is a must for all request methods. That 
was the reason I planned to put the construction of header parts inside the "if 
(Message->Data.Request != NULL" condition. Now, I wonder if we need to add this 
condition in the entry of this function itself and return a 
EFI_INVALID_PARAMETER or allow it to go to HTTP server and return back as "400 
Bad Request". In case we allow, I see an issue. We would miss sending the 
additional "\r\n" which would mark the end of request and headers (if any). 
Adding additional "\r\n" is embedded into HttpUtilities Build function.

Regards,
Nagaraj.

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
Sent: Thursday, May 05, 2016 8:10 AM
To: Hegde, Nagaraj P >; 
edk2-devel@lists.01.org
Cc: Fu, Siyuan >; Ye, Ting 
>; El-Haj-Mahmoud, Samer 
>
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi  Nagaraj,

Constructing header parts seems should be outside the conditional of "if 
(Message->Data.Request != NULL)".

if (HttpHdr != NULL) {
  //
  // Construct header
  //
  CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
  RequestPtr += HttpHdrSize;
}

Others is good to me.

Reviewed-By: Wu Jiaxin >

Thanks.
Jiaxin

> -Original Message-
> From: Nagaraj Hegde [mailto:nagaraj-p.he...@hpe.com]
> Sent: Wednesday, May 4, 2016 5:33 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin >; Fu, Siyuan
> >; Ye, Ting 
> >;
> samer.el-haj-mahm...@hpe.com
> Subject: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
>
> HttpGenRequestMessage assumes that HTTP message would always contain a
> request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests would
> contain just the message body. This patch supports creation of such
> request messages with additional checks.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P 
> nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 219 +++---
> --
>  1 file changed, 124 insertions(+), 95 deletions(-)
>
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..fa0f591 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,86 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
>
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // If we have a Request, we cannot have a NULL Url
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG 

Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

[Fu, Siyuan]

Nagaraj,

You are right, the HTTP 1.1 requests the client to put at least a Host header 
in all request messages, that means the Request and Header should be either 
both non-zero for a new request message, or bother zero for a subsequent 
message body. So I vote for the EFI_INVALID_PARAMETER, if the input parameters 
not follow this rule.

Best Regards,
Siyuan



From: Hegde, Nagaraj P [mailto:nagaraj-p.he...@hpe.com]
Sent: Thursday, May 5, 2016 11:48 AM
To: Wu, Jiaxin ; edk2-devel@lists.01.org
Cc: Fu, Siyuan ; Ye, Ting ; 
El-Haj-Mahmoud, Samer 
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi Jiaxin,

I believe in HTTP 1.1, servers do not accept Request methods without any 
header. In other words, "Host:" header is a must for all request methods. That 
was the reason I planned to put the construction of header parts inside the "if 
(Message->Data.Request != NULL" condition. Now, I wonder if we need to add this 
condition in the entry of this function itself and return a 
EFI_INVALID_PARAMETER or allow it to go to HTTP server and return back as "400 
Bad Request". In case we allow, I see an issue. We would miss sending the 
additional "\r\n" which would mark the end of request and headers (if any). 
Adding additional "\r\n" is embedded into HttpUtilities Build function.

Regards,
Nagaraj.

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
Sent: Thursday, May 05, 2016 8:10 AM
To: Hegde, Nagaraj P >; 
edk2-devel@lists.01.org
Cc: Fu, Siyuan >; Ye, Ting 
>; El-Haj-Mahmoud, Samer 
>
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi  Nagaraj,

Constructing header parts seems should be outside the conditional of "if 
(Message->Data.Request != NULL)".

if (HttpHdr != NULL) {
  //
  // Construct header
  //
  CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
  RequestPtr += HttpHdrSize;
}

Others is good to me.

Reviewed-By: Wu Jiaxin >

Thanks.
Jiaxin

> -Original Message-
> From: Nagaraj Hegde [mailto:nagaraj-p.he...@hpe.com]
> Sent: Wednesday, May 4, 2016 5:33 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin >; Fu, Siyuan
> >; Ye, Ting 
> >;
> samer.el-haj-mahm...@hpe.com
> Subject: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
>
> HttpGenRequestMessage assumes that HTTP message would always contain a
> request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests would
> contain just the message body. This patch supports creation of such
> request messages with additional checks.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P 
> nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 219 +++---
> --
>  1 file changed, 124 insertions(+), 95 deletions(-)
>
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..fa0f591 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,86 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
>
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // If we have a Request, we cannot have a NULL Url
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> -return Status;
> +  if (Message->Data.Request != NULL && Url == NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Message->HeaderCount != 0) {
> +//
> +// Locate the HTTP_UTILITIES protocol.
> +//
> +Status = gBS->LocateProtocol (
> +,
> +NULL,
> +(VOID **)
> +);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol.
> + Status
> = %r.\n", Status));
> +  return Status;
> +}
> +
> +//
> +// Build AppendList to send into HttpUtilitiesBuild
> +//
> +AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) *
> + (Message-
> 

Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

[Wu, Jiaxin]

Yeah, according UEFI Spec, in my understanding, only two extra cases are 
allowed:
A. Body is not NULL and BodyLength is non-zero and all other fields are NULL or 
0 (POST and PUT methods, depending on the size of the data)
B. Body is NULL and BodyLength is 0 and all other fields are not NULL or 
non-zero (Not POST or PUT method)

If so, keep this condition inside is ok, how about we update the below piece 
code:  
  "if (Message->Data.Request != NULL && Url == NULL) {
return EFI_INVALID_PARAMETER;
  }" 
To:
  "if ((Message->Data.Request != NULL && Url == NULL) || (Message->Data.Request 
!= NULL && Message->HeaderCount == 0)) {
return EFI_INVALID_PARAMETER;
  }"

Thanks.
Jiaxin

> -Original Message-
> From: Hegde, Nagaraj P [mailto:nagaraj-p.he...@hpe.com]
> Sent: Thursday, May 5, 2016 11:48 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Fu, Siyuan ; Ye, Ting ; El-Haj-
> Mahmoud, Samer 
> Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
> 
> Hi Jiaxin,
> 
> I believe in HTTP 1.1, servers do not accept Request methods without any
> header. In other words, "Host:" header is a must for all request methods.
> That was the reason I planned to put the construction of header parts inside
> the "if (Message->Data.Request != NULL" condition. Now, I wonder if we
> need to add this condition in the entry of this function itself and return a
> EFI_INVALID_PARAMETER or allow it to go to HTTP server and return back as
> "400 Bad Request". In case we allow, I see an issue. We would miss sending
> the additional "\r\n" which would mark the end of request and headers (if
> any). Adding additional "\r\n" is embedded into HttpUtilities Build function.
> 
> Regards,
> Nagaraj.
> 
> -Original Message-
> From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
> Sent: Thursday, May 05, 2016 8:10 AM
> To: Hegde, Nagaraj P ; edk2-devel@lists.01.org
> Cc: Fu, Siyuan ; Ye, Ting ; El-Haj-
> Mahmoud, Samer 
> Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
> 
> Hi  Nagaraj,
> 
> Constructing header parts seems should be outside the conditional of "if
> (Message->Data.Request != NULL)".
> 
> if (HttpHdr != NULL) {
>   //
>   // Construct header
>   //
>   CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
>   RequestPtr += HttpHdrSize;
> }
> 
> Others is good to me.
> 
> Reviewed-By: Wu Jiaxin 
> 
> Thanks.
> Jiaxin
> 
> > -Original Message-
> > From: Nagaraj Hegde [mailto:nagaraj-p.he...@hpe.com]
> > Sent: Wednesday, May 4, 2016 5:33 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Jiaxin ; Fu, Siyuan
> > ; Ye, Ting ;
> > samer.el-haj-mahm...@hpe.com
> > Subject: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> > HttpGenRequestMessage API
> >
> > HttpGenRequestMessage assumes that HTTP message would always
> contain a
> > request-line, headers and an optional message body.
> > However, subsequent to a HTTP PUT/POST request, HTTP requests would
> > contain just the message body. This patch supports creation of such
> > request messages with additional checks.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
> > ---
> >  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 219 +++-
> --
> > --
> >  1 file changed, 124 insertions(+), 95 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > index 8d61d0b..fa0f591 100644
> > --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > @@ -1681,61 +1681,86 @@ HttpGenRequestMessage (
> >HttpUtilitiesProtocol = NULL;
> >
> >//
> > -  // Locate the HTTP_UTILITIES protocol.
> > +  // If we have a Request, we cannot have a NULL Url
> >//
> > -  Status = gBS->LocateProtocol (
> > -  ,
> > -  NULL,
> > -  (VOID **)
> > -  );
> > -
> > -  if (EFI_ERROR (Status)) {
> > -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> > = %r.\n", Status));
> > -return Status;
> > +  if (Message->Data.Request != NULL && Url == NULL) {
> > +return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (Message->HeaderCount != 0) {
> > +//
> > +// Locate the HTTP_UTILITIES protocol.
> > +//
> > +Status = gBS->LocateProtocol (
> > +,
> > +NULL,
> > +(VOID **)
> > +);
> > +
> > +if (EFI_ERROR (Status)) {
> > +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol.
> > + Status
> > = %r.\n", 

Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

[Hegde, Nagaraj P]

Hi Jiaxin,

I believe in HTTP 1.1, servers do not accept Request methods without any 
header. In other words, "Host:" header is a must for all request methods. That 
was the reason I planned to put the construction of header parts inside the "if 
(Message->Data.Request != NULL" condition. Now, I wonder if we need to add this 
condition in the entry of this function itself and return a 
EFI_INVALID_PARAMETER or allow it to go to HTTP server and return back as "400 
Bad Request". In case we allow, I see an issue. We would miss sending the 
additional "\r\n" which would mark the end of request and headers (if any). 
Adding additional "\r\n" is embedded into HttpUtilities Build function.

Regards,
Nagaraj.

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com] 
Sent: Thursday, May 05, 2016 8:10 AM
To: Hegde, Nagaraj P ; edk2-devel@lists.01.org
Cc: Fu, Siyuan ; Ye, Ting ; 
El-Haj-Mahmoud, Samer 
Subject: RE: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

Hi  Nagaraj,

Constructing header parts seems should be outside the conditional of "if 
(Message->Data.Request != NULL)".

if (HttpHdr != NULL) {
  //
  // Construct header
  //
  CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
  RequestPtr += HttpHdrSize;
}

Others is good to me.

Reviewed-By: Wu Jiaxin 

Thanks.
Jiaxin

> -Original Message-
> From: Nagaraj Hegde [mailto:nagaraj-p.he...@hpe.com]
> Sent: Wednesday, May 4, 2016 5:33 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Fu, Siyuan 
> ; Ye, Ting ; 
> samer.el-haj-mahm...@hpe.com
> Subject: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
> HttpGenRequestMessage API
> 
> HttpGenRequestMessage assumes that HTTP message would always contain a 
> request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests would 
> contain just the message body. This patch supports creation of such 
> request messages with additional checks.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 219 +++---
> --
>  1 file changed, 124 insertions(+), 95 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..fa0f591 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,86 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
> 
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // If we have a Request, we cannot have a NULL Url
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> -return Status;
> +  if (Message->Data.Request != NULL && Url == NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Message->HeaderCount != 0) {
> +//
> +// Locate the HTTP_UTILITIES protocol.
> +//
> +Status = gBS->LocateProtocol (
> +,
> +NULL,
> +(VOID **)
> +);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. 
> + Status
> = %r.\n", Status));
> +  return Status;
> +}
> +
> +//
> +// Build AppendList to send into HttpUtilitiesBuild
> +//
> +AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * 
> + (Message-
> >HeaderCount));
> +if (AppendList == NULL) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +
> +for(Index = 0; Index < Message->HeaderCount; Index++){
> +  AppendList[Index] = >Headers[Index];
> +}
> +
> +//
> +// Build raw HTTP Headers
> +//
> +Status = HttpUtilitiesProtocol->Build (
> +HttpUtilitiesProtocol,
> +0,
> +NULL,
> +0,
> +NULL,
> +Message->HeaderCount,
> +AppendList,
> +,
> +
> +);
> +
> +if (AppendList != NULL) {
> +  FreePool (AppendList);
> +}
> +
> +if (EFI_ERROR (Status) || HttpHdr == NULL){
> +  return Status;
> +}
>}
> 
>//
> -  // Build AppendList to send into HttpUtilitiesBuild
> +  // If we have headers to be sent, account for it.
>//
> -  AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * 
> (Message-
> >HeaderCount));
> -  if (AppendList == NULL) {
> -return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  for(Index = 0; Index < 

Re: [edk2] [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

[Wu, Jiaxin]

Hi  Nagaraj,

Constructing header parts seems should be outside the conditional of "if 
(Message->Data.Request != NULL)".

if (HttpHdr != NULL) {
  //
  // Construct header
  //
  CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
  RequestPtr += HttpHdrSize;
}

Others is good to me.

Reviewed-By: Wu Jiaxin 

Thanks.
Jiaxin

> -Original Message-
> From: Nagaraj Hegde [mailto:nagaraj-p.he...@hpe.com]
> Sent: Wednesday, May 4, 2016 5:33 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Fu, Siyuan ; Ye,
> Ting ; samer.el-haj-mahm...@hpe.com
> Subject: [PATCH v1 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
> 
> HttpGenRequestMessage assumes that HTTP message would always contain
> a request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests would
> contain just the message body. This patch supports creation of such request
> messages with additional checks.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 219 +++---
> --
>  1 file changed, 124 insertions(+), 95 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..fa0f591 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,86 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
> 
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // If we have a Request, we cannot have a NULL Url
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> -return Status;
> +  if (Message->Data.Request != NULL && Url == NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Message->HeaderCount != 0) {
> +//
> +// Locate the HTTP_UTILITIES protocol.
> +//
> +Status = gBS->LocateProtocol (
> +,
> +NULL,
> +(VOID **)
> +);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> +  return Status;
> +}
> +
> +//
> +// Build AppendList to send into HttpUtilitiesBuild
> +//
> +AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * (Message-
> >HeaderCount));
> +if (AppendList == NULL) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +
> +for(Index = 0; Index < Message->HeaderCount; Index++){
> +  AppendList[Index] = >Headers[Index];
> +}
> +
> +//
> +// Build raw HTTP Headers
> +//
> +Status = HttpUtilitiesProtocol->Build (
> +HttpUtilitiesProtocol,
> +0,
> +NULL,
> +0,
> +NULL,
> +Message->HeaderCount,
> +AppendList,
> +,
> +
> +);
> +
> +if (AppendList != NULL) {
> +  FreePool (AppendList);
> +}
> +
> +if (EFI_ERROR (Status) || HttpHdr == NULL){
> +  return Status;
> +}
>}
> 
>//
> -  // Build AppendList to send into HttpUtilitiesBuild
> +  // If we have headers to be sent, account for it.
>//
> -  AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * (Message-
> >HeaderCount));
> -  if (AppendList == NULL) {
> -return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  for(Index = 0; Index < Message->HeaderCount; Index++){
> -AppendList[Index] = >Headers[Index];
> +  if (Message->HeaderCount != 0) {
> +MsgSize = HttpHdrSize;
>}
> 
>//
> -  // Build raw HTTP Headers
> +  // If we have a request line, account for the fields.
>//
> -  Status = HttpUtilitiesProtocol->Build (
> -  HttpUtilitiesProtocol,
> -  0,
> -  NULL,
> -  0,
> -  NULL,
> -  Message->HeaderCount,
> -  AppendList,
> -  ,
> -  
> -  );
> -
> -  if (AppendList != NULL) {
> -FreePool (AppendList);
> -  }
> -
> -  if (EFI_ERROR (Status) || HttpHdr == NULL){
> -return Status;
> +  if (Message->Data.Request != NULL) {
> +MsgSize += HTTP_METHOD_MAXIMUM_LEN + AsciiStrLen
> + (HTTP_VERSION_CRLF_STR) + AsciiStrLen (Url);
>}
> 
> +
>//
> -  // Calculate HTTP message length.
> +  // If we have a message body to be sent, account for it.
>//
> -  MsgSize = Message->BodyLength + HTTP_METHOD_MAXIMUM_LEN +
> AsciiStrLen (Url) +
> -AsciiStrLen (HTTP_VERSION_CRLF_STR) + HttpHdrSize;
> -
> +