Connection reused after premature HTTP server close

[cgpe-a]

On upgrading a project using libcurl from version 7.44.0 to 7.72.0, I encountered a regression in the handling of a premature HTTP server close. The dead connection was correctly discarded in 7.44.0, but incorrectly re-used for a subsequent HTTP request in 7.72.0.

Full details follow with trace output and a tentative fix...

The client starts an HTTP PUT:

Connected to 127.0.0.1 (127.0.0.1) port 33671 (#1)
H>: PUT /test-1-url HTTP/1.1
Host: 127.0.0.1:33671
Accept: */*
Content-Type: application/octet-stream
Content-Length: 1030
Expect: 100-continue

The test server says go ahead:

Mark bundle as not supporting multiuse
H<: HTTP/1.1 100 Continue
We are completely uploaded and fine

The client starts sending data, but in this test as soon as the test server receives the first bytes of data it deliberately does a shutdown() (a half-close) on the socket. The client sees this and completes the request with an error:

Empty reply from server
Connection #1 to host 127.0.0.1 left intact

The client then makes a new request to the server.
In 7.44.0 (and in 7.72.0 after my fix), the connection that was just closed by the server is detected as dead and a new connection is created for the new request:

Found bundle for host 127.0.0.1: 0x212bee0 [serially]
Connection 1 seems to be dead!
Closing connection 1
Hostname 127.0.0.1 was found in DNS cache
  Trying 127.0.0.1:33671...
Connected to 127.0.0.1 (127.0.0.1) port 33671 (#2)
H>: POST /api/external/status HTTP/1.1

However, in un-patched 7.7.20 the connection is reused and the new request fails with EPIPE:

Found bundle for host 127.0.0.1: 0x14a4ef0 [serially]
Re-using existing connection! (#1) with host 127.0.0.1
Connected to 127.0.0.1 (127.0.0.1) port 33671 (#1)
H>: POST /api/external/status HTTP/1.1
...
Send failure: Broken pipe
Closing connection 1

After comparing the code flow between the 2 versions of curl, I deduced that the problem was probably introduced by this commit:

755083d00deb167667882e775b0885da0e63d034

    conn_maxage: move the check to prune_dead_connections()
    
    ... and avoid the locking issue.
    
    Reported-by: Kunal Ekawde
    Fixes #4029
    Closes #4032

That commit changes ConnectionExists() in lib/url.c, moving the conn_maxage() test, but deleting the call to extract_if_dead() which tested if the socket is still readable.

I don't know if this is the correct fix for 7.72.0, but I restored the previous behaviour by restoring the extract_if_dead() call in the original location using this patch:

diff --git a/lib/url.c b/lib/url.c
index 150667aa9..558cd2505 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1121,6 +1121,12 @@ ConnectionExists(struct Curl_easy *data,
         /* connect-only or to-be-closed connections will not be reused */
         continue;
 
+      if(extract_if_dead(check, data)) {
+        /* disconnect it */
+        (void)Curl_disconnect(data, check, /* dead_connection */TRUE);
+        continue;
+      }
+
       if(bundle->multiuse == BUNDLE_MULTIPLEX)
         multiplexed = CONN_INUSE(check);
 

[bagder]

@cgpe-a can you submit your proposed fix as a "real" pull request? Then we get it run through a thorough set of tests automatically...