From: Stefan Eissing Date: Tue, 27 Aug 2024 10:12:37 +0000 (+0200) Subject: connect: limit update IP info X-Git-Tag: curl-8_10_0~100 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ea6f5c9f0fff4f069b3060f0c21f8c8ebc69b386;p=thirdparty%2Fcurl.git 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 --- diff --git a/lib/cf-haproxy.c b/lib/cf-haproxy.c index aa4fca0cf1..4b59909024 100644 --- 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 index 0bdec5bf72..facb3da270 100644 --- 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 index c1f1ac866e..3fdd84303d 100644 --- a/lib/cf-socket.c +++ b/lib/cf-socket.c @@ -1614,6 +1614,18 @@ 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; @@ -1621,17 +1633,15 @@ static void cf_socket_active(struct Curl_cfilter *cf, struct Curl_easy *data) /* 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 index af49028487..bed59f9fbb 100644 --- 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 index a516e578ce..af696f52a5 100644 --- 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 index 00be19a078..3905996521 100644 --- 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 index e9a4f9c598..160db9420f 100644 --- 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 index cbb89012cc..5406175bba 100644 --- 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 index ef16a12eb6..714610156f 100644 --- 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 index ccd2cdd39e..1d9d652bd6 100644 --- 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 index ef62ba40f0..012e9ecabc 100644 --- a/tests/http/test_01_basic.py +++ b/tests/http/test_01_basic.py @@ -94,7 +94,9 @@ class TestBasic: 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 @@ class TestBasic: 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 index 4fed6aeb9a..9d5f8c8c54 100644 --- a/tests/http/test_02_download.py +++ b/tests/http/test_02_download.py @@ -272,7 +272,9 @@ class TestDownload: 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 @@ class TestDownload: 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") diff --git a/tests/http/test_16_info.py b/tests/http/test_16_info.py index 41673ce303..570619af35 100644 --- a/tests/http/test_16_info.py +++ b/tests/http/test_16_info.py @@ -62,7 +62,9 @@ class TestInfo: curl = CurlClient(env=env) url = f'https://{env.authority_for(env.domain1, proto)}/data.json?[0-{count-1}]' r = curl.http_download(urls=[url], alpn_proto=proto, with_stats=True) - r.check_stats(count=count, http_status=200, exitcode=0) + r.check_stats(count=count, http_status=200, exitcode=0, + remote_port=env.port_for(alpn_proto=proto), + remote_ip='127.0.0.1') for idx, s in enumerate(r.stats): self.check_stat(idx, s, r, dl_size=30, ul_size=0) @@ -77,7 +79,9 @@ class TestInfo: r = curl.http_download(urls=[url], alpn_proto=proto, with_stats=True, extra_args=[ '--location' ]) - r.check_stats(count=count, http_status=200, exitcode=0) + r.check_stats(count=count, http_status=200, exitcode=0, + remote_port=env.port_for(alpn_proto=proto), + remote_ip='127.0.0.1') for idx, s in enumerate(r.stats): self.check_stat(idx, s, r, dl_size=30, ul_size=0) @@ -95,7 +99,9 @@ class TestInfo: '--trace-config', 'http/2,http/3' ]) r.check_response(count=count, http_status=200) - r.check_stats(count=count, http_status=200, exitcode=0) + r.check_stats(count=count, http_status=200, exitcode=0, + remote_port=env.port_for(alpn_proto=proto), + remote_ip='127.0.0.1') for idx, s in enumerate(r.stats): self.check_stat(idx, s, r, dl_size=fsize, ul_size=fsize) @@ -106,7 +112,8 @@ class TestInfo: curl = CurlClient(env=env) url = f'http://{env.domain1}:{env.http_port}/data.json?[0-{count-1}]' r = curl.http_download(urls=[url], alpn_proto=proto, with_stats=True) - r.check_stats(count=count, http_status=200, exitcode=0) + r.check_stats(count=count, http_status=200, exitcode=0, + remote_port=env.http_port, remote_ip='127.0.0.1') for idx, s in enumerate(r.stats): self.check_stat(idx, s, r, dl_size=30, ul_size=0) diff --git a/tests/http/testenv/curl.py b/tests/http/testenv/curl.py index 3201850387..65caeb784c 100644 --- a/tests/http/testenv/curl.py +++ b/tests/http/testenv/curl.py @@ -390,7 +390,9 @@ class ExecResult: f'were made\n{self.dump_logs()}' def check_stats(self, count: int, http_status: Optional[int] = None, - exitcode: Optional[int] = None): + exitcode: Optional[int] = None, + remote_port: Optional[int] = None, + remote_ip: Optional[str] = None): if exitcode is None: self.check_exit_code(0) assert len(self.stats) == count, \ @@ -408,6 +410,18 @@ class ExecResult: assert x['exitcode'] == exitcode, \ f'status #{idx} exitcode: expected {exitcode}, '\ f'got {x["exitcode"]}\n{self.dump_stat(x)}' + if remote_port is not None: + for idx, x in enumerate(self.stats): + assert 'remote_port' in x, f'remote_port missing\n{self.dump_stat(x)}' + assert x['remote_port'] == remote_port, \ + f'status #{idx} remote_port: expected {remote_port}, '\ + f'got {x["remote_port"]}\n{self.dump_stat(x)}' + if remote_ip is not None: + for idx, x in enumerate(self.stats): + assert 'remote_ip' in x, f'remote_ip missing\n{self.dump_stat(x)}' + assert x['remote_ip'] == remote_ip, \ + f'status #{idx} remote_ip: expected {remote_ip}, '\ + f'got {x["remote_ip"]}\n{self.dump_stat(x)}' def dump_logs(self): lines = ['>>--stdout ----------------------------------------------\n'] diff --git a/tests/http/testenv/env.py b/tests/http/testenv/env.py index 2cd6432364..77b4b6e639 100644 --- a/tests/http/testenv/env.py +++ b/tests/http/testenv/env.py @@ -552,13 +552,16 @@ class Env: def ci_run(self) -> bool: return "CURL_CI" in os.environ - def authority_for(self, domain: str, alpn_proto: Optional[str] = None): + def port_for(self, alpn_proto: Optional[str] = None): if alpn_proto is None or \ alpn_proto in ['h2', 'http/1.1', 'http/1.0', 'http/0.9']: - return f'{domain}:{self.https_port}' + return self.https_port if alpn_proto in ['h3']: - return f'{domain}:{self.h3_port}' - return f'{domain}:{self.http_port}' + return self.h3_port + return self.http_port + + def authority_for(self, domain: str, alpn_proto: Optional[str] = None): + return f'{domain}:{self.port_for(alpn_proto=alpn_proto)}' def make_data_file(self, indir: str, fname: str, fsize: int) -> str: fpath = os.path.join(indir, fname)