From 8bcd7fe69e5343071fc917738d6092a8b974ef3f Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 4 Feb 2021 13:57:01 +0100
Subject: [PATCH] Use stale on error also when unable to recurse

The 'query_usestale()' function was only called when in
'query_gotanswer()' and an unexpected error occurred. This may have
been "quota reached", and thus we were in some cases returning
stale data on fetch-limits (and if serve-stale enabled of course).

But we can also hit fetch-limits when recursing because we are
following a referral (in 'query_notfound()' and
'query_delegation_recurse()'). Here we should also check for using
stale data in case an error occurred.

Specifically don't check for using stale data when refetching a
zero TTL RRset from cache.

Move the setting of DNS_DBFIND_STALESTART into the 'query_usestale()'
function to avoid code duplication.
---
 lib/ns/query.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/lib/ns/query.c b/lib/ns/query.c
index e7a666ef2a..9d5844084f 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -7440,7 +7440,7 @@ root_key_sentinel_return_servfail(query_ctx_t *qctx, isc_result_t result) {
  * return true; otherwise, return false.
  */
 static bool
-query_usestale(query_ctx_t *qctx) {
+query_usestale(query_ctx_t *qctx, isc_result_t result) {
 	if ((qctx->client->query.dboptions & DNS_DBFIND_STALEOK) != 0) {
 		/*
 		 * Query was already using stale, if that didn't work the
@@ -7459,6 +7459,13 @@ query_usestale(query_ctx_t *qctx) {
 			dns_resolver_destroyfetch(&qctx->client->query.fetch);
 		}
 
+		/*
+		 * Start the stale-refresh-time window in case there was a
+		 * resolver query timeout.
+		 */
+		if (qctx->resuming && result == ISC_R_TIMEDOUT) {
+			qctx->client->query.dboptions |= DNS_DBFIND_STALESTART;
+		}
 		return (true);
 	}
 
@@ -7556,19 +7563,11 @@ query_gotanswer(query_ctx_t *qctx, isc_result_t res) {
 			 "query_gotanswer: unexpected error: %s",
 			 isc_result_totext(result));
 		CCTRACE(ISC_LOG_ERROR, errmsg);
-		if (query_usestale(qctx)) {
+		if (query_usestale(qctx, result)) {
 			/*
 			 * If serve-stale is enabled, query_usestale() already
 			 * set up 'qctx' for looking up a stale response.
-			 *
-			 * We only need to check if the query timed out or
-			 * something else has gone wrong. If the query timed
-			 * out, we will start the stale-refresh-time window.
 			 */
-			if (qctx->resuming && result == ISC_R_TIMEDOUT) {
-				qctx->client->query.dboptions |=
-					DNS_DBFIND_STALESTART;
-			}
 			return (query_lookup(qctx));
 		}
 
@@ -8527,6 +8526,13 @@ query_notfound(query_ctx_t *qctx) {
 					qctx->client->query.attributes |=
 						NS_QUERYATTR_DNS64EXCLUDE;
 				}
+			} else if (query_usestale(qctx, result)) {
+				/*
+				 * If serve-stale is enabled, query_usestale()
+				 * already set up 'qctx' for looking up a
+				 * stale response.
+				 */
+				return (query_lookup(qctx));
 			} else {
 				QUERY_ERROR(qctx, result);
 			}
@@ -8831,6 +8837,12 @@ query_delegation_recurse(query_ctx_t *qctx) {
 			qctx->client->query.attributes |=
 				NS_QUERYATTR_DNS64EXCLUDE;
 		}
+	} else if (query_usestale(qctx, result)) {
+		/*
+		 * If serve-stale is enabled, query_usestale() already set up
+		 * 'qctx' for looking up a stale response.
+		 */
+		return (query_lookup(qctx));
 	} else {
 		QUERY_ERROR(qctx, result);
 	}
@@ -10225,6 +10237,10 @@ query_zerottl_refetch(query_ctx_t *qctx) {
 				NS_QUERYATTR_DNS64EXCLUDE;
 		}
 	} else {
+		/*
+		 * There was a zero ttl from the cache, don't fallback to
+		 * serve-stale lookup.
+		 */
 		QUERY_ERROR(qctx, result);
 	}