From d4618a3fc4d2753493555814c991dd048264e7e7 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Mon, 31 Jul 2023 17:27:03 +0200 Subject: [PATCH] http2: avoid too early connection re-use/multiplexing HTTP/1 connections that are upgraded to HTTP/2 should not be picked up for reuse and multiplexing by other handles until the 101 switching process is completed. Lots-of-debgging-by: Stefan Eissing Reported-by: Richard W.M. Jones Bug: https://curl.se/mail/lib-2023-07/0045.html Closes #11557 --- lib/http.c | 6 ++++++ lib/http2.c | 24 ++++++------------------ lib/multi.c | 12 ++++++++++++ lib/multiif.h | 2 ++ lib/url.c | 18 ++++++++++++++++-- 5 files changed, 42 insertions(+), 20 deletions(-) diff --git a/lib/http.c b/lib/http.c index f6633528bc..f7c71afd7d 100644 --- a/lib/http.c +++ b/lib/http.c @@ -4058,6 +4058,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, /* Switching Protocols */ if(k->upgr101 == UPGR101_H2) { /* Switching to HTTP/2 */ + DEBUGASSERT(conn->httpversion < 20); infof(data, "Received 101, Switching to HTTP/2"); k->upgr101 = UPGR101_RECEIVED; @@ -4100,6 +4101,11 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, } } else { + if(k->upgr101 == UPGR101_H2) { + /* A requested upgrade was denied, poke the multi handle to possibly + allow a pending pipewait to continue */ + Curl_multi_connchanged(data->multi); + } k->header = FALSE; /* no more header to parse! */ if((k->size == -1) && !k->chunk && !conn->bits.close && diff --git a/lib/http2.c b/lib/http2.c index 0956e510c8..e48ef96f38 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -388,18 +388,6 @@ static int on_header(nghttp2_session *session, const nghttp2_frame *frame, static int error_callback(nghttp2_session *session, const char *msg, size_t len, void *userp); -/* - * multi_connchanged() is called to tell that there is a connection in - * this multi handle that has changed state (multiplexing become possible, the - * number of allowed streams changed or similar), and a subsequent use of this - * multi handle should move CONNECT_PEND handles back to CONNECT to have them - * retry. - */ -static void multi_connchanged(struct Curl_multi *multi) -{ - multi->recheckstate = TRUE; -} - /* * Initialize the cfilter context */ @@ -1120,7 +1108,7 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, /* only signal change if the value actually changed */ DEBUGF(LOG_CF(data, cf, "MAX_CONCURRENT_STREAMS now %u", ctx->max_concurrent_streams)); - multi_connchanged(data->multi); + Curl_multi_connchanged(data->multi); } /* Since the initial stream window is 64K, a request might be on HOLD, * due to exhaustion. The (initial) SETTINGS may announce a much larger @@ -1148,7 +1136,7 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, ctx->goaway_error, ctx->last_stream_id)); infof(data, "received GOAWAY, error=%d, last_stream=%u", ctx->goaway_error, ctx->last_stream_id); - multi_connchanged(data->multi); + Curl_multi_connchanged(data->multi); } break; case NGHTTP2_WINDOW_UPDATE: @@ -1838,7 +1826,7 @@ static ssize_t cf_h2_recv(struct Curl_cfilter *cf, struct Curl_easy *data, ssize_t nread = -1; CURLcode result; struct cf_call_data save; - + DEBUGASSERT(stream); CF_DATA_SAVE(save, cf, data); nread = stream_recv(cf, data, buf, len, err); @@ -2601,7 +2589,7 @@ CURLcode Curl_http2_switch(struct Curl_easy *data, conn->httpversion = 20; /* we know we're on HTTP/2 now */ conn->bits.multiplex = TRUE; /* at least potentially multiplexed */ conn->bundle->multiuse = BUNDLE_MULTIPLEX; - multi_connchanged(data->multi); + Curl_multi_connchanged(data->multi); if(cf->next) { bool done; @@ -2629,7 +2617,7 @@ CURLcode Curl_http2_switch_at(struct Curl_cfilter *cf, struct Curl_easy *data) cf->conn->httpversion = 20; /* we know we're on HTTP/2 now */ cf->conn->bits.multiplex = TRUE; /* at least potentially multiplexed */ cf->conn->bundle->multiuse = BUNDLE_MULTIPLEX; - multi_connchanged(data->multi); + Curl_multi_connchanged(data->multi); if(cf_h2->next) { bool done; @@ -2686,7 +2674,7 @@ CURLcode Curl_http2_upgrade(struct Curl_easy *data, conn->httpversion = 20; /* we know we're on HTTP/2 now */ conn->bits.multiplex = TRUE; /* at least potentially multiplexed */ conn->bundle->multiuse = BUNDLE_MULTIPLEX; - multi_connchanged(data->multi); + Curl_multi_connchanged(data->multi); if(cf->next) { bool done; diff --git a/lib/multi.c b/lib/multi.c index 50bf15ab40..6c0b06d7de 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -1577,6 +1577,18 @@ static bool multi_ischanged(struct Curl_multi *multi, bool clear) return retval; } +/* + * Curl_multi_connchanged() is called to tell that there is a connection in + * this multi handle that has changed state (multiplexing become possible, the + * number of allowed streams changed or similar), and a subsequent use of this + * multi handle should move CONNECT_PEND handles back to CONNECT to have them + * retry. + */ +void Curl_multi_connchanged(struct Curl_multi *multi) +{ + multi->recheckstate = TRUE; +} + CURLMcode Curl_multi_add_perform(struct Curl_multi *multi, struct Curl_easy *data, struct connectdata *conn) diff --git a/lib/multiif.h b/lib/multiif.h index cae02cb08c..88180bd0c8 100644 --- a/lib/multiif.h +++ b/lib/multiif.h @@ -41,6 +41,8 @@ void Curl_set_in_callback(struct Curl_easy *data, bool value); bool Curl_is_in_callback(struct Curl_easy *easy); CURLcode Curl_preconnect(struct Curl_easy *data); +void Curl_multi_connchanged(struct Curl_multi *multi); + /* Internal version of curl_multi_init() accepts size parameters for the socket, connection and dns hashes */ struct Curl_multi *Curl_multi_handle(int hashsize, int chashsize, diff --git a/lib/url.c b/lib/url.c index 73f2da4ba0..8eecfc4214 100644 --- a/lib/url.c +++ b/lib/url.c @@ -1073,6 +1073,9 @@ ConnectionExists(struct Curl_easy *data, bool wantProxyNTLMhttp = FALSE; #endif #endif + /* plain HTTP with upgrade */ + bool h2upgrade = (data->state.httpwant == CURL_HTTP_VERSION_2_0) && + (needle->handler->protocol & CURLPROTO_HTTP); *force_reuse = FALSE; *waitpipe = FALSE; @@ -1135,7 +1138,7 @@ ConnectionExists(struct Curl_easy *data, } if(data->set.ipver != CURL_IPRESOLVE_WHATEVER - && data->set.ipver != check->ip_version) { + && data->set.ipver != check->ip_version) { /* skip because the connection is not via the requested IP version */ continue; } @@ -1233,6 +1236,17 @@ ConnectionExists(struct Curl_easy *data, } #endif + if(h2upgrade && !check->httpversion && canmultiplex) { + if(data->set.pipewait) { + infof(data, "Server upgrade doesn't support multiplex yet, wait"); + *waitpipe = TRUE; + CONNCACHE_UNLOCK(data); + return FALSE; /* no re-use */ + } + infof(data, "Server upgrade cannot be used"); + continue; /* can't be used atm */ + } + if(!canmultiplex && CONN_INUSE(check)) /* this request can't be multiplexed but the checked connection is already in use so we skip it */ @@ -1289,7 +1303,7 @@ ConnectionExists(struct Curl_easy *data, (((check->httpversion >= 20) && (data->state.httpwant < CURL_HTTP_VERSION_2_0)) || ((check->httpversion >= 30) && - (data->state.httpwant < CURL_HTTP_VERSION_3)))) + (data->state.httpwant < CURL_HTTP_VERSION_3)))) continue; #ifdef USE_SSH else if(get_protocol_family(needle->handler) & PROTO_FAMILY_SSH) {