From 3533def3d556e09f178e52e37b89fe8015b907f9 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Thu, 11 Aug 2016 14:00:23 +0200 Subject: [PATCH] http2: make sure stream errors don't needlessly close the connection With HTTP/2 each transfer is made in an indivial logical stream over the connection, making most previous errors that caused the connection to get forced-closed now instead just kill the stream and not the connection. Fixes #941 --- docs/KNOWN_BUGS | 12 ++++++++++++ lib/connect.c | 31 +++++++++++++++--------------- lib/connect.h | 40 ++++++++++++++++++++++++++------------ lib/http.c | 39 +++++++++---------------------------- lib/http2.c | 48 +++++++++++++++++++++++++++++++++++++++++++--- lib/http2.h | 4 +++- lib/http_proxy.c | 2 +- lib/multi.c | 47 +++++++++++++++++++++++---------------------- lib/url.c | 11 +++++++++++ lib/urldata.h | 1 + tests/data/test161 | 5 ++--- 11 files changed, 152 insertions(+), 88 deletions(-) diff --git a/docs/KNOWN_BUGS b/docs/KNOWN_BUGS index 05802f17a..dc043803f 100644 --- a/docs/KNOWN_BUGS +++ b/docs/KNOWN_BUGS @@ -63,6 +63,7 @@ problems may have been fixed or changed somewhat since this was written! 7.5 ASCII FTP 7.6 FTP with NULs in URL parts 7.7 FTP and empty path parts in the URL + 7.8 Premature transfer end but healthy control channel 8. TELNET 8.1 TELNET and time limtiations don't work @@ -441,6 +442,17 @@ problems may have been fixed or changed somewhat since this was written! the user wants to reach the root dir (this exception SHALL remain even when this bug is fixed). +7.8 Premature transfer end but healthy control channel + + When 'multi_done' is called before the transfer has been completed the normal + way, it is considered a "premature" transfer end. In this situation, libcurl + closes the connection assuming it doesn't know the state of the connection so + it can't be reused for subsequent requests. + + With FTP however, this isn't necessarily true but there are a bunch of + situations (listed in the ftp_done code) where it *could* keep the connection + alive even in this situation - but the current code doesn't. Fixing this would + allow libcurl to reuse FTP connections better. 8. TELNET diff --git a/lib/connect.c b/lib/connect.c index 7f8c0870a..eca817c35 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -1371,25 +1371,26 @@ CURLcode Curl_socket(struct connectdata *conn, } -#ifdef CURLDEBUG /* - * Curl_conncontrol() is used to set the conn->bits.close bit on or off. It - * MUST be called with the connclose() or connkeep() macros with a stated - * reason. The reason is only shown in debug builds but helps to figure out - * decision paths when connections are or aren't re-used as expected. + * Curl_conncontrol() marks streams or connection for closure. */ -void Curl_conncontrol(struct connectdata *conn, bool closeit, - const char *reason) -{ -#if defined(CURL_DISABLE_VERBOSE_STRINGS) - (void) reason; +void Curl_conncontrol(struct connectdata *conn, + int ctrl /* see defines in header */ +#ifdef CURLDEBUG + , const char *reason #endif - if(closeit != conn->bits.close) { - infof(conn->data, "Marked for [%s]: %s\n", closeit?"closure":"keep alive", - reason); - + ) +{ + /* close if a connection, or a stream that isn't multiplexed */ + bool closeit = (ctrl == CONNCTRL_CONNECTION) || + ((ctrl == CONNCTRL_STREAM) && !(conn->handler->flags & PROTOPT_STREAM)); + if((ctrl == CONNCTRL_STREAM) && + (conn->handler->flags & PROTOPT_STREAM)) + DEBUGF(infof(conn->data, "Kill stream: %s\n", reason)); + else if(closeit != conn->bits.close) { + DEBUGF(infof(conn->data, "Marked for [%s]: %s\n", + closeit?"closure":"keep alive", reason)); conn->bits.close = closeit; /* the only place in the source code that should assign this bit */ } } -#endif diff --git a/lib/connect.h b/lib/connect.h index 6d60e0d81..c3e43cf66 100644 --- a/lib/connect.h +++ b/lib/connect.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2015, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2016, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -104,21 +104,37 @@ CURLcode Curl_socket(struct connectdata *conn, void Curl_tcpnodelay(struct connectdata *conn, curl_socket_t sockfd); -#ifdef CURLDEBUG /* - * Curl_connclose() sets the bit.close bit to TRUE with an explanation. - * Nothing else. + * Curl_conncontrol() marks the end of a connection/stream. The 'closeit' + * argument specifies if it is the end of a connection or a stream. + * + * For stream-based protocols (such as HTTP/2), a stream close will not cause + * a connection close. Other protocols will close the connection for both + * cases. + * + * It sets the bit.close bit to TRUE (with an explanation for debug builds), + * when the connection will close. */ + +#define CONNCTRL_KEEP 0 /* undo a marked closure */ +#define CONNCTRL_CONNECTION 1 +#define CONNCTRL_STREAM 2 + void Curl_conncontrol(struct connectdata *conn, - bool closeit, - const char *reason); -#define connclose(x,y) Curl_conncontrol(x,TRUE, y) -#define connkeep(x,y) Curl_conncontrol(x, FALSE, y) + int closeit +#ifdef CURLDEBUG + , const char *reason +#endif + ); + +#ifdef CURLDEBUG +#define streamclose(x,y) Curl_conncontrol(x, CONNCTRL_STREAM, y) +#define connclose(x,y) Curl_conncontrol(x, CONNCTRL_CONNECTION, y) +#define connkeep(x,y) Curl_conncontrol(x, CONNCTRL_KEEP, y) #else /* if !CURLDEBUG */ - -#define connclose(x,y) (x)->bits.close = TRUE -#define connkeep(x,y) (x)->bits.close = FALSE - +#define streamclose(x,y) Curl_conncontrol(x, CONNCTRL_STREAM) +#define connclose(x,y) Curl_conncontrol(x, CONNCTRL_CONNECTION) +#define connkeep(x,y) Curl_conncontrol(x, CONNCTRL_KEEP) #endif #endif /* HEADER_CURL_CONNECT_H */ diff --git a/lib/http.c b/lib/http.c index 087d1af02..e4b9d8b4b 100644 --- a/lib/http.c +++ b/lib/http.c @@ -462,7 +462,7 @@ static CURLcode http_perhapsrewind(struct connectdata *conn) #endif /* This is not NTLM or many bytes left to send: close */ - connclose(conn, "Mid-auth HTTP and much data left to send"); + streamclose(conn, "Mid-auth HTTP and much data left to send"); data->req.size = 0; /* don't download any more than 0 bytes */ /* There still is data left to send, but this connection is marked for @@ -1452,9 +1452,8 @@ CURLcode Curl_http_done(struct connectdata *conn, { struct Curl_easy *data = conn->data; struct HTTP *http = data->req.protop; -#ifdef USE_NGHTTP2 - struct http_conn *httpc = &conn->proto.httpc; -#endif + + infof(data, "Curl_http_done: called premature == %d\n", premature); Curl_unencode_cleanup(conn); @@ -1467,7 +1466,7 @@ CURLcode Curl_http_done(struct connectdata *conn, * Do not close CONNECT_ONLY connections. */ if((data->req.httpcode != 401) && (data->req.httpcode != 407) && !data->set.connect_only) - connclose(conn, "Negotiate transfer completed"); + streamclose(conn, "Negotiate transfer completed"); Curl_cleanup_negotiate(data); } #endif @@ -1484,27 +1483,7 @@ CURLcode Curl_http_done(struct connectdata *conn, http->send_buffer = NULL; /* clear the pointer */ } -#ifdef USE_NGHTTP2 - if(http->header_recvbuf) { - DEBUGF(infof(data, "free header_recvbuf!!\n")); - Curl_add_buffer_free(http->header_recvbuf); - http->header_recvbuf = NULL; /* clear the pointer */ - Curl_add_buffer_free(http->trailer_recvbuf); - http->trailer_recvbuf = NULL; /* clear the pointer */ - if(http->push_headers) { - /* if they weren't used and then freed before */ - for(; http->push_headers_used > 0; --http->push_headers_used) { - free(http->push_headers[http->push_headers_used - 1]); - } - free(http->push_headers); - http->push_headers = NULL; - } - } - if(http->stream_id) { - nghttp2_session_set_stream_user_data(httpc->h2, http->stream_id, 0); - http->stream_id = 0; - } -#endif + Curl_http2_done(conn, premature); if(HTTPREQ_POST_FORM == data->set.httpreq) { data->req.bytecount = http->readbytecount + http->writebytecount; @@ -3118,7 +3097,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, signal the end of the document. */ infof(data, "no chunk, no close, no size. Assume close to " "signal end\n"); - connclose(conn, "HTTP: No end-of-message indicator"); + streamclose(conn, "HTTP: No end-of-message indicator"); } } @@ -3199,7 +3178,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, */ if(!k->upload_done) { infof(data, "HTTP error before end of send, stop sending\n"); - connclose(conn, "Stop sending data before everything sent"); + streamclose(conn, "Stop sending data before everything sent"); k->upload_done = TRUE; k->keepon &= ~KEEP_SEND; /* don't send */ if(data->state.expect100header) @@ -3503,7 +3482,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, /* Negative Content-Length is really odd, and we know it happens for example when older Apache servers send large files */ - connclose(conn, "negative content-length"); + streamclose(conn, "negative content-length"); infof(data, "Negative content-length: %" CURL_FORMAT_CURL_OFF_T ", closing after transfer\n", contentlength); } @@ -3576,7 +3555,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, * the connection will close when this request has been * served. */ - connclose(conn, "Connection: close used"); + streamclose(conn, "Connection: close used"); } else if(checkprefix("Transfer-Encoding:", k->p)) { /* One or more encodings. We check for chunked and/or a compression diff --git a/lib/http2.c b/lib/http2.c index a14f75e62..e51e72ab4 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -185,7 +185,7 @@ const struct Curl_handler Curl_handler_http2 = { ZERO_NULL, /* readwrite */ PORT_HTTP, /* defport */ CURLPROTO_HTTP, /* protocol */ - PROTOPT_NONE /* flags */ + PROTOPT_STREAM /* flags */ }; const struct Curl_handler Curl_handler_http2_ssl = { @@ -205,7 +205,7 @@ const struct Curl_handler Curl_handler_http2_ssl = { ZERO_NULL, /* readwrite */ PORT_HTTP, /* defport */ CURLPROTO_HTTPS, /* protocol */ - PROTOPT_SSL /* flags */ + PROTOPT_SSL | PROTOPT_STREAM /* flags */ }; /* @@ -489,8 +489,11 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, } stream = data_s->req.protop; - if(!stream) + if(!stream) { + DEBUGF(infof(conn->data, "No proto pointer for stream: %x\n", + stream_id)); return NGHTTP2_ERR_CALLBACK_FAILURE; + } DEBUGF(infof(data_s, "on_frame_recv() header %x stream %x\n", frame->hd.type, stream_id)); @@ -979,6 +982,43 @@ static int error_callback(nghttp2_session *session, } #endif +void Curl_http2_done(struct connectdata *conn, bool premature) +{ + struct Curl_easy *data = conn->data; + struct HTTP *http = data->req.protop; + struct http_conn *httpc = &conn->proto.httpc; + + if(http->header_recvbuf) { + DEBUGF(infof(data, "free header_recvbuf!!\n")); + Curl_add_buffer_free(http->header_recvbuf); + http->header_recvbuf = NULL; /* clear the pointer */ + Curl_add_buffer_free(http->trailer_recvbuf); + http->trailer_recvbuf = NULL; /* clear the pointer */ + if(http->push_headers) { + /* if they weren't used and then freed before */ + for(; http->push_headers_used > 0; --http->push_headers_used) { + free(http->push_headers[http->push_headers_used - 1]); + } + free(http->push_headers); + http->push_headers = NULL; + } + } + + if(premature) { + /* RST_STREAM */ + nghttp2_submit_rst_stream(httpc->h2, NGHTTP2_FLAG_NONE, http->stream_id, + NGHTTP2_STREAM_CLOSED); + if(http->stream_id == httpc->pause_stream_id) { + infof(data, "stopped the pause stream!\n"); + httpc->pause_stream_id = 0; + } + } + if(http->stream_id) { + nghttp2_session_set_stream_user_data(httpc->h2, http->stream_id, 0); + http->stream_id = 0; + } +} + /* * Initialize nghttp2 for a Curl connection */ @@ -1378,6 +1418,8 @@ static ssize_t http2_recv(struct connectdata *conn, int sockindex, socket is not read. But it seems that usually streams are notified with its drain property, and socket is read again quickly. */ + DEBUGF(infof(data, "stream %x is paused, pause id: %x\n", + stream->stream_id, httpc->pause_stream_id)); *err = CURLE_AGAIN; return -1; } diff --git a/lib/http2.h b/lib/http2.h index bedbebf16..cad578ca1 100644 --- a/lib/http2.h +++ b/lib/http2.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2015, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2016, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -51,6 +51,7 @@ CURLcode Curl_http2_switched(struct connectdata *conn, /* called from Curl_http_setup_conn */ void Curl_http2_setup_conn(struct connectdata *conn); void Curl_http2_setup_req(struct Curl_easy *data); +void Curl_http2_done(struct connectdata *conn, bool premature); #else /* USE_NGHTTP2 */ #define Curl_http2_init(x) CURLE_UNSUPPORTED_PROTOCOL #define Curl_http2_send_request(x) CURLE_UNSUPPORTED_PROTOCOL @@ -61,6 +62,7 @@ void Curl_http2_setup_req(struct Curl_easy *data); #define Curl_http2_setup_req(x) #define Curl_http2_init_state(x) #define Curl_http2_init_userset(x) +#define Curl_http2_done(x,y) #endif #endif /* HEADER_CURL_HTTP2_H */ diff --git a/lib/http_proxy.c b/lib/http_proxy.c index 87f86b0c4..082b73ad7 100644 --- a/lib/http_proxy.c +++ b/lib/http_proxy.c @@ -574,7 +574,7 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn, free(data->req.newurl); data->req.newurl = NULL; /* failure, close this connection to avoid re-use */ - connclose(conn, "proxy CONNECT failure"); + streamclose(conn, "proxy CONNECT failure"); Curl_closesocket(conn, conn->sock[sockindex]); conn->sock[sockindex] = CURL_SOCKET_BAD; } diff --git a/lib/multi.c b/lib/multi.c index 7a25103de..e1325f029 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -569,10 +569,9 @@ static CURLcode multi_done(struct connectdata **connp, result = CURLE_ABORTED_BY_CALLBACK; } - if((!premature && - conn->send_pipe->size + conn->recv_pipe->size != 0 && - !data->set.reuse_forbid && - !conn->bits.close)) { + if(conn->send_pipe->size + conn->recv_pipe->size != 0 && + !data->set.reuse_forbid && + !conn->bits.close) { /* Stop if pipeline is not empty and we do not have to close connection. */ DEBUGF(infof(data, "Connection still in use, no more multi_done now!\n")); @@ -685,7 +684,7 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi, /* If the handle is in a pipeline and has started sending off its request but not received its response yet, we need to close connection. */ - connclose(data->easy_conn, "Removed with partial response"); + streamclose(data->easy_conn, "Removed with partial response"); /* Set connection owner so that the DONE function closes it. We can safely do this here since connection is killed. */ data->easy_conn->data = easy; @@ -1298,7 +1297,9 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, return CURLM_BAD_EASY_HANDLE; do { - bool disconnect_conn = FALSE; + /* A "stream" here is a logical stream if the protocol can handle that + (HTTP/2), or the full connection for older protocols */ + bool stream_error = FALSE; rc = CURLM_OK; /* Handle the case when the pipe breaks, i.e., the connection @@ -1376,8 +1377,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* Force connection closed if the connection has indeed been used */ if(data->mstate > CURLM_STATE_DO) { - connclose(data->easy_conn, "Disconnected with pending data"); - disconnect_conn = TRUE; + streamclose(data->easy_conn, "Disconnected with pending data"); + stream_error = TRUE; } result = CURLE_OPERATION_TIMEDOUT; (void)multi_done(&data->easy_conn, result, TRUE); @@ -1426,7 +1427,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* Add this handle to the send or pend pipeline */ result = Curl_add_handle_to_pipeline(data, data->easy_conn); if(result) - disconnect_conn = TRUE; + stream_error = TRUE; else { if(async) /* We're now waiting for an asynchronous name lookup */ @@ -1518,7 +1519,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, if(result) { /* failure detected */ - disconnect_conn = TRUE; + stream_error = TRUE; break; } } @@ -1558,7 +1559,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, else if(result) { /* failure detected */ /* Just break, the cleaning up is handled all in one place */ - disconnect_conn = TRUE; + stream_error = TRUE; break; } break; @@ -1578,7 +1579,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* failure detected */ Curl_posttransfer(data); multi_done(&data->easy_conn, result, TRUE); - disconnect_conn = TRUE; + stream_error = TRUE; } break; @@ -1595,7 +1596,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* failure detected */ Curl_posttransfer(data); multi_done(&data->easy_conn, result, TRUE); - disconnect_conn = TRUE; + stream_error = TRUE; } break; @@ -1670,7 +1671,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, if(drc) { /* a failure here pretty much implies an out of memory */ result = drc; - disconnect_conn = TRUE; + stream_error = TRUE; } else retry = (newurl)?TRUE:FALSE; @@ -1703,7 +1704,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, } else { /* Have error handler disconnect conn if we can't retry */ - disconnect_conn = TRUE; + stream_error = TRUE; free(newurl); } } @@ -1712,7 +1713,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, Curl_posttransfer(data); if(data->easy_conn) multi_done(&data->easy_conn, result, FALSE); - disconnect_conn = TRUE; + stream_error = TRUE; } } break; @@ -1734,7 +1735,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* failure detected */ Curl_posttransfer(data); multi_done(&data->easy_conn, result, FALSE); - disconnect_conn = TRUE; + stream_error = TRUE; } break; @@ -1763,7 +1764,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* failure detected */ Curl_posttransfer(data); multi_done(&data->easy_conn, result, FALSE); - disconnect_conn = TRUE; + stream_error = TRUE; } break; @@ -1885,10 +1886,10 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, if(!(data->easy_conn->handler->flags & PROTOPT_DUAL) && result != CURLE_HTTP2_STREAM) - connclose(data->easy_conn, "Transfer returned error"); + streamclose(data->easy_conn, "Transfer returned error"); Curl_posttransfer(data); - multi_done(&data->easy_conn, result, FALSE); + multi_done(&data->easy_conn, result, TRUE); } else if(done) { followtype follow=FOLLOW_NONE; @@ -1944,7 +1945,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, if(!result) newurl = NULL; /* allocation was handed over Curl_follow() */ else - disconnect_conn = TRUE; + stream_error = TRUE; } multistate(data, CURLM_STATE_DONE); @@ -2045,7 +2046,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, Curl_removeHandleFromPipeline(data, data->easy_conn->send_pipe); Curl_removeHandleFromPipeline(data, data->easy_conn->recv_pipe); - if(disconnect_conn) { + if(stream_error) { /* Don't attempt to send data over a connection that timed out */ bool dead_connection = result == CURLE_OPERATION_TIMEDOUT; /* disconnect properly */ @@ -2069,7 +2070,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* aborted due to progress callback return code must close the connection */ result = CURLE_ABORTED_BY_CALLBACK; - connclose(data->easy_conn, "Aborted by callback"); + streamclose(data->easy_conn, "Aborted by callback"); /* if not yet in DONE state, go there, otherwise COMPLETED */ multistate(data, (data->mstate < CURLM_STATE_DONE)? diff --git a/lib/url.c b/lib/url.c index 29c32cfba..f355c7a22 100644 --- a/lib/url.c +++ b/lib/url.c @@ -2830,6 +2830,17 @@ CURLcode Curl_disconnect(struct connectdata *conn, bool dead_connection) return CURLE_OK; } + /* + * If this connection isn't marked to force-close, leave it open if there + * are other users of it + */ + if(!conn->bits.close && + (conn->send_pipe->size + conn->recv_pipe->size)) { + DEBUGF(infof(data, "Curl_disconnect, usecounter: %d\n", + conn->send_pipe->size + conn->recv_pipe->size)); + return CURLE_OK; + } + if(conn->dns_entry != NULL) { Curl_resolv_unlock(data, conn->dns_entry); conn->dns_entry = NULL; diff --git a/lib/urldata.h b/lib/urldata.h index 44f8dc5c0..1f4d2551e 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -818,6 +818,7 @@ struct Curl_handler { #define PROTOPT_CREDSPERREQUEST (1<<7) /* requires login credentials per request instead of per connection */ #define PROTOPT_ALPN_NPN (1<<8) /* set ALPN and/or NPN for this */ +#define PROTOPT_STREAM (1<<9) /* a protocol with individual logical streams */ /* return the count of bytes sent, or -1 on error */ typedef ssize_t (Curl_send)(struct connectdata *conn, /* connection data */ diff --git a/tests/data/test161 b/tests/data/test161 index c7f4b632c..91a3388fc 100644 --- a/tests/data/test161 +++ b/tests/data/test161 @@ -32,8 +32,8 @@ ftp://%HOSTIP:%FTPPORT/161 # Verify data after the test has been "shot" -# This gets QUIT sent because CURLE_PARTIAL_FILE does NOT mark the control -# connection as bad +# This doesn't send QUIT because of known bug: +# "7.8 Premature transfer end but healthy control channel" USER anonymous PASS ftp@example.com @@ -42,7 +42,6 @@ EPSV TYPE I SIZE 161 RETR 161 -QUIT # CURLE_PARTIAL_FILE = 18