From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 20 Apr 2023 14:22:53 +0000 (+0200)
Subject: Fix serve-stale bug when cache has no data
X-Git-Tag: v9.19.14~20^2~2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bbd163acf67843c76099921e467dd0ef90f3f670;p=thirdparty%2Fbind9.git

Fix serve-stale bug when cache has no data

We recently fixed a bug where in some cases (when following an
expired CNAME for example), named could return SERVFAIL if the target
record is still valid (see isc-projects/bind9#3678, and
isc-projects/bind9!7096). We fixed this by considering non-stale
RRsets as well during the stale lookup.

However, this triggered a new bug because despite the answer from
cache not being stale, the lookup may be triggered by serve-stale.
If the answer from database is not stale, the fix in
isc-projects/bind9!7096 erroneously skips the serve-stale logic.

Add 'answer_found' checks to the serve-stale logic to fix this issue.
---

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 99fee3e775b..f9c6f64f71b 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5853,6 +5853,27 @@ qctx_prepare_buffers(query_ctx_t *qctx, isc_buffer_t *buffer) {
 	return (ISC_R_SUCCESS);
 }
 
+/*%
+ * Depending on the db lookup result, we can respond to the
+ * client this stale answer.
+ */
+static bool
+stale_client_answer(isc_result_t result) {
+	switch (result) {
+	case ISC_R_SUCCESS:
+	case DNS_R_EMPTYNAME:
+	case DNS_R_NXRRSET:
+	case DNS_R_NCACHENXRRSET:
+	case DNS_R_CNAME:
+	case DNS_R_DNAME:
+		return (true);
+	default:
+		return (false);
+	}
+
+	UNREACHABLE();
+}
+
 /*%
  * Perform a local database lookup, in either an authoritative or
  * cache database. If unable to answer, call ns_query_done(); otherwise
@@ -5985,7 +6006,6 @@ query_lookup(query_ctx_t *qctx) {
 	{
 		/* Found non-stale usable rdataset. */
 		answer_found = true;
-		goto gotanswer;
 	}
 
 	if (dbfind_stale || stale_refresh_window || stale_timeout) {
@@ -6022,7 +6042,7 @@ query_lookup(query_ctx_t *qctx) {
 		if (stale_found) {
 			ns_client_extendederror(qctx->client, ede,
 						"resolver failure");
-		} else {
+		} else if (!answer_found) {
 			/*
 			 * Resolver failure, no stale data, nothing more we
 			 * can do, return SERVFAIL.
@@ -6046,7 +6066,7 @@ query_lookup(query_ctx_t *qctx) {
 			ns_client_extendederror(
 				qctx->client, ede,
 				"query within stale refresh time window");
-		} else {
+		} else if (!answer_found) {
 			/*
 			 * During the stale refresh window explicitly do not try
 			 * to refresh the data, because a recent lookup failed.
@@ -6056,7 +6076,7 @@ query_lookup(query_ctx_t *qctx) {
 		}
 	} else if (stale_timeout) {
 		if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
-			if (!stale_found) {
+			if (!stale_found && !answer_found) {
 				/*
 				 * We have nothing useful in cache to return
 				 * immediately.
@@ -6075,7 +6095,7 @@ query_lookup(query_ctx_t *qctx) {
 							qctx->client));
 				}
 				return (query_lookup(qctx));
-			} else {
+			} else if (stale_client_answer(result)) {
 				/*
 				 * Immediately return the stale answer, start a
 				 * resolver fetch to refresh the data in cache.
@@ -6088,9 +6108,12 @@ query_lookup(query_ctx_t *qctx) {
 					"made",
 					namebuf, typebuf);
 				qctx->refresh_rrset = STALE(qctx->rdataset);
-				ns_client_extendederror(
-					qctx->client, ede,
-					"stale data prioritized over lookup");
+				if (stale_found) {
+					ns_client_extendederror(
+						qctx->client, ede,
+						"stale data prioritized over "
+						"lookup");
+				}
 			}
 		} else {
 			/*
@@ -6106,7 +6129,11 @@ query_lookup(query_ctx_t *qctx) {
 			if (stale_found) {
 				ns_client_extendederror(qctx->client, ede,
 							"client timeout");
-			} else {
+			} else if (!answer_found) {
+				return (result);
+			}
+
+			if (!stale_client_answer(result)) {
 				return (result);
 			}
 
@@ -6119,7 +6146,6 @@ query_lookup(query_ctx_t *qctx) {
 		}
 	}
 
-gotanswer:
 	if (stale_timeout && (answer_found || stale_found)) {
 		/*
 		 * Mark RRsets that we are adding to the client message on a