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

diff --git a/lib/http.c b/lib/http.c
index f6633528bc634c5546de70c7c49b75d7405f4594..f7c71afd7d847639b6d5ea7d0cad0861ba688597 100644 (file)
--- 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 0956e510c8bb1e923e90159a044882dadd6e0d99..e48ef96f38950e114d905eefaca98695c431281e 100644 (file)
--- 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 50bf15ab4097b820957b9976297e1e58f83063b6..6c0b06d7de3c29273d85607e4ae7a426302ecf45 100644 (file)
--- 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 cae02cb08c33fde59f9cf01aa75f58bca7d2e487..88180bd0c88e7da9d1fb8d82b7a8cee5cbe4a7ef 100644 (file)
--- 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 73f2da4ba0fba8c55983c5984fb995cc034b7df6..8eecfc4214399b9e601f5a9e6f3a2b0963fccaea 100644 (file)
--- 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) {