cfilter: unlink and discard

Rewrite the code that removes a filter from the connection and discards
it. Always look at the connection, otherwise it will not work of the
filter is at the top of the chain.

Change QUIC filter setup code to always tear down the chain in
construction when an error occured.

HTTP proxy, do not remove the h1/h2 sub filter on close. Leave it to be
discarded with the connection. Avoids keeping an additional pointer that
might become dangling.

Triggered by a reported on a code bug in discard method.

Reported-by: Joshua Rogers
Closes #18596

diff --git a/lib/cfilters.c b/lib/cfilters.c
index efd2ac6f63d49797e5f8c7da69c3c3cdef671a46..07f3c5f7b4622a8d3001c5e89ae6bdb8c383aee4 100644 (file)
--- a/lib/cfilters.c
+++ b/lib/cfilters.c
@@ -383,29 +383,26 @@ void Curl_conn_cf_insert_after(struct Curl_cfilter *cf_at,
   *pnext = tail;
 }
 
-bool Curl_conn_cf_discard_sub(struct Curl_cfilter *cf,
-                              struct Curl_cfilter *discard,
-                              struct Curl_easy *data,
-                              bool destroy_always)
+bool Curl_conn_cf_discard(struct Curl_cfilter **pcf,
+                          struct Curl_easy *data)
 {
-  struct Curl_cfilter **pprev = &cf->next;
+  struct Curl_cfilter *cf = pcf ? *pcf : NULL;
   bool found = FALSE;
-
-  /* remove from sub-chain and destroy */
-  DEBUGASSERT(cf);
-  while(*pprev) {
-    if(*pprev == cf) {
-      *pprev = discard->next;
-      discard->next = NULL;
-      found = TRUE;
-      break;
+  if(cf) {
+    if(cf->conn) {
+      /* unlink if present in connection filter chain */
+      struct Curl_cfilter **pprev = &cf->conn->cfilter[cf->sockindex];
+      while(*pprev) {
+        if(*pprev == *pcf) {
+          *pprev = (*pcf)->next;
+          cf->next = NULL;
+          found = TRUE;
+          break;
+        }
+        pprev = &((*pprev)->next);
+      }
     }
-    pprev = &((*pprev)->next);
-  }
-  if(found || destroy_always) {
-    discard->next = NULL;
-    discard->cft->destroy(discard, data);
-    free(discard);
+    Curl_conn_cf_discard_chain(pcf, data);
   }
   return found;
 }

diff --git a/lib/cfilters.h b/lib/cfilters.h
index 815b72a6e801deda86bdd30b5e847c6c36238b1f..af38191a931d52f8e32fb9bb7a501e4b2df37ac5 100644 (file)
--- a/lib/cfilters.h
+++ b/lib/cfilters.h
@@ -297,16 +297,12 @@ void Curl_conn_cf_insert_after(struct Curl_cfilter *cf_at,
                                struct Curl_cfilter *cf_new);
 
 /**
- * Discard, e.g. remove and destroy `discard` iff
- * it still is in the filter chain below `cf`. If `discard`
- * is no longer found beneath `cf` return FALSE.
- * if `destroy_always` is TRUE, will call `discard`s destroy
- * function and free it even if not found in the subchain.
- */
-bool Curl_conn_cf_discard_sub(struct Curl_cfilter *cf,
-                              struct Curl_cfilter *discard,
-                              struct Curl_easy *data,
-                              bool destroy_always);
+ * Extract filter `*pcf` from its connection filter chain.
+ * Destroy `*pcf`, even if it was not part of the chain and NULL it.
+ * Returns TRUE of cf has been part of chain.
+ */
+bool Curl_conn_cf_discard(struct Curl_cfilter **pcf,
+                          struct Curl_easy *data);
 
 /**
  * Discard all cfilters starting with `*pcf` and clearing it afterwards.

diff --git a/lib/http_proxy.c b/lib/http_proxy.c
index 2d742856ce81acc111b9ff8f404d107239d6e07c..b756efe711b40bae0a1c95291757ca27a184927e 100644 (file)
--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -215,9 +215,8 @@ CURLcode Curl_http_proxy_get_destination(struct Curl_cfilter *cf,
 }
 
 struct cf_proxy_ctx {
-  /* the protocol specific sub-filter we install during connect */
-  struct Curl_cfilter *cf_protocol;
   int httpversion; /* HTTP version used to CONNECT */
+  BIT(sub_filter_installed);
 };
 
 CURLcode Curl_http_proxy_create_CONNECT(struct httpreq **preq,
@@ -317,8 +316,7 @@ connect_sub:
     return result;
 
   *done = FALSE;
-  if(!ctx->cf_protocol) {
-    struct Curl_cfilter *cf_protocol = NULL;
+  if(!ctx->sub_filter_installed) {
     int httpversion = 0;
     const char *alpn = Curl_conn_cf_get_alpn_negotiated(cf->next, data);
 
@@ -332,7 +330,6 @@ connect_sub:
       result = Curl_cf_h1_proxy_insert_after(cf, data);
       if(result)
         goto out;
-      cf_protocol = cf->next;
       httpversion = 10;
     }
     else if(!alpn || !strcmp(alpn, "http/1.1")) {
@@ -340,7 +337,6 @@ connect_sub:
       result = Curl_cf_h1_proxy_insert_after(cf, data);
       if(result)
         goto out;
-      cf_protocol = cf->next;
       /* Assume that without an ALPN, we are talking to an ancient one */
       httpversion = 11;
     }
@@ -350,7 +346,6 @@ connect_sub:
       result = Curl_cf_h2_proxy_insert_after(cf, data);
       if(result)
         goto out;
-      cf_protocol = cf->next;
       httpversion = 20;
     }
 #endif
@@ -360,7 +355,7 @@ connect_sub:
       goto out;
     }
 
-    ctx->cf_protocol = cf_protocol;
+    ctx->sub_filter_installed = TRUE;
     ctx->httpversion = httpversion;
     /* after we installed the filter "below" us, we call connect
      * on out sub-chain again.
@@ -371,7 +366,7 @@ connect_sub:
     /* subchain connected and we had already installed the protocol filter.
      * This means the protocol tunnel is established, we are done.
      */
-    DEBUGASSERT(ctx->cf_protocol);
+    DEBUGASSERT(ctx->sub_filter_installed);
     result = CURLE_OK;
   }
 
@@ -419,23 +414,8 @@ static void http_proxy_cf_destroy(struct Curl_cfilter *cf,
 static void http_proxy_cf_close(struct Curl_cfilter *cf,
                                 struct Curl_easy *data)
 {
-  struct cf_proxy_ctx *ctx = cf->ctx;
-
   CURL_TRC_CF(data, cf, "close");
   cf->connected = FALSE;
-  if(ctx->cf_protocol) {
-    struct Curl_cfilter *f;
-    /* if someone already removed it, we assume he also
-     * took care of destroying it. */
-    for(f = cf->next; f; f = f->next) {
-      if(f == ctx->cf_protocol) {
-        /* still in our sub-chain */
-        Curl_conn_cf_discard_sub(cf, ctx->cf_protocol, data, FALSE);
-        break;
-      }
-    }
-    ctx->cf_protocol = NULL;
-  }
   if(cf->next)
     cf->next->cft->do_close(cf->next, data);
 }

diff --git a/lib/vquic/curl_ngtcp2.c b/lib/vquic/curl_ngtcp2.c
index 4191924b5ab907949683127166c7a2db25f03353..6cadc8fc58d1125bf1d5c9d07ce048a3ad74f742 100644 (file)
--- a/lib/vquic/curl_ngtcp2.c
+++ b/lib/vquic/curl_ngtcp2.c
@@ -2767,7 +2767,7 @@ CURLcode Curl_cf_ngtcp2_create(struct Curl_cfilter **pcf,
                                const struct Curl_addrinfo *ai)
 {
   struct cf_ngtcp2_ctx *ctx = NULL;
-  struct Curl_cfilter *cf = NULL, *udp_cf = NULL;
+  struct Curl_cfilter *cf = NULL;
   CURLcode result;
 
   (void)data;
@@ -2781,23 +2781,21 @@ CURLcode Curl_cf_ngtcp2_create(struct Curl_cfilter **pcf,
   result = Curl_cf_create(&cf, &Curl_cft_http3, ctx);
   if(result)
     goto out;
+  cf->conn = conn;
 
-  result = Curl_cf_udp_create(&udp_cf, data, conn, ai, TRNSPRT_QUIC);
+  result = Curl_cf_udp_create(&cf->next, data, conn, ai, TRNSPRT_QUIC);
   if(result)
     goto out;
-
-  cf->conn = conn;
-  udp_cf->conn = cf->conn;
-  udp_cf->sockindex = cf->sockindex;
-  cf->next = udp_cf;
+  cf->next->conn = cf->conn;
+  cf->next->sockindex = cf->sockindex;
 
 out:
   *pcf = (!result) ? cf : NULL;
   if(result) {
-    if(udp_cf)
-      Curl_conn_cf_discard_sub(cf, udp_cf, data, TRUE);
-    Curl_safefree(cf);
-    cf_ngtcp2_ctx_free(ctx);
+    if(cf)
+      Curl_conn_cf_discard_chain(&cf, data);
+    else if(ctx)
+      cf_ngtcp2_ctx_free(ctx);
   }
   return result;
 }

diff --git a/lib/vquic/curl_osslq.c b/lib/vquic/curl_osslq.c
index e82cbf2a3eaa08933a098a0ba37b4a229e486e0a..c292072a65b6b7526a73cae73253224cbcafa467 100644 (file)
--- a/lib/vquic/curl_osslq.c
+++ b/lib/vquic/curl_osslq.c
@@ -2398,7 +2398,7 @@ CURLcode Curl_cf_osslq_create(struct Curl_cfilter **pcf,
                               const struct Curl_addrinfo *ai)
 {
   struct cf_osslq_ctx *ctx = NULL;
-  struct Curl_cfilter *cf = NULL, *udp_cf = NULL;
+  struct Curl_cfilter *cf = NULL;
   CURLcode result;
 
   (void)data;
@@ -2412,23 +2412,22 @@ CURLcode Curl_cf_osslq_create(struct Curl_cfilter **pcf,
   result = Curl_cf_create(&cf, &Curl_cft_http3, ctx);
   if(result)
     goto out;
+  cf->conn = conn;
 
-  result = Curl_cf_udp_create(&udp_cf, data, conn, ai, TRNSPRT_QUIC);
+  result = Curl_cf_udp_create(&cf->next, data, conn, ai, TRNSPRT_QUIC);
   if(result)
     goto out;
 
-  cf->conn = conn;
-  udp_cf->conn = cf->conn;
-  udp_cf->sockindex = cf->sockindex;
-  cf->next = udp_cf;
+  cf->next->conn = cf->conn;
+  cf->next->sockindex = cf->sockindex;
 
 out:
   *pcf = (!result) ? cf : NULL;
   if(result) {
-    if(udp_cf)
-      Curl_conn_cf_discard_sub(cf, udp_cf, data, TRUE);
-    Curl_safefree(cf);
-    cf_osslq_ctx_free(ctx);
+    if(cf)
+      Curl_conn_cf_discard_chain(&cf, data);
+    else if(ctx)
+      cf_osslq_ctx_free(ctx);
   }
   return result;
 }

diff --git a/lib/vquic/curl_quiche.c b/lib/vquic/curl_quiche.c
index 8f0535d65d2acf89488d4c9773af3315245982d5..c1563c4e6309a9efdb4b3563ab7357647645310b 100644 (file)
--- a/lib/vquic/curl_quiche.c
+++ b/lib/vquic/curl_quiche.c
@@ -1636,7 +1636,7 @@ CURLcode Curl_cf_quiche_create(struct Curl_cfilter **pcf,
                                const struct Curl_addrinfo *ai)
 {
   struct cf_quiche_ctx *ctx = NULL;
-  struct Curl_cfilter *cf = NULL, *udp_cf = NULL;
+  struct Curl_cfilter *cf = NULL;
   CURLcode result;
 
   (void)data;
@@ -1651,22 +1651,21 @@ CURLcode Curl_cf_quiche_create(struct Curl_cfilter **pcf,
   result = Curl_cf_create(&cf, &Curl_cft_http3, ctx);
   if(result)
     goto out;
+  cf->conn = conn;
 
-  result = Curl_cf_udp_create(&udp_cf, data, conn, ai, TRNSPRT_QUIC);
+  result = Curl_cf_udp_create(&cf->next, data, conn, ai, TRNSPRT_QUIC);
   if(result)
     goto out;
-
-  udp_cf->conn = cf->conn;
-  udp_cf->sockindex = cf->sockindex;
-  cf->next = udp_cf;
+  cf->next->conn = cf->conn;
+  cf->next->sockindex = cf->sockindex;
 
 out:
   *pcf = (!result) ? cf : NULL;
   if(result) {
-    if(udp_cf)
-      Curl_conn_cf_discard_sub(cf, udp_cf, data, TRUE);
-    Curl_safefree(cf);
-    cf_quiche_ctx_free(ctx);
+    if(cf)
+      Curl_conn_cf_discard_chain(&cf, data);
+    else if(ctx)
+      cf_quiche_ctx_free(ctx);
   }
 
   return result;

diff --git a/lib/vtls/vtls.c b/lib/vtls/vtls.c
index f17f9142bed1964ca7a84820f9fcc629020629ae..bfec585ce21051e60ab250fe54350c0708e196c1 100644 (file)
--- a/lib/vtls/vtls.c
+++ b/lib/vtls/vtls.c
@@ -1862,7 +1862,7 @@ CURLcode Curl_ssl_cfilter_remove(struct Curl_easy *data,
       Curl_shutdown_clear(data, sockindex);
       if(!result && !done) /* blocking failed? */
         result = CURLE_SSL_SHUTDOWN_FAILED;
-      Curl_conn_cf_discard_sub(head, cf, data, FALSE);
+      Curl_conn_cf_discard(&cf, data);
       CURL_TRC_CF(data, cf, "shutdown and remove SSL, done -> %d", result);
       break;
     }