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 99fee3e775bb516e739c779326260f73caafc0e1..f9c6f64f71b64c68954015305faf75c7d32ee49b 100644 (file)
--- 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