connect: limit update IP info

Update IP related information at the connection and the transfer in two places only: once the filter chain connects and when a transfer is added to a connection. The latter only updates on reuse when the filters already are connected. The only user of that information before a full connect is the HAProxy filter. Add cfilter CF_QUERY_IP_INFO query to let it find the information from the filters "below". This solves two issues with the previous version: - updates where often done twice with the same info - happy eyeballing filter "forks" could overwrite each others updates before the full winner was determined. Closes #14699
curl · Aug 28, 2024 · ea6f5c9 · ea6f5c9
1 parent 87f0a79
commit ea6f5c9
Show file tree

Hide file tree

Showing 15 changed files with 122 additions and 77 deletions.
diff --git a/lib/cf-haproxy.c b/lib/cf-haproxy.c
@@ -70,8 +70,9 @@ static CURLcode cf_haproxy_date_out_set(struct Curl_cfilter*cf,
 {
   struct cf_haproxy_ctx *ctx = cf->ctx;
   CURLcode result;
-  const char *tcp_version;
   const char *client_ip;
+  struct ip_quadruple ipquad;
+  int is_ipv6;
 
   DEBUGASSERT(ctx);
   DEBUGASSERT(ctx->state == HAPROXY_INIT);
@@ -81,19 +82,20 @@ static CURLcode cf_haproxy_date_out_set(struct Curl_cfilter*cf,
     result = Curl_dyn_addn(&ctx->data_out, STRCONST("PROXY UNKNOWN\r\n"));
   else {
 #endif /* USE_UNIX_SOCKETS */
+  result = Curl_conn_cf_get_ip_info(cf->next, data, &is_ipv6, &ipquad);
+  if(result)
+    return result;
+
   /* Emit the correct prefix for IPv6 */
-  tcp_version = cf->conn->bits.ipv6 ? "TCP6" : "TCP4";
   if(data->set.str[STRING_HAPROXY_CLIENT_IP])
     client_ip = data->set.str[STRING_HAPROXY_CLIENT_IP];
   else
-    client_ip = data->info.primary.local_ip;
+    client_ip = ipquad.local_ip;
 
   result = Curl_dyn_addf(&ctx->data_out, "PROXY %s %s %s %i %i\r\n",
-                         tcp_version,
-                         client_ip,
-                         data->info.primary.remote_ip,
-                         data->info.primary.local_port,
-                         data->info.primary.remote_port);
+                         is_ipv6? "TCP6" : "TCP4",
+                         client_ip, ipquad.remote_ip,
+                         ipquad.local_port, ipquad.remote_port);
 
 #ifdef USE_UNIX_SOCKETS
   }

diff --git a/lib/cf-https-connect.c b/lib/cf-https-connect.c
@@ -210,8 +210,6 @@ static CURLcode baller_connected(struct Curl_cfilter *cf,
   }
   ctx->state = CF_HC_SUCCESS;
   cf->connected = TRUE;
-  Curl_conn_cf_cntrl(cf->next, data, TRUE,
-                     CF_CTRL_CONN_INFO_UPDATE, 0, NULL);
   return result;
 }
 

diff --git a/lib/cf-socket.c b/lib/cf-socket.c
@@ -1614,24 +1614,34 @@ static ssize_t cf_socket_recv(struct Curl_cfilter *cf, struct Curl_easy *data,
   return nread;
 }
 
+static void cf_socket_update_data(struct Curl_cfilter *cf,
+                                  struct Curl_easy *data)
+{
+  /* Update the IP info held in the transfer, if we have that. */
+  if(cf->connected && (cf->sockindex == FIRSTSOCKET)) {
+    struct cf_socket_ctx *ctx = cf->ctx;
+    data->info.primary = ctx->ip;
+    /* not sure if this is redundant... */
+    data->info.conn_remote_port = cf->conn->remote_port;
+  }
+}
+
 static void cf_socket_active(struct Curl_cfilter *cf, struct Curl_easy *data)
 {
   struct cf_socket_ctx *ctx = cf->ctx;
 
   /* use this socket from now on */
   cf->conn->sock[cf->sockindex] = ctx->sock;
   set_local_ip(cf, data);
-  if(cf->sockindex == SECONDARYSOCKET)
-    cf->conn->secondary = ctx->ip;
-  else
-    cf->conn->primary = ctx->ip;
-  /* the first socket info gets some specials */
   if(cf->sockindex == FIRSTSOCKET) {
+    cf->conn->primary = ctx->ip;
     cf->conn->remote_addr = &ctx->addr;
   #ifdef USE_IPV6
     cf->conn->bits.ipv6 = (ctx->addr.family == AF_INET6)? TRUE : FALSE;
   #endif
-    Curl_persistconninfo(data, cf->conn, &ctx->ip);
+  }
+  else {
+    cf->conn->secondary = ctx->ip;
   }
   ctx->active = TRUE;
 }
@@ -1647,9 +1657,10 @@ static CURLcode cf_socket_cntrl(struct Curl_cfilter *cf,
   switch(event) {
   case CF_CTRL_CONN_INFO_UPDATE:
     cf_socket_active(cf, data);
+    cf_socket_update_data(cf, data);
     break;
   case CF_CTRL_DATA_SETUP:
-    Curl_persistconninfo(data, cf->conn, &ctx->ip);
+    cf_socket_update_data(cf, data);
     break;
   case CF_CTRL_FORGET_SOCKET:
     ctx->sock = CURL_SOCKET_BAD;
@@ -1732,6 +1743,10 @@ static CURLcode cf_socket_query(struct Curl_cfilter *cf,
     }
     return CURLE_OK;
   }
+  case CF_QUERY_IP_INFO:
+    *pres1 = (ctx->addr.family == AF_INET6)? TRUE : FALSE;
+    *(struct ip_quadruple *)pres2 = ctx->ip;
+    return CURLE_OK;
   default:
     break;
   }

diff --git a/lib/cfilters.c b/lib/cfilters.c
@@ -45,6 +45,9 @@
 #define ARRAYSIZE(A) (sizeof(A)/sizeof((A)[0]))
 #endif
 
+static void cf_cntrl_update_info(struct Curl_easy *data,
+                                 struct connectdata *conn);
+
 #ifdef UNITTESTS
 /* used by unit2600.c */
 void Curl_cf_def_close(struct Curl_cfilter *cf, struct Curl_easy *data)
@@ -428,7 +431,10 @@ CURLcode Curl_conn_connect(struct Curl_easy *data,
 
     result = cf->cft->do_connect(cf, data, blocking, done);
     if(!result && *done) {
-      Curl_conn_ev_update_info(data, data->conn);
+      /* Now that the complete filter chain is connected, let all filters
+       * persist information at the connection. E.g. cf-socket sets the
+       * socket and ip related information. */
+      cf_cntrl_update_info(data, data->conn);
       conn_report_connect_stats(data, data->conn);
       data->conn->keepalive = Curl_now();
     }
@@ -652,6 +658,15 @@ curl_socket_t Curl_conn_cf_get_socket(struct Curl_cfilter *cf,
   return CURL_SOCKET_BAD;
 }
 
+CURLcode Curl_conn_cf_get_ip_info(struct Curl_cfilter *cf,
+                                  struct Curl_easy *data,
+                                  int *is_ipv6, struct ip_quadruple *ipquad)
+{
+  if(cf)
+    return cf->cft->query(cf, data, CF_QUERY_IP_INFO, is_ipv6, ipquad);
+  return CURLE_UNKNOWN_OPTION;
+}
+
 curl_socket_t Curl_conn_get_socket(struct Curl_easy *data, int sockindex)
 {
   struct Curl_cfilter *cf;
@@ -749,8 +764,8 @@ CURLcode Curl_conn_ev_data_pause(struct Curl_easy *data, bool do_pause)
                       CF_CTRL_DATA_PAUSE, do_pause, NULL);
 }
 
-void Curl_conn_ev_update_info(struct Curl_easy *data,
-                              struct connectdata *conn)
+static void cf_cntrl_update_info(struct Curl_easy *data,
+                                 struct connectdata *conn)
 {
   cf_cntrl_all(conn, data, TRUE, CF_CTRL_CONN_INFO_UPDATE, 0, NULL);
 }

diff --git a/lib/cfilters.h b/lib/cfilters.h
@@ -30,6 +30,7 @@ struct Curl_cfilter;
 struct Curl_easy;
 struct Curl_dns_entry;
 struct connectdata;
+struct ip_quadruple;
 
 /* Callback to destroy resources held by this filter instance.
  * Implementations MUST NOT chain calls to cf->next.
@@ -165,6 +166,8 @@ typedef CURLcode Curl_cft_cntrl(struct Curl_cfilter *cf,
  *                   -1 if not determined yet.
  * - CF_QUERY_SOCKET: the socket used by the filter chain
  * - CF_QUERY_NEED_FLUSH: TRUE iff any of the filters have unsent data
+ * - CF_QUERY_IP_INFO: res1 says if connection used IPv6, res2 is the
+ *                   ip quadruple
  */
 /*      query                             res1       res2     */
 #define CF_QUERY_MAX_CONCURRENT     1  /* number     -        */
@@ -174,6 +177,7 @@ typedef CURLcode Curl_cft_cntrl(struct Curl_cfilter *cf,
 #define CF_QUERY_TIMER_APPCONNECT   5  /* -          struct curltime */
 #define CF_QUERY_STREAM_ERROR       6  /* error code - */
 #define CF_QUERY_NEED_FLUSH         7  /* TRUE/FALSE - */
+#define CF_QUERY_IP_INFO            8  /* TRUE/FALSE struct ip_quadruple */
 
 /**
  * Query the cfilter for properties. Filters ignorant of a query will
@@ -344,6 +348,10 @@ bool Curl_conn_cf_is_ssl(struct Curl_cfilter *cf);
 curl_socket_t Curl_conn_cf_get_socket(struct Curl_cfilter *cf,
                                       struct Curl_easy *data);
 
+CURLcode Curl_conn_cf_get_ip_info(struct Curl_cfilter *cf,
+                                  struct Curl_easy *data,
+                                  int *is_ipv6, struct ip_quadruple *ipquad);
+
 bool Curl_conn_cf_needs_flush(struct Curl_cfilter *cf,
                               struct Curl_easy *data);
 
@@ -515,12 +523,6 @@ void Curl_conn_ev_data_done(struct Curl_easy *data, bool premature);
  */
 CURLcode Curl_conn_ev_data_pause(struct Curl_easy *data, bool do_pause);
 
-/**
- * Inform connection filters to update their info in `conn`.
- */
-void Curl_conn_ev_update_info(struct Curl_easy *data,
-                              struct connectdata *conn);
-
 /**
  * Check if FIRSTSOCKET's cfilter chain deems connection alive.
  */

diff --git a/lib/connect.c b/lib/connect.c
@@ -208,31 +208,6 @@ bool Curl_shutdown_started(struct Curl_easy *data, int sockindex)
   return (pt->tv_sec > 0) || (pt->tv_usec > 0);
 }
 
-/* Copies connection info into the transfer handle to make it available when
-   the transfer handle is no longer associated with the connection. */
-void Curl_persistconninfo(struct Curl_easy *data, struct connectdata *conn,
-                          struct ip_quadruple *ip)
-{
-  if(ip)
-    data->info.primary = *ip;
-  else {
-    memset(&data->info.primary, 0, sizeof(data->info.primary));
-    data->info.primary.remote_port = -1;
-    data->info.primary.local_port = -1;
-  }
-  data->info.conn_scheme = conn->handler->scheme;
-  /* conn_protocol can only provide "old" protocols */
-  data->info.conn_protocol = (conn->handler->protocol) & CURLPROTO_MASK;
-  data->info.conn_remote_port = conn->remote_port;
-  data->info.used_proxy =
-#ifdef CURL_DISABLE_PROXY
-    0
-#else
-    conn->bits.proxy
-#endif
-    ;
-}
-
 static const struct Curl_addrinfo *
 addr_first_match(const struct Curl_addrinfo *addr, int family)
 {
@@ -992,8 +967,6 @@ static CURLcode cf_he_connect(struct Curl_cfilter *cf,
         cf->next = ctx->winner->cf;
         ctx->winner->cf = NULL;
         cf_he_ctx_clear(cf, data);
-        Curl_conn_cf_cntrl(cf->next, data, TRUE,
-                           CF_CTRL_CONN_INFO_UPDATE, 0, NULL);
 
         if(cf->conn->handler->protocol & PROTO_FAMILY_SSH)
           Curl_pgrsTime(data, TIMER_APPCONNECT); /* we are connected already */

diff --git a/lib/connect.h b/lib/connect.h
@@ -72,9 +72,6 @@ curl_socket_t Curl_getconnectinfo(struct Curl_easy *data,
 bool Curl_addr2string(struct sockaddr *sa, curl_socklen_t salen,
                       char *addr, int *port);
 
-void Curl_persistconninfo(struct Curl_easy *data, struct connectdata *conn,
-                          struct ip_quadruple *ip);
-
 /*
  * Curl_conncontrol() marks the end of a connection/stream. The 'closeit'
  * argument specifies if it is the end of a connection or a stream.

diff --git a/lib/ftp.c b/lib/ftp.c
@@ -2082,7 +2082,6 @@ static CURLcode ftp_state_pasv_resp(struct Curl_easy *data,
 
     /* postponed address resolution in case of tcp fastopen */
     if(conn->bits.tcp_fastopen && !conn->bits.reuse && !ftpc->newhost[0]) {
-      Curl_conn_ev_update_info(data, conn);
       Curl_safefree(ftpc->newhost);
       ftpc->newhost = strdup(control_address(conn));
       if(!ftpc->newhost)

diff --git a/lib/getinfo.c b/lib/getinfo.c
@@ -77,10 +77,9 @@ CURLcode Curl_initinfo(struct Curl_easy *data)
   free(info->wouldredirect);
   info->wouldredirect = NULL;
 
-  info->primary.remote_ip[0] = '\0';
-  info->primary.local_ip[0] = '\0';
-  info->primary.remote_port = 0;
-  info->primary.local_port = 0;
+  memset(&info->primary, 0, sizeof(info->primary));
+  info->primary.remote_port = -1;
+  info->primary.local_port = -1;
   info->retry_after = 0;
 
   info->conn_scheme = 0;

diff --git a/lib/url.c b/lib/url.c
@@ -3434,7 +3434,9 @@ static CURLcode create_conn(struct Curl_easy *data,
     /* this is supposed to be the connect function so we better at least check
        that the file is present here! */
     DEBUGASSERT(conn->handler->connect_it);
-    Curl_persistconninfo(data, conn, NULL);
+    data->info.conn_scheme = conn->handler->scheme;
+    /* conn_protocol can only provide "old" protocols */
+    data->info.conn_protocol = (conn->handler->protocol) & CURLPROTO_MASK;
     result = conn->handler->connect_it(data, &done);
 
     /* Setup a "faked" transfer that will do nothing */
@@ -3630,9 +3632,20 @@ static CURLcode create_conn(struct Curl_easy *data,
       goto out;
   }
 
+  /* persist the scheme and handler the transfer is using */
+  data->info.conn_scheme = conn->handler->scheme;
+  /* conn_protocol can only provide "old" protocols */
+  data->info.conn_protocol = (conn->handler->protocol) & CURLPROTO_MASK;
+  data->info.used_proxy =
+#ifdef CURL_DISABLE_PROXY
+    0
+#else
+    conn->bits.proxy
+#endif
+    ;
+
   /* Everything general done, inform filters that they need
-   * to prepare for a data transfer.
-   */
+   * to prepare for a data transfer. */
   result = Curl_conn_ev_data_setup(data);
 
 out:

diff --git a/tests/http/test_01_basic.py b/tests/http/test_01_basic.py
@@ -94,7 +94,9 @@ def test_01_06_timings(self, env: Env, httpd, nghttpx, repeat, proto):
         curl = CurlClient(env=env)
         url = f'https://{env.authority_for(env.domain1, proto)}/data.json'
         r = curl.http_download(urls=[url], alpn_proto=proto, with_stats=True)
-        r.check_stats(http_status=200, count=1)
+        r.check_stats(http_status=200, count=1,
+                      remote_port=env.port_for(alpn_proto=proto),
+                      remote_ip='127.0.0.1')
         assert r.stats[0]['time_connect'] > 0, f'{r.stats[0]}'
         assert r.stats[0]['time_appconnect'] > 0, f'{r.stats[0]}'
 
@@ -108,7 +110,9 @@ def test_01_07_head(self, env: Env, httpd, nghttpx, repeat, proto):
         url = f'https://{env.authority_for(env.domain1, proto)}/data.json'
         r = curl.http_download(urls=[url], with_stats=True, with_headers=True,
                                extra_args=['-I'])
-        r.check_stats(http_status=200, count=1, exitcode=0)
+        r.check_stats(http_status=200, count=1, exitcode=0,
+                      remote_port=env.port_for(alpn_proto=proto),
+                      remote_ip='127.0.0.1')
         # got the Conten-Length: header, but did not download anything
         assert r.responses[0]['header']['content-length'] == '30', f'{r.responses[0]}'
         assert r.stats[0]['size_download'] == 0, f'{r.stats[0]}'
diff --git a/tests/http/test_02_download.py b/tests/http/test_02_download.py
@@ -272,7 +272,9 @@ def test_02_14_not_found(self, env: Env, httpd, nghttpx, repeat, proto):
         r = curl.http_download(urls=[urln], alpn_proto=proto, extra_args=[
             '--parallel'
         ])
-        r.check_stats(count=count, http_status=404, exitcode=0)
+        r.check_stats(count=count, http_status=404, exitcode=0,
+                      remote_port=env.port_for(alpn_proto=proto),
+                      remote_ip='127.0.0.1')
 
     @pytest.mark.parametrize("proto", ['h2', 'h3'])
     def test_02_15_fail_not_found(self, env: Env, httpd, nghttpx, repeat, proto):
@@ -286,7 +288,9 @@ def test_02_15_fail_not_found(self, env: Env, httpd, nghttpx, repeat, proto):
         r = curl.http_download(urls=[urln], alpn_proto=proto, extra_args=[
             '--fail'
         ])
-        r.check_stats(count=count, http_status=404, exitcode=22)
+        r.check_stats(count=count, http_status=404, exitcode=22,
+                      remote_port=env.port_for(alpn_proto=proto),
+                      remote_ip='127.0.0.1')
 
     @pytest.mark.skipif(condition=Env().slow_network, reason="not suitable for slow network tests")
     @pytest.mark.skipif(condition=Env().ci_run, reason="not suitable for CI runs")