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>; edk2-devel@lists.01.org Cc: Fu, Siyuan <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>; El-Haj-Mahmoud, Samer <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> 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 <jiaxin...@intel.com>; Fu, Siyuan > <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>; > 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 ( > - &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 https://lists.01.org/mailman/listinfo/edk2-devel
