From: Aram Sargsyan <aram@isc.org>
Date: Fri, 20 Dec 2024 08:41:03 +0000 (+0000)
Subject: Fix rtt calculation bug for TCP in the resolver
X-Git-Tag: v9.21.5~39^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=87c453850c716fb8ac283b10668aa95a9cef421e;p=thirdparty%2Fbind9.git

Fix rtt calculation bug for TCP in the resolver

When TCP is used, 'fctx_query()' adds one second to the rtt
(round-trip time) value, but there's a bug when the decision
about using TCP is made already after the calculation. Move the
block of the code which looks up the peers list to decide
whether to use TCP into a place that's before the rtt calculation
is performed. This commit doesn't add or remove any code, it just
moves the code and adds a comment block.
---

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 746fe6d9473..2aa4ca381a2 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -1911,55 +1911,6 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 		}
 	}
 
-	/*
-	 * Allow an additional second for the kernel to resend the SYN
-	 * (or SYN without ECN in the case of stupid firewalls blocking
-	 * ECN negotiation) over the current RTT estimate.
-	 */
-	if ((options & DNS_FETCHOPT_TCP) != 0) {
-		srtt += US_PER_SEC;
-	}
-
-	/*
-	 * A forwarder needs to make multiple queries. Give it at least
-	 * a second to do these in.
-	 */
-	if (ISFORWARDER(addrinfo) && srtt < US_PER_SEC) {
-		srtt = US_PER_SEC;
-	}
-
-	fctx_setretryinterval(fctx, srtt);
-	if (isc_interval_iszero(&fctx->interval)) {
-		FCTXTRACE("fetch expired");
-		return ISC_R_TIMEDOUT;
-	}
-
-	INSIST(ISC_LIST_EMPTY(fctx->validators));
-
-	query = isc_mem_get(fctx->mctx, sizeof(*query));
-	*query = (resquery_t){
-		.options = options,
-		.addrinfo = addrinfo,
-		.dispatchmgr = res->view->dispatchmgr,
-		.link = ISC_LINK_INITIALIZER,
-	};
-
-#if DNS_RESOLVER_TRACE
-	fprintf(stderr, "rctx_init:%s:%s:%d:%p->references = 1\n", __func__,
-		__FILE__, __LINE__, query);
-#endif
-	isc_refcount_init(&query->references, 1);
-
-	/*
-	 * Note that the caller MUST guarantee that 'addrinfo' will
-	 * remain valid until this query is canceled.
-	 */
-
-	dns_message_create(fctx->mctx, fctx->res->namepools[fctx->tid],
-			   fctx->res->rdspools[fctx->tid],
-			   DNS_MESSAGE_INTENTPARSE, &query->rmessage);
-	query->start = isc_time_now();
-
 	/*
 	 * Maybe apply DNS64 mappings to IPv4 addresses.
 	 */
@@ -1996,6 +1947,11 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 				      sockaddrbuf2, sockaddrbuf1);
 		}
 	}
+
+	/*
+	 * Check if the address is in the peers list and has a special
+	 * confguration.
+	 */
 	if (res->view->peers != NULL) {
 		dns_peer_t *peer = NULL;
 		isc_netaddr_t dstip;
@@ -2010,11 +1966,60 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 			}
 			result = dns_peer_getforcetcp(peer, &usetcp);
 			if (result == ISC_R_SUCCESS && usetcp) {
-				query->options |= DNS_FETCHOPT_TCP;
+				options |= DNS_FETCHOPT_TCP;
 			}
 		}
 	}
 
+	/*
+	 * Allow an additional second for the kernel to resend the SYN
+	 * (or SYN without ECN in the case of stupid firewalls blocking
+	 * ECN negotiation) over the current RTT estimate.
+	 */
+	if ((options & DNS_FETCHOPT_TCP) != 0) {
+		srtt += US_PER_SEC;
+	}
+
+	/*
+	 * A forwarder needs to make multiple queries. Give it at least
+	 * a second to do these in.
+	 */
+	if (ISFORWARDER(addrinfo) && srtt < US_PER_SEC) {
+		srtt = US_PER_SEC;
+	}
+
+	fctx_setretryinterval(fctx, srtt);
+	if (isc_interval_iszero(&fctx->interval)) {
+		FCTXTRACE("fetch expired");
+		return ISC_R_TIMEDOUT;
+	}
+
+	INSIST(ISC_LIST_EMPTY(fctx->validators));
+
+	query = isc_mem_get(fctx->mctx, sizeof(*query));
+	*query = (resquery_t){
+		.options = options,
+		.addrinfo = addrinfo,
+		.dispatchmgr = res->view->dispatchmgr,
+		.link = ISC_LINK_INITIALIZER,
+	};
+
+#if DNS_RESOLVER_TRACE
+	fprintf(stderr, "rctx_init:%s:%s:%d:%p->references = 1\n", __func__,
+		__FILE__, __LINE__, query);
+#endif
+	isc_refcount_init(&query->references, 1);
+
+	/*
+	 * Note that the caller MUST guarantee that 'addrinfo' will
+	 * remain valid until this query is canceled.
+	 */
+
+	dns_message_create(fctx->mctx, fctx->res->namepools[fctx->tid],
+			   fctx->res->rdspools[fctx->tid],
+			   DNS_MESSAGE_INTENTPARSE, &query->rmessage);
+	query->start = isc_time_now();
+
 	/*
 	 * If this is a TCP query, then we need to make a socket and
 	 * a dispatch for it here.  Otherwise we use the resolver's