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...@hpe.com] > Sent: Wednesday, May 4, 2016 5:33 PM > To: > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>> > Cc: Wu, Jiaxin > <jiaxin...@intel.com<mailto:jiaxin...@intel.com<mailto:jiaxin...@intel.com%3cmailto:jiaxin...@intel.com>>>; > 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>>>; > 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: [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<mailto:nagaraj-p.he...@hpe.com<mailto:nagaraj-p.he...@hpe.com%3cmailto: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 ( > - &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 edk2-devel@lists.01.org<mailto: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
