[edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

[edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

[Andrei Warkentin]

Python http.server seems to FIN after the first HEAD request to size
the loaded file is completed and ACKed. What happens next is interesting.
On low latency connections, the GET request to download may get sent
after the server sends the FIN but before the client has a chance to
process it. The net result is:
- Server ignores GET
- HttpBootLoadFile returns EFI_CONNECTION_FIN. Boot fails.

In the other case, client handles the FIN before attempting the GET,
so there's a proper three-way handshake as part of GET.

The solution is to retry HttpBootLoadFile 2 times if it returns
EFI_CONNECTION_FIN. This is because HttpBootLoadFile may issue up to
3 requests: HEAD/GET to get size and final GET to load. Some servers
may send a FIN after each request. The first request (HEAD) is not
supposed to fail this way, so only the two subsequent GET request
may result in a FIN.

Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=2720

Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
---
 NetworkPkg/HttpBootDxe/HttpBootImpl.c | 27 +++++++++++++++++++-
 NetworkPkg/HttpDxe/HttpImpl.c         |  5 ++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
index 4a51f35cdd..2d74d5f293 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
@@ -288,6 +288,7 @@ HttpBootDhcp (
   @retval EFI_NOT_STARTED             The driver is in stopped state.
   @retval EFI_BUFFER_TOO_SMALL        The BufferSize is too small to read the boot file. BufferSize has
                                       been updated with the size needed to complete the request.
+  @retval EFI_CONNECTION_FIN          Server had closed the connection while we were waiting downloading.
   @retval EFI_DEVICE_ERROR            An unexpected network error occurred.
   @retval Others                      Other errors as indicated.
 
@@ -535,6 +536,7 @@ HttpBootStop (
   @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current directory entry.
                                 BufferSize has been updated with the size needed to complete
                                 the request.
+  @retval EFI_CONNECTION_FIN    Server had closed the connection while we were waiting for a response.
 
 **/
 EFI_STATUS
@@ -553,6 +555,7 @@ HttpBootDxeLoadFile (
   BOOLEAN                       UsingIpv6;
   EFI_STATUS                    Status;
   HTTP_BOOT_IMAGE_TYPE          ImageType;
+  UINTN                         MaxTries;
 
   if (This == NULL || BufferSize == NULL || FilePath == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -598,7 +601,29 @@ HttpBootDxeLoadFile (
   // Load the boot file.
   //
   ImageType = ImageTypeMax;
-  Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
+  //
+  // HttpBootLoadFile may issue up to 2 requests: HEAD/GET to get
+  // size and final GET to load. Some servers may send a FIN after
+  // each request. The first request (HEAD) is not supposed to
+  // fail this way, so only the two possible GETs need the special
+  // handling.
+  //
+  MaxTries = 2;
+  do {
+    Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
+    if (Status == EFI_CONNECTION_FIN) {
+      if (Private->HttpCreated) {
+        //
+        // Tear down HTTP/TCP state entirely. Http->Configure (NULL) is not
+        // sufficient (EFI_ACCESS_DENIED from TCP stack on subsequent
+        // HttpBootLoadFile.
+        //
+        HttpIoDestroyIo (&Private->HttpIo);
+        Private->HttpCreated = FALSE;
+      }
+    }
+  } while (MaxTries-- && Status == EFI_CONNECTION_FIN);
+
   if (EFI_ERROR (Status)) {
     if (Status == EFI_BUFFER_TOO_SMALL && (ImageType == ImageTypeVirtualCd || ImageType == ImageTypeVirtualDisk)) {
       Status = EFI_WARN_FILE_SYSTEM;
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 5a6ecbc9d9..34a33b09f7 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -959,6 +959,8 @@ HttpBodyParserCallback (
   @retval EFI_OUT_OF_RESOURCES    Failed to complete the operation due to lack of resources.
   @retval EFI_NOT_READY           Can't find a corresponding Tx4Token/Tx6Token or
                                   the EFI_HTTP_UTILITIES_PROTOCOL is not available.
+  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
+                                  a response.
 
 **/
 EFI_STATUS
@@ -1528,6 +1530,9 @@ Error:
   @retval EFI_OUT_OF_RESOURCES    Could not allocate enough system resources.
   @retval EFI_ACCESS_DENIED       An open TCP connection is not present with the host
                                   specified by response URL.
+
+  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
+                                  a response.
 **/
 EFI_STATUS
 EFIAPI
-- 
2.17.1

Re: [edk2-devel] [edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

[Andrei Warkentin]

[-- Attachment #1: Type: text/plain, Size: 6043 bytes --]

Anyone?

FWIW I've been doing at least 20 boots a day with this, 200-300 MiB at a time. Super stable.

This is also tracked now https://bugzilla.tianocore.org/show_bug.cgi?id=2720

A
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Andrei Warkentin via groups.io <andrey.warkentin=gmail.com@groups.io>
Sent: Thursday, May 14, 2020 2:02 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: maciej.rabeda@linux.intel.com <maciej.rabeda@linux.intel.com>; jiaxin.wu@intel.com <jiaxin.wu@intel.com>; siyuan.fu@intel.com <siyuan.fu@intel.com>
Subject: [edk2-devel] [edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

Python http.server seems to FIN after the first HEAD request to size
the loaded file is completed and ACKed. What happens next is interesting.
On low latency connections, the GET request to download may get sent
after the server sends the FIN but before the client has a chance to
process it. The net result is:
- Server ignores GET
- HttpBootLoadFile returns EFI_CONNECTION_FIN. Boot fails.

In the other case, client handles the FIN before attempting the GET,
so there's a proper three-way handshake as part of GET.

The solution is to retry HttpBootLoadFile 2 times if it returns
EFI_CONNECTION_FIN. This is because HttpBootLoadFile may issue up to
3 requests: HEAD/GET to get size and final GET to load. Some servers
may send a FIN after each request. The first request (HEAD) is not
supposed to fail this way, so only the two subsequent GET request
may result in a FIN.

Fixes https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2720&amp;data=02%7C01%7Cawarkentin%40vmware.com%7C73254836459946b2441e08d7f7d4c742%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637250365557622212&amp;sdata=HOKwm430VLz1xB9yyrF3T7XKgYipWy5%2FH%2BKbWuIhm24%3D&amp;reserved=0

Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
---
 NetworkPkg/HttpBootDxe/HttpBootImpl.c | 27 +++++++++++++++++++-
 NetworkPkg/HttpDxe/HttpImpl.c         |  5 ++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
index 4a51f35cdd..2d74d5f293 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
@@ -288,6 +288,7 @@ HttpBootDhcp (
   @retval EFI_NOT_STARTED             The driver is in stopped state.
   @retval EFI_BUFFER_TOO_SMALL        The BufferSize is too small to read the boot file. BufferSize has
                                       been updated with the size needed to complete the request.
+  @retval EFI_CONNECTION_FIN          Server had closed the connection while we were waiting downloading.
   @retval EFI_DEVICE_ERROR            An unexpected network error occurred.
   @retval Others                      Other errors as indicated.

@@ -535,6 +536,7 @@ HttpBootStop (
   @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current directory entry.
                                 BufferSize has been updated with the size needed to complete
                                 the request.
+  @retval EFI_CONNECTION_FIN    Server had closed the connection while we were waiting for a response.

 **/
 EFI_STATUS
@@ -553,6 +555,7 @@ HttpBootDxeLoadFile (
   BOOLEAN                       UsingIpv6;
   EFI_STATUS                    Status;
   HTTP_BOOT_IMAGE_TYPE          ImageType;
+  UINTN                         MaxTries;

   if (This == NULL || BufferSize == NULL || FilePath == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -598,7 +601,29 @@ HttpBootDxeLoadFile (
   // Load the boot file.
   //
   ImageType = ImageTypeMax;
-  Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
+  //
+  // HttpBootLoadFile may issue up to 2 requests: HEAD/GET to get
+  // size and final GET to load. Some servers may send a FIN after
+  // each request. The first request (HEAD) is not supposed to
+  // fail this way, so only the two possible GETs need the special
+  // handling.
+  //
+  MaxTries = 2;
+  do {
+    Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
+    if (Status == EFI_CONNECTION_FIN) {
+      if (Private->HttpCreated) {
+        //
+        // Tear down HTTP/TCP state entirely. Http->Configure (NULL) is not
+        // sufficient (EFI_ACCESS_DENIED from TCP stack on subsequent
+        // HttpBootLoadFile.
+        //
+        HttpIoDestroyIo (&Private->HttpIo);
+        Private->HttpCreated = FALSE;
+      }
+    }
+  } while (MaxTries-- && Status == EFI_CONNECTION_FIN);
+
   if (EFI_ERROR (Status)) {
     if (Status == EFI_BUFFER_TOO_SMALL && (ImageType == ImageTypeVirtualCd || ImageType == ImageTypeVirtualDisk)) {
       Status = EFI_WARN_FILE_SYSTEM;
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 5a6ecbc9d9..34a33b09f7 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -959,6 +959,8 @@ HttpBodyParserCallback (
   @retval EFI_OUT_OF_RESOURCES    Failed to complete the operation due to lack of resources.
   @retval EFI_NOT_READY           Can't find a corresponding Tx4Token/Tx6Token or
                                   the EFI_HTTP_UTILITIES_PROTOCOL is not available.
+  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
+                                  a response.

 **/
 EFI_STATUS
@@ -1528,6 +1530,9 @@ Error:
   @retval EFI_OUT_OF_RESOURCES    Could not allocate enough system resources.
   @retval EFI_ACCESS_DENIED       An open TCP connection is not present with the host
                                   specified by response URL.
+
+  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
+                                  a response.
 **/
 EFI_STATUS
 EFIAPI
--
2.17.1





[-- Attachment #2: Type: text/html, Size: 11430 bytes --]

Re: [edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

[Maciej Rabeda]

Hi Andrei,

Thanks for catching this.

Smells a bit like a potential issue with Python's implementation of 
underlying socketserver and interoperability with Windows.
https://stackoverflow.com/questions/46785946/python-socketserver-sends-fin-flag
Nevertheless, HTTP client should be aware that it can receive TCP FIN 
from HTTP server at any time.

As for your patch:
1. Please add maintainer and reviewers as CC: to the patch before 
'Signed-off-by:'. Example: https://edk2.groups.io/g/devel/message/58045
2. Patch & commit title should contain names of package and driver that 
are subject to changes - so it should start with 'NetworkPkg/HttpBootDxe:'
3. HttpBootDxeLoadFile is an implementation of 
EFI_LOAD_FILE_PROTOCOL.LoadFile() protocol function and UEFI 
specification clearly describes possible error codes.
Whatever code is handling boot menu and that protocol, it may not be 
able to react correctly to that error code.
 From the error codes that are available, EFI_ABORTED best fits the 
'Load process was cancelled by the server' narrative.

HttpBootDxeLoadFile, line 602 contains an if (EFI_ERROR (Status)) clause.
Please, catch EFI_CONNECTION_FIN error inside that clause.
Use AsciiPrint to report the following message to the user: "\n Error: 
Server has terminated the connection."
Set Status to EFI_ABORTED before the whole function returns it.

Thanks,
Maciej

On 14-May-20 09:02, Andrei Warkentin wrote:
> Python http.server seems to FIN after the first HEAD request to size
> the loaded file is completed and ACKed. What happens next is interesting.
> On low latency connections, the GET request to download may get sent
> after the server sends the FIN but before the client has a chance to
> process it. The net result is:
> - Server ignores GET
> - HttpBootLoadFile returns EFI_CONNECTION_FIN. Boot fails.
>
> In the other case, client handles the FIN before attempting the GET,
> so there's a proper three-way handshake as part of GET.
>
> The solution is to retry HttpBootLoadFile 2 times if it returns
> EFI_CONNECTION_FIN. This is because HttpBootLoadFile may issue up to
> 3 requests: HEAD/GET to get size and final GET to load. Some servers
> may send a FIN after each request. The first request (HEAD) is not
> supposed to fail this way, so only the two subsequent GET request
> may result in a FIN.
>
> Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=2720
>
> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> ---
>   NetworkPkg/HttpBootDxe/HttpBootImpl.c | 27 +++++++++++++++++++-
>   NetworkPkg/HttpDxe/HttpImpl.c         |  5 ++++
>   2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> index 4a51f35cdd..2d74d5f293 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> @@ -288,6 +288,7 @@ HttpBootDhcp (
>     @retval EFI_NOT_STARTED             The driver is in stopped state.
>     @retval EFI_BUFFER_TOO_SMALL        The BufferSize is too small to read the boot file. BufferSize has
>                                         been updated with the size needed to complete the request.
> +  @retval EFI_CONNECTION_FIN          Server had closed the connection while we were waiting downloading.
>     @retval EFI_DEVICE_ERROR            An unexpected network error occurred.
>     @retval Others                      Other errors as indicated.
>   
> @@ -535,6 +536,7 @@ HttpBootStop (
>     @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current directory entry.
>                                   BufferSize has been updated with the size needed to complete
>                                   the request.
> +  @retval EFI_CONNECTION_FIN    Server had closed the connection while we were waiting for a response.
>   
>   **/
>   EFI_STATUS
> @@ -553,6 +555,7 @@ HttpBootDxeLoadFile (
>     BOOLEAN                       UsingIpv6;
>     EFI_STATUS                    Status;
>     HTTP_BOOT_IMAGE_TYPE          ImageType;
> +  UINTN                         MaxTries;
>   
>     if (This == NULL || BufferSize == NULL || FilePath == NULL) {
>       return EFI_INVALID_PARAMETER;
> @@ -598,7 +601,29 @@ HttpBootDxeLoadFile (
>     // Load the boot file.
>     //
>     ImageType = ImageTypeMax;
> -  Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
> +  //
> +  // HttpBootLoadFile may issue up to 2 requests: HEAD/GET to get
> +  // size and final GET to load. Some servers may send a FIN after
> +  // each request. The first request (HEAD) is not supposed to
> +  // fail this way, so only the two possible GETs need the special
> +  // handling.
> +  //
> +  MaxTries = 2;
> +  do {
> +    Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
> +    if (Status == EFI_CONNECTION_FIN) {
> +      if (Private->HttpCreated) {
> +        //
> +        // Tear down HTTP/TCP state entirely. Http->Configure (NULL) is not
> +        // sufficient (EFI_ACCESS_DENIED from TCP stack on subsequent
> +        // HttpBootLoadFile.
> +        //
> +        HttpIoDestroyIo (&Private->HttpIo);
> +        Private->HttpCreated = FALSE;
> +      }
> +    }
> +  } while (MaxTries-- && Status == EFI_CONNECTION_FIN);
> +
>     if (EFI_ERROR (Status)) {
>       if (Status == EFI_BUFFER_TOO_SMALL && (ImageType == ImageTypeVirtualCd || ImageType == ImageTypeVirtualDisk)) {
>         Status = EFI_WARN_FILE_SYSTEM;
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index 5a6ecbc9d9..34a33b09f7 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -959,6 +959,8 @@ HttpBodyParserCallback (
>     @retval EFI_OUT_OF_RESOURCES    Failed to complete the operation due to lack of resources.
>     @retval EFI_NOT_READY           Can't find a corresponding Tx4Token/Tx6Token or
>                                     the EFI_HTTP_UTILITIES_PROTOCOL is not available.
> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
> +                                  a response.
>   
>   **/
>   EFI_STATUS
> @@ -1528,6 +1530,9 @@ Error:
>     @retval EFI_OUT_OF_RESOURCES    Could not allocate enough system resources.
>     @retval EFI_ACCESS_DENIED       An open TCP connection is not present with the host
>                                     specified by response URL.
> +
> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
> +                                  a response.
>   **/
>   EFI_STATUS
>   EFIAPI

Re: [edk2-devel] [edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

[Andrei Warkentin]

[-- Attachment #1: Type: text/plain, Size: 8144 bytes --]

Thank you. I made the updates and will now send v2.

A
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Maciej Rabeda via groups.io <maciej.rabeda=linux.intel.com@groups.io>
Sent: Tuesday, May 19, 2020 7:02 AM
To: Andrei Warkentin <andrey.warkentin@gmail.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: jiaxin.wu@intel.com <jiaxin.wu@intel.com>; siyuan.fu@intel.com <siyuan.fu@intel.com>
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

Hi Andrei,

Thanks for catching this.

Smells a bit like a potential issue with Python's implementation of
underlying socketserver and interoperability with Windows.
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstackoverflow.com%2Fquestions%2F46785946%2Fpython-socketserver-sends-fin-flag&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Cf607c49796204ebaffe108d7fbec88f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637254865643641251&amp;sdata=WC0rfvSVDu%2Fbq2IhqgBRJCSOMeq%2BQNYu9VZBhG1H4cY%3D&amp;reserved=0
Nevertheless, HTTP client should be aware that it can receive TCP FIN
from HTTP server at any time.

As for your patch:
1. Please add maintainer and reviewers as CC: to the patch before
'Signed-off-by:'. Example: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F58045&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Cf607c49796204ebaffe108d7fbec88f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637254865643641251&amp;sdata=fj9mXIvN7Vvq7M1gOd0DmFKTwnfGIoh2oY%2FthJkAGf0%3D&amp;reserved=0
2. Patch & commit title should contain names of package and driver that
are subject to changes - so it should start with 'NetworkPkg/HttpBootDxe:'
3. HttpBootDxeLoadFile is an implementation of
EFI_LOAD_FILE_PROTOCOL.LoadFile() protocol function and UEFI
specification clearly describes possible error codes.
Whatever code is handling boot menu and that protocol, it may not be
able to react correctly to that error code.
 From the error codes that are available, EFI_ABORTED best fits the
'Load process was cancelled by the server' narrative.

HttpBootDxeLoadFile, line 602 contains an if (EFI_ERROR (Status)) clause.
Please, catch EFI_CONNECTION_FIN error inside that clause.
Use AsciiPrint to report the following message to the user: "\n Error:
Server has terminated the connection."
Set Status to EFI_ABORTED before the whole function returns it.

Thanks,
Maciej

On 14-May-20 09:02, Andrei Warkentin wrote:
> Python http.server seems to FIN after the first HEAD request to size
> the loaded file is completed and ACKed. What happens next is interesting.
> On low latency connections, the GET request to download may get sent
> after the server sends the FIN but before the client has a chance to
> process it. The net result is:
> - Server ignores GET
> - HttpBootLoadFile returns EFI_CONNECTION_FIN. Boot fails.
>
> In the other case, client handles the FIN before attempting the GET,
> so there's a proper three-way handshake as part of GET.
>
> The solution is to retry HttpBootLoadFile 2 times if it returns
> EFI_CONNECTION_FIN. This is because HttpBootLoadFile may issue up to
> 3 requests: HEAD/GET to get size and final GET to load. Some servers
> may send a FIN after each request. The first request (HEAD) is not
> supposed to fail this way, so only the two subsequent GET request
> may result in a FIN.
>
> Fixes https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2720&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Cf607c49796204ebaffe108d7fbec88f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637254865643641251&amp;sdata=kKFG8ZhccLHTdW%2BBs7iQcqNK2yG%2FQaLgcVS%2BtQ2O59o%3D&amp;reserved=0
>
> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> ---
>   NetworkPkg/HttpBootDxe/HttpBootImpl.c | 27 +++++++++++++++++++-
>   NetworkPkg/HttpDxe/HttpImpl.c         |  5 ++++
>   2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> index 4a51f35cdd..2d74d5f293 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> @@ -288,6 +288,7 @@ HttpBootDhcp (
>     @retval EFI_NOT_STARTED             The driver is in stopped state.
>     @retval EFI_BUFFER_TOO_SMALL        The BufferSize is too small to read the boot file. BufferSize has
>                                         been updated with the size needed to complete the request.
> +  @retval EFI_CONNECTION_FIN          Server had closed the connection while we were waiting downloading.
>     @retval EFI_DEVICE_ERROR            An unexpected network error occurred.
>     @retval Others                      Other errors as indicated.
>
> @@ -535,6 +536,7 @@ HttpBootStop (
>     @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current directory entry.
>                                   BufferSize has been updated with the size needed to complete
>                                   the request.
> +  @retval EFI_CONNECTION_FIN    Server had closed the connection while we were waiting for a response.
>
>   **/
>   EFI_STATUS
> @@ -553,6 +555,7 @@ HttpBootDxeLoadFile (
>     BOOLEAN                       UsingIpv6;
>     EFI_STATUS                    Status;
>     HTTP_BOOT_IMAGE_TYPE          ImageType;
> +  UINTN                         MaxTries;
>
>     if (This == NULL || BufferSize == NULL || FilePath == NULL) {
>       return EFI_INVALID_PARAMETER;
> @@ -598,7 +601,29 @@ HttpBootDxeLoadFile (
>     // Load the boot file.
>     //
>     ImageType = ImageTypeMax;
> -  Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
> +  //
> +  // HttpBootLoadFile may issue up to 2 requests: HEAD/GET to get
> +  // size and final GET to load. Some servers may send a FIN after
> +  // each request. The first request (HEAD) is not supposed to
> +  // fail this way, so only the two possible GETs need the special
> +  // handling.
> +  //
> +  MaxTries = 2;
> +  do {
> +    Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
> +    if (Status == EFI_CONNECTION_FIN) {
> +      if (Private->HttpCreated) {
> +        //
> +        // Tear down HTTP/TCP state entirely. Http->Configure (NULL) is not
> +        // sufficient (EFI_ACCESS_DENIED from TCP stack on subsequent
> +        // HttpBootLoadFile.
> +        //
> +        HttpIoDestroyIo (&Private->HttpIo);
> +        Private->HttpCreated = FALSE;
> +      }
> +    }
> +  } while (MaxTries-- && Status == EFI_CONNECTION_FIN);
> +
>     if (EFI_ERROR (Status)) {
>       if (Status == EFI_BUFFER_TOO_SMALL && (ImageType == ImageTypeVirtualCd || ImageType == ImageTypeVirtualDisk)) {
>         Status = EFI_WARN_FILE_SYSTEM;
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index 5a6ecbc9d9..34a33b09f7 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -959,6 +959,8 @@ HttpBodyParserCallback (
>     @retval EFI_OUT_OF_RESOURCES    Failed to complete the operation due to lack of resources.
>     @retval EFI_NOT_READY           Can't find a corresponding Tx4Token/Tx6Token or
>                                     the EFI_HTTP_UTILITIES_PROTOCOL is not available.
> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
> +                                  a response.
>
>   **/
>   EFI_STATUS
> @@ -1528,6 +1530,9 @@ Error:
>     @retval EFI_OUT_OF_RESOURCES    Could not allocate enough system resources.
>     @retval EFI_ACCESS_DENIED       An open TCP connection is not present with the host
>                                     specified by response URL.
> +
> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
> +                                  a response.
>   **/
>   EFI_STATUS
>   EFIAPI





[-- Attachment #2: Type: text/html, Size: 14612 bytes --]

Re: [edk2-devel] [edk2][PATCH 1/1] HttpBoot: handle servers which may FIN after file sizing in HttpBootLoadFile

[Maciej Rabeda]

[-- Attachment #1: Type: text/plain, Size: 6623 bytes --]

Copying my comments from Bugzilla:

I have gone through the Wireshark trace that you have provided. It seems 
to be all clear now and the approach to the issue must change. 
HttpBootDxe supports HTTP 1.1 and assumes that the HTTP session stays 
persistent (single TCP connection for multiple requests instead of one 
for each HTTP request/response pair). 
https://en.wikipedia.org/wiki/HTTP_persistent_connection I am hesitant 
to introduce HTTP/1.0 keep-alive additions to the header. Kindly request 
more debugging on Python server side whether to confirm that 
implementation you are using supports HTTP/1.1. From what I see in here: 
https://github.com/python/cpython/blob/master/Lib/http/server.py 
http.server has support for HTTP/1.1 and assumption on not closing the 
connection. At the time of writing this comment, it is line 311: if 
version_number >= (1, 1) and self.protocol_version >= "HTTP/1.1": 
self.close_connection = False Putting the patch on hold (with trend for 
rejecting with no support for HTTP/1.0 keep-alive). Additional comment: 
https://github.com/python/cpython/blob/master/Lib/http/server.py Line 
616: # The version of the HTTP protocol we support. # Set this to 
HTTP/1.1 to enable automatic keepalive protocol_version = "HTTP/1.0" 
Please modify it that variable to "HTTP/1.1". Your scenario should start 
working. Please come back to me with confirmation.


Thanks,
Maciej

On 14-May-20 09:02, Andrei Warkentin wrote:
> Python http.server seems to FIN after the first HEAD request to size
> the loaded file is completed and ACKed. What happens next is interesting.
> On low latency connections, the GET request to download may get sent
> after the server sends the FIN but before the client has a chance to
> process it. The net result is:
> - Server ignores GET
> - HttpBootLoadFile returns EFI_CONNECTION_FIN. Boot fails.
>
> In the other case, client handles the FIN before attempting the GET,
> so there's a proper three-way handshake as part of GET.
>
> The solution is to retry HttpBootLoadFile 2 times if it returns
> EFI_CONNECTION_FIN. This is because HttpBootLoadFile may issue up to
> 3 requests: HEAD/GET to get size and final GET to load. Some servers
> may send a FIN after each request. The first request (HEAD) is not
> supposed to fail this way, so only the two subsequent GET request
> may result in a FIN.
>
> Fixes https://bugzilla.tianocore.org/show_bug.cgi?id=2720
>
> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> ---
>   NetworkPkg/HttpBootDxe/HttpBootImpl.c | 27 +++++++++++++++++++-
>   NetworkPkg/HttpDxe/HttpImpl.c         |  5 ++++
>   2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> index 4a51f35cdd..2d74d5f293 100644
> --- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> +++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
> @@ -288,6 +288,7 @@ HttpBootDhcp (
>     @retval EFI_NOT_STARTED             The driver is in stopped state.
>     @retval EFI_BUFFER_TOO_SMALL        The BufferSize is too small to read the boot file. BufferSize has
>                                         been updated with the size needed to complete the request.
> +  @retval EFI_CONNECTION_FIN          Server had closed the connection while we were waiting downloading.
>     @retval EFI_DEVICE_ERROR            An unexpected network error occurred.
>     @retval Others                      Other errors as indicated.
>   
> @@ -535,6 +536,7 @@ HttpBootStop (
>     @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current directory entry.
>                                   BufferSize has been updated with the size needed to complete
>                                   the request.
> +  @retval EFI_CONNECTION_FIN    Server had closed the connection while we were waiting for a response.
>   
>   **/
>   EFI_STATUS
> @@ -553,6 +555,7 @@ HttpBootDxeLoadFile (
>     BOOLEAN                       UsingIpv6;
>     EFI_STATUS                    Status;
>     HTTP_BOOT_IMAGE_TYPE          ImageType;
> +  UINTN                         MaxTries;
>   
>     if (This == NULL || BufferSize == NULL || FilePath == NULL) {
>       return EFI_INVALID_PARAMETER;
> @@ -598,7 +601,29 @@ HttpBootDxeLoadFile (
>     // Load the boot file.
>     //
>     ImageType = ImageTypeMax;
> -  Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
> +  //
> +  // HttpBootLoadFile may issue up to 2 requests: HEAD/GET to get
> +  // size and final GET to load. Some servers may send a FIN after
> +  // each request. The first request (HEAD) is not supposed to
> +  // fail this way, so only the two possible GETs need the special
> +  // handling.
> +  //
> +  MaxTries = 2;
> +  do {
> +    Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
> +    if (Status == EFI_CONNECTION_FIN) {
> +      if (Private->HttpCreated) {
> +        //
> +        // Tear down HTTP/TCP state entirely. Http->Configure (NULL) is not
> +        // sufficient (EFI_ACCESS_DENIED from TCP stack on subsequent
> +        // HttpBootLoadFile.
> +        //
> +        HttpIoDestroyIo (&Private->HttpIo);
> +        Private->HttpCreated = FALSE;
> +      }
> +    }
> +  } while (MaxTries-- && Status == EFI_CONNECTION_FIN);
> +
>     if (EFI_ERROR (Status)) {
>       if (Status == EFI_BUFFER_TOO_SMALL && (ImageType == ImageTypeVirtualCd || ImageType == ImageTypeVirtualDisk)) {
>         Status = EFI_WARN_FILE_SYSTEM;
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index 5a6ecbc9d9..34a33b09f7 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -959,6 +959,8 @@ HttpBodyParserCallback (
>     @retval EFI_OUT_OF_RESOURCES    Failed to complete the operation due to lack of resources.
>     @retval EFI_NOT_READY           Can't find a corresponding Tx4Token/Tx6Token or
>                                     the EFI_HTTP_UTILITIES_PROTOCOL is not available.
> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
> +                                  a response.
>   
>   **/
>   EFI_STATUS
> @@ -1528,6 +1530,9 @@ Error:
>     @retval EFI_OUT_OF_RESOURCES    Could not allocate enough system resources.
>     @retval EFI_ACCESS_DENIED       An open TCP connection is not present with the host
>                                     specified by response URL.
> +
> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
> +                                  a response.
>   **/
>   EFI_STATUS
>   EFIAPI


[-- Attachment #2: Type: text/html, Size: 7845 bytes --]