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:[email protected]]
Sent: Thursday, May 05, 2016 11:16 AM
To: Hegde, Nagaraj P <[email protected]>; Wu, Jiaxin
<[email protected]>; [email protected]
Cc: Ye, Ting <[email protected]>; El-Haj-Mahmoud, Samer
<[email protected]>
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:[email protected]]
Sent: Thursday, May 5, 2016 11:48 AM
To: Wu, Jiaxin <[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>
Cc: Fu, Siyuan <[email protected]<mailto:[email protected]>>; Ye, Ting
<[email protected]<mailto:[email protected]>>; El-Haj-Mahmoud, Samer
<[email protected]<mailto:[email protected]>>
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:[email protected]]
Sent: Thursday, May 05, 2016 8:10 AM
To: Hegde, Nagaraj P <[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>
Cc: Fu, Siyuan <[email protected]<mailto:[email protected]>>; Ye, Ting
<[email protected]<mailto:[email protected]>>; El-Haj-Mahmoud, Samer
<[email protected]<mailto:[email protected]>>
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 <[email protected]<mailto:[email protected]>>
Thanks.
Jiaxin
> -----Original Message-----
> From: Nagaraj Hegde [mailto:[email protected]]
> Sent: Wednesday, May 4, 2016 5:33 PM
> To: [email protected]<mailto:[email protected]>
> Cc: Wu, Jiaxin <[email protected]<mailto:[email protected]>>; Fu, Siyuan
> <[email protected]<mailto:[email protected]>>; Ye, Ting
> <[email protected]<mailto:[email protected]>>;
> [email protected]<mailto:[email protected]>
> 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
> [email protected]<mailto:[email protected]>
> ---
> 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 (
> - &gEfiHttpUtilitiesProtocolGuid,
> - NULL,
> - (VOID **)&HttpUtilitiesProtocol
> - );
> -
> - 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 (
> + &gEfiHttpUtilitiesProtocolGuid,
> + NULL,
> + (VOID **)&HttpUtilitiesProtocol
> + );
> +
> + 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] = &Message->Headers[Index];
> + }
> +
> + //
> + // Build raw HTTP Headers
> + //
> + Status = HttpUtilitiesProtocol->Build (
> + HttpUtilitiesProtocol,
> + 0,
> + NULL,
> + 0,
> + NULL,
> + Message->HeaderCount,
> + AppendList,
> + &HttpHdrSize,
> + &HttpHdr
> + );
> +
> + 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] = &Message->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,
> - &HttpHdrSize,
> - &HttpHdr
> - );
> -
> - 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;
> -
> + MsgSize += Message->BodyLength;
>
> + //
> + // memory for the string that needs to be sent to TCP //
> *RequestMsg = AllocateZeroPool (MsgSize);
> if (*RequestMsg == NULL) {
> Status = EFI_OUT_OF_RESOURCES;
> @@ -1746,60 +1771,64 @@ HttpGenRequestMessage (
> //
> // Construct header request
> //
> - switch (Message->Data.Request->Method) {
> - case HttpMethodGet:
> - StrLength = sizeof (HTTP_METHOD_GET) - 1;
> - CopyMem (RequestPtr, HTTP_METHOD_GET, StrLength);
> - RequestPtr += StrLength;
> - break;
> - case HttpMethodPut:
> - StrLength = sizeof (HTTP_METHOD_PUT) - 1;
> - CopyMem (RequestPtr, HTTP_METHOD_PUT, StrLength);
> - RequestPtr += StrLength;
> - break;
> - case HttpMethodPatch:
> - StrLength = sizeof (HTTP_METHOD_PATCH) - 1;
> - CopyMem (RequestPtr, HTTP_METHOD_PATCH, StrLength);
> - RequestPtr += StrLength;
> - break;
> - case HttpMethodPost:
> - StrLength = sizeof (HTTP_METHOD_POST) - 1;
> - CopyMem (RequestPtr, HTTP_METHOD_POST, StrLength);
> - RequestPtr += StrLength;
> - break;
> - case HttpMethodHead:
> - StrLength = sizeof (HTTP_METHOD_HEAD) - 1;
> - CopyMem (RequestPtr, HTTP_METHOD_HEAD, StrLength);
> - RequestPtr += StrLength;
> - break;
> - case HttpMethodDelete:
> - StrLength = sizeof (HTTP_METHOD_DELETE) - 1;
> - CopyMem (RequestPtr, HTTP_METHOD_DELETE, StrLength);
> - RequestPtr += StrLength;
> - break;
> - default:
> - ASSERT (FALSE);
> - Status = EFI_INVALID_PARAMETER;
> - goto Exit;
> - }
> -
> - StrLength = AsciiStrLen(EMPTY_SPACE);
> - CopyMem (RequestPtr, EMPTY_SPACE, StrLength);
> - RequestPtr += StrLength;
> -
> - StrLength = AsciiStrLen (Url);
> - CopyMem (RequestPtr, Url, StrLength);
> - RequestPtr += StrLength;
> -
> - StrLength = sizeof (HTTP_VERSION_CRLF_STR) - 1;
> - CopyMem (RequestPtr, HTTP_VERSION_CRLF_STR, StrLength);
> - RequestPtr += StrLength;
> -
> - //
> - // Construct header
> - //
> - CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
> - RequestPtr += HttpHdrSize;
> + if (Message->Data.Request != NULL) {
> + switch (Message->Data.Request->Method) {
> + case HttpMethodGet:
> + StrLength = sizeof (HTTP_METHOD_GET) - 1;
> + CopyMem (RequestPtr, HTTP_METHOD_GET, StrLength);
> + RequestPtr += StrLength;
> + break;
> + case HttpMethodPut:
> + StrLength = sizeof (HTTP_METHOD_PUT) - 1;
> + CopyMem (RequestPtr, HTTP_METHOD_PUT, StrLength);
> + RequestPtr += StrLength;
> + break;
> + case HttpMethodPatch:
> + StrLength = sizeof (HTTP_METHOD_PATCH) - 1;
> + CopyMem (RequestPtr, HTTP_METHOD_PATCH, StrLength);
> + RequestPtr += StrLength;
> + break;
> + case HttpMethodPost:
> + StrLength = sizeof (HTTP_METHOD_POST) - 1;
> + CopyMem (RequestPtr, HTTP_METHOD_POST, StrLength);
> + RequestPtr += StrLength;
> + break;
> + case HttpMethodHead:
> + StrLength = sizeof (HTTP_METHOD_HEAD) - 1;
> + CopyMem (RequestPtr, HTTP_METHOD_HEAD, StrLength);
> + RequestPtr += StrLength;
> + break;
> + case HttpMethodDelete:
> + StrLength = sizeof (HTTP_METHOD_DELETE) - 1;
> + CopyMem (RequestPtr, HTTP_METHOD_DELETE, StrLength);
> + RequestPtr += StrLength;
> + break;
> + default:
> + ASSERT (FALSE);
> + Status = EFI_INVALID_PARAMETER;
> + goto Exit;
> + }
> +
> + StrLength = AsciiStrLen(EMPTY_SPACE);
> + CopyMem (RequestPtr, EMPTY_SPACE, StrLength);
> + RequestPtr += StrLength;
> +
> + StrLength = AsciiStrLen (Url);
> + CopyMem (RequestPtr, Url, StrLength);
> + RequestPtr += StrLength;
> +
> + StrLength = sizeof (HTTP_VERSION_CRLF_STR) - 1;
> + CopyMem (RequestPtr, HTTP_VERSION_CRLF_STR, StrLength);
> + RequestPtr += StrLength;
> +
> + if (HttpHdr != NULL) {
> + //
> + // Construct header
> + //
> + CopyMem (RequestPtr, HttpHdr, HttpHdrSize);
> + RequestPtr += HttpHdrSize;
> + }
> + }
>
> //
> // Construct body
> --
> 2.6.2.windows.1
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
