From: Michał Kępień <michal@isc.org>
Date: Mon, 8 Oct 2018 10:47:28 +0000 (+0200)
Subject: Set up stale response lookup before query_done() is called
X-Git-Tag: v9.13.4~116^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cb48d410d89929bbc161a16ab95fb53fd311e5a9;p=thirdparty%2Fbind9.git

Set up stale response lookup before query_done() is called

When something goes wrong while recursing for an answer to a query,
query_gotanswer() sets a flag (qctx->want_stale) in the query context.
query_done() is subsequently called and it can either set up a stale
response lookup (if serve-stale is enabled) or conclude that a SERVFAIL
response should be sent.  This may cause confusion when looking at query
error logs since the QUERY_ERROR() line responsible for setting the
response's RCODE to SERVFAIL is not in a catch-all branch of a switch
statement inside query_gotanswer() (like it is for authoritative
responses) but rather in a code branch which appears to have something
to do with serve-stale, even when the latter is not enabled.

Extract the part of query_done() responsible for checking serve-stale
configuration and optionally setting up a stale response lookup into a
separate function, query_usestale(), shifting the responsibility for
setting the response's RCODE to SERVFAIL to the same QUERY_ERROR() line
in query_gotanswer() which is evaluated for authoritative responses.
---

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 3c09fe39114..036eb6ce914 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -6617,6 +6617,56 @@ root_key_sentinel_return_servfail(query_ctx_t *qctx, isc_result_t result) {
 	return (false);
 }
 
+/*%
+ * If serving stale answers is allowed, set up 'qctx' to look for one and
+ * return true; otherwise, return false.
+ */
+static bool
+query_usestale(query_ctx_t *qctx) {
+	bool staleanswersok = false;
+	dns_ttl_t stale_ttl = 0;
+	isc_result_t result;
+
+	qctx_clean(qctx);
+	qctx_freedata(qctx);
+
+	/*
+	 * Stale answers only make sense if stale_ttl > 0 but we want rndc to
+	 * be able to control returning stale answers if they are configured.
+	 */
+	dns_db_attach(qctx->client->view->cachedb, &qctx->db);
+	result = dns_db_getservestalettl(qctx->db, &stale_ttl);
+	if (result == ISC_R_SUCCESS && stale_ttl > 0)  {
+		switch (qctx->client->view->staleanswersok) {
+		case dns_stale_answer_yes:
+			staleanswersok = true;
+			break;
+		case dns_stale_answer_conf:
+			staleanswersok =
+				qctx->client->view->staleanswersenable;
+			break;
+		case dns_stale_answer_no:
+			staleanswersok = false;
+			break;
+		}
+	} else {
+		staleanswersok = false;
+	}
+
+	if (staleanswersok) {
+		qctx->want_stale = true;
+		qctx->client->query.dboptions |= DNS_DBFIND_STALEOK;
+		inc_stats(qctx->client, ns_statscounter_trystale);
+		if (qctx->client->query.fetch != NULL) {
+			dns_resolver_destroyfetch(&qctx->client->query.fetch);
+		}
+	} else {
+		dns_db_detach(&qctx->db);
+	}
+
+	return (staleanswersok);
+}
+
 /*%
  * Continue after doing a database lookup or returning from
  * recursion, and call out to the next function depending on the
@@ -6706,11 +6756,14 @@ query_gotanswer(query_ctx_t *qctx, isc_result_t result) {
 			 "query_gotanswer: unexpected error: %s",
 			 isc_result_totext(result));
 		CCTRACE(ISC_LOG_ERROR, errmsg);
-		if (qctx->resuming) {
-			qctx->want_stale = true;
-		} else {
-			QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		if (qctx->resuming && query_usestale(qctx)) {
+			/*
+			 * If serve-stale is enabled, query_usestale() already
+			 * set up 'qctx' for looking up a stale response.
+			 */
+			return (query_lookup(qctx));
 		}
+		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
 		return (query_done(qctx));
 	}
 }
@@ -10639,49 +10692,6 @@ query_done(query_ctx_t *qctx) {
 		return (ns__query_start(qctx));
 	}
 
-	if (qctx->want_stale) {
-		dns_ttl_t stale_ttl = 0;
-		isc_result_t result;
-		bool staleanswersok = false;
-
-		/*
-		 * Stale answers only make sense if stale_ttl > 0 but
-		 * we want rndc to be able to control returning stale
-		 * answers if they are configured.
-		 */
-		dns_db_attach(qctx->client->view->cachedb, &qctx->db);
-		result = dns_db_getservestalettl(qctx->db, &stale_ttl);
-		if (result == ISC_R_SUCCESS && stale_ttl > 0)  {
-			switch (qctx->client->view->staleanswersok) {
-			case dns_stale_answer_yes:
-				staleanswersok = true;
-				break;
-			case dns_stale_answer_conf:
-				staleanswersok =
-					qctx->client->view->staleanswersenable;
-				break;
-			case dns_stale_answer_no:
-				staleanswersok = false;
-				break;
-			}
-		} else {
-			staleanswersok = false;
-		}
-
-		if (staleanswersok) {
-			qctx->client->query.dboptions |= DNS_DBFIND_STALEOK;
-			inc_stats(qctx->client, ns_statscounter_trystale);
-			if (qctx->client->query.fetch != NULL)
-				dns_resolver_destroyfetch(
-						   &qctx->client->query.fetch);
-			return (query_lookup(qctx));
-		}
-		dns_db_detach(&qctx->db);
-		qctx->want_stale = false;
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
-		return (query_done(qctx));
-	}
-
 	if (qctx->result != ISC_R_SUCCESS &&
 	    (!PARTIALANSWER(qctx->client) || WANTRECURSION(qctx->client) ||
 	     qctx->result == DNS_R_DROP))