3333. [bug] Setting resolver-query-timeout too low can cause

named to not recover if it looses connectivity.
                        [RT #29623]

CHANGES

@@ -1,3 +1,7 @@
+3333.	[bug]		Setting resolver-query-timeout too low can cause
+			named to not recover if it looses connectivity.
+			[RT #29623]
+
 3332.	[bug]		Re-use cached DS rrsets if possible. [RT 29446]
 
 3331.	[security]	dns_rdataslab_fromrdataset could produce bad

bin/named/config.c

@@ -89,7 +89,7 @@ options {\n\
 #endif
 "\
 	recursive-clients 1000;\n\
-	resolver-query-timeout 30;\n\
+	resolver-query-timeout 10;\n\
 	rrset-order { order random; };\n\
 	serial-queries 20;\n\
 	serial-query-rate 20;\n\

bin/tests/resolver/t_resolver.c

@@ -154,11 +154,11 @@ test_dns_resolver_settimeout(void) {
 	t_info("The default timeout is %d second%s\n", default_timeout,
 	       (default_timeout == 1 ? "" : "s"));
 
-	dns_resolver_settimeout(resolver, default_timeout - 1);
+	dns_resolver_settimeout(resolver, default_timeout + 1);
 	timeout = dns_resolver_gettimeout(resolver);
 	t_info("The new timeout is %d second%s\n", timeout,
 	       (timeout == 1 ? "" : "s"));
-	test_result = (timeout == default_timeout - 1) ? T_PASS : T_FAIL;
+	test_result = (timeout == default_timeout + 1) ? T_PASS : T_FAIL;
 
 	destroy_resolver(&resolver);
 	teardown();

doc/arm/Bv9ARM-book.xml

@@ -7474,8 +7474,8 @@ options {
               <listitem>
                 <para>
 		  The amount of time the resolver will spend attempting
-		  to resolve a recursive query before failing.  The
+		  to resolve a recursive query before failing.  The default
-		  default is <literal>10</literal> and the maximum is
+		  and minimum is <literal>10</literal> and the maximum is
 		  <literal>30</literal>.  Setting it to <literal>0</literal>
 		  will result in the default being used.
                 </para>

lib/dns/resolver.c

@@ -106,8 +106,21 @@
 #define QTRACE(m)
 #endif
 
+#define US_PER_SEC 1000000U
+/*
+ * The maximum time we will wait for a single query.
+ */
+#define MAX_SINGLE_QUERY_TIMEOUT 9U
+#define MAX_SINGLE_QUERY_TIMEOUT_US (MAX_SINGLE_QUERY_TIMEOUT*US_PER_SEC)
+
+/*
+ * We need to allow a individual query time to complete / timeout.
+ */
+#define MINIMUM_QUERY_TIMEOUT (MAX_SINGLE_QUERY_TIMEOUT + 1U)
+
+/* The default time in seconds for the whole query to live. */
 #ifndef DEFAULT_QUERY_TIMEOUT
-#define DEFAULT_QUERY_TIMEOUT 10  /* The default time in seconds for the whole query to live. */
+#define DEFAULT_QUERY_TIMEOUT MINIMUM_QUERY_TIMEOUT
 #endif
 
 #ifndef MAXIMUM_QUERY_TIMEOUT
@@ -827,8 +840,8 @@ fctx_cancelquery(resquery_t **queryp, dns_dispatchevent_t **deventp,
 			 */
 			INSIST(no_response);
 			rtt = query->addrinfo->srtt + 200000;
-			if (rtt > 10000000)
+			if (rtt > MAX_SINGLE_QUERY_TIMEOUT_US)
-				rtt = 10000000;
+				rtt = MAX_SINGLE_QUERY_TIMEOUT_US;
 			/*
 			 * Replace the current RTT with our value.
 			 */
@@ -1343,12 +1356,18 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) {
 		us = (800000 << (fctx->restarts - 2));
 
 	/*
-	 * Double the round-trip time.
+	 * Add a fudge factor to the expected rtt based on the current
+	 * estimate.
 	 */
-	rtt *= 2;
+	if (rtt < 50000)
+		rtt += 50000;
+	else if (rtt < 100000)
+		rtt += 100000;
+	else
+		rtt += 200000;
 
 	/*
-	 * Always wait for at least the doubled round-trip time.
+	 * Always wait for at least the expected rtt.
 	 */
 	if (us < rtt)
 		us = rtt;
@@ -1356,11 +1375,11 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) {
 	/*
 	 * But don't ever wait for more than 10 seconds.
 	 */
-	if (us > 10000000)
+	if (us > MAX_SINGLE_QUERY_TIMEOUT_US)
-		us = 10000000;
+		us = MAX_SINGLE_QUERY_TIMEOUT_US;
 
-	seconds = us / 1000000;
+	seconds = us / US_PER_SEC;
-	us -= seconds * 1000000;
+	us -= seconds * US_PER_SEC;
 	isc_interval_set(&fctx->interval, seconds, us * 1000);
 }
 
@@ -1382,6 +1401,11 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 	task = res->buckets[fctx->bucketnum].task;
 
 	srtt = addrinfo->srtt;
+
+	/*
+	 * A forwarder needs to make multiple queries. Give it at least
+	 * a second to do these in.
+	 */
 	if (ISFORWARDER(addrinfo) && srtt < 1000000)
 		srtt = 1000000;
 
@@ -8191,8 +8215,8 @@ dns_resolver_logfetch(dns_fetch_t *fetch, isc_log_t *lctx,
 			      "timeout:%u,lame:%u,neterr:%u,badresp:%u,"
 			      "adberr:%u,findfail:%u,valfail:%u]",
 			      __FILE__, fctx->exitline, fctx->info,
-			      fctx->duration / 1000000,
+			      fctx->duration / US_PER_SEC,
-			      fctx->duration % 1000000,
+			      fctx->duration % US_PER_SEC,
 			      isc_result_totext(fctx->result),
 			      isc_result_totext(fctx->vresult), domainbuf,
 			      fctx->referrals, fctx->restarts,
@@ -8797,6 +8821,8 @@ dns_resolver_settimeout(dns_resolver_t *resolver, unsigned int seconds) {
 		seconds = DEFAULT_QUERY_TIMEOUT;
 	if (seconds > MAXIMUM_QUERY_TIMEOUT)
 		seconds = MAXIMUM_QUERY_TIMEOUT;
+	if (seconds < MINIMUM_QUERY_TIMEOUT)
+		seconds =  MINIMUM_QUERY_TIMEOUT;
 
 	resolver->query_timeout = seconds;
 }