Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Connection reused after premature HTTP server close #5884

Closed
cgpe-a opened this issue Aug 28, 2020 · 1 comment
Closed

Connection reused after premature HTTP server close #5884

cgpe-a opened this issue Aug 28, 2020 · 1 comment
Labels
connecting & proxies

Comments

@cgpe-a
Copy link
Contributor

cgpe-a commented Aug 28, 2020

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
Copy link
Member

bagder commented Aug 28, 2020

@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...

@cgpe-a cgpe-a mentioned this issue Aug 29, 2020
Closed
@bagder bagder closed this as completed in 728f8d3 Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
connecting & proxies
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Check for connection being dead before reuse.
2 participants
@bagder @cgpe-a