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:[email protected]]
> Sent: Thursday, May 5, 2016 11:48 AM
> To: Wu, Jiaxin <[email protected]>; [email protected]
> Cc: Fu, Siyuan <[email protected]>; Ye, Ting <[email protected]>; El-Haj-
> Mahmoud, Samer <[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]>; [email protected]
> Cc: Fu, Siyuan <[email protected]>; Ye, Ting <[email protected]>; El-Haj-
> Mahmoud, Samer <[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]>
>
> Thanks.
> Jiaxin
>
> > -----Original Message-----
> > From: Nagaraj Hegde [mailto:[email protected]]
> > Sent: Wednesday, May 4, 2016 5:33 PM
> > To: [email protected]
> > Cc: Wu, Jiaxin <[email protected]>; Fu, Siyuan
> > <[email protected]>; Ye, Ting <[email protected]>;
> > [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]
> > ---
> > 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
