Clear REDIRECT flag when it isn't needed

The NS_QUERYATTR_REDIRECT flag is set when processing a recursive
NXDOMAIN redirection lookup, so that if that lookup also returns
NXDOMAIN we don't end up looping.

Previously, the flag was left active after use, but if the
same client triggered a subsequent recursive lookup (for example,
in the filter-aaaa plugin), then the wrong branch could be reached
in query_resume(), potentially leading to an assertion failure.  This
has been fixed.

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 28cbf992502c272f120c0f7ce3a5b12cd4b4ead0..b160171935ff11e35c91d9e7b9a2b46fbeaadc78 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -4840,9 +4840,12 @@ redirect2(ns_client_t *client, dns_name_t *name, dns_rdataset_t *rdataset,
        dns_zone_t *zone = NULL;
        bool is_zone;
        unsigned int labels;
+       bool redirected = REDIRECT(client);
 
        CTRACE(ISC_LOG_DEBUG(3), "redirect2");
 
+       client->query.attributes &= ~NS_QUERYATTR_REDIRECT;
+
        if (client->inner.view->redirectzone == NULL) {
                return ISC_R_NOTFOUND;
        }
@@ -4932,17 +4935,17 @@ redirect2(ns_client_t *client, dns_name_t *name, dns_rdataset_t *rdataset,
                        dns_db_detachnode(&node);
                }
                dns_db_detach(&db);
+
                /*
                 * Don't loop forever if the lookup failed last time.
                 */
-               if (!REDIRECT(client)) {
+               if (!redirected) {
                        result = ns_query_recurse(client, qtype, redirectname,
                                                  NULL, NULL, true);
                        if (result == ISC_R_SUCCESS) {
                                client->query.attributes |=
-                                       NS_QUERYATTR_RECURSING;
-                               client->query.attributes |=
-                                       NS_QUERYATTR_REDIRECT;
+                                       (NS_QUERYATTR_RECURSING |
+                                        NS_QUERYATTR_REDIRECT);
                                return DNS_R_CONTINUE;
                        }
                }
@@ -6318,6 +6321,7 @@ query_resume(query_ctx_t *qctx) {
        char qbuf[DNS_NAME_FORMATSIZE];
        char tbuf[DNS_RDATATYPE_FORMATSIZE];
 #endif /* ifdef WANT_QUERYTRACE */
+       bool redirect = REDIRECT(qctx->client);
 
        CCTRACE(ISC_LOG_DEBUG(3), "query_resume");
 
@@ -6326,9 +6330,10 @@ query_resume(query_ctx_t *qctx) {
        qctx->want_restart = false;
 
        qctx->rpz_st = qctx->client->query.rpz_st;
-       if (qctx->rpz_st != NULL &&
-           (qctx->rpz_st->state & DNS_RPZ_RECURSING) != 0)
-       {
+       bool rpz = (qctx->rpz_st != NULL &&
+                   (qctx->rpz_st->state & DNS_RPZ_RECURSING) != 0);
+
+       if (rpz) {
                CCTRACE(ISC_LOG_DEBUG(3), "resume from RPZ recursion");
 #ifdef WANT_QUERYTRACE
                {
@@ -6373,7 +6378,7 @@ query_resume(query_ctx_t *qctx) {
                qctx->rpz_st->r.r_rdataset =
                        MOVE_OWNERSHIP(qctx->fresp->rdataset);
                ns_client_putrdataset(qctx->client, &qctx->fresp->sigrdataset);
-       } else if (REDIRECT(qctx->client)) {
+       } else if (redirect) {
                /*
                 * Restore saved state.
                 */
@@ -6441,9 +6446,7 @@ query_resume(query_ctx_t *qctx) {
                qctx->dns64_exclude = true;
        }
 
-       if (qctx->rpz_st != NULL &&
-           (qctx->rpz_st->state & DNS_RPZ_RECURSING) != 0)
-       {
+       if (rpz) {
                /*
                 * Has response policy changed out from under us?
                 */
@@ -6465,11 +6468,9 @@ query_resume(query_ctx_t *qctx) {
        qctx->dbuf = ns_client_getnamebuf(qctx->client);
        qctx->fname = ns_client_newname(qctx->client, qctx->dbuf, &b);
 
-       if (qctx->rpz_st != NULL &&
-           (qctx->rpz_st->state & DNS_RPZ_RECURSING) != 0)
-       {
+       if (rpz) {
                tname = qctx->rpz_st->fname;
-       } else if (REDIRECT(qctx->client)) {
+       } else if (redirect) {
                tname = qctx->client->query.redirect.fname;
        } else {
                tname = qctx->fresp->foundname;
@@ -6477,14 +6478,25 @@ query_resume(query_ctx_t *qctx) {
 
        dns_name_copy(tname, qctx->fname);
 
-       if (qctx->rpz_st != NULL &&
-           (qctx->rpz_st->state & DNS_RPZ_RECURSING) != 0)
-       {
+       if (rpz) {
                qctx->rpz_st->r.r_result = qctx->fresp->result;
                result = qctx->rpz_st->q.result;
                free_fresp(qctx->client, &qctx->fresp);
-       } else if (REDIRECT(qctx->client)) {
+       } else if (redirect) {
                result = qctx->client->query.redirect.result;
+
+               /*
+                * If we got an answer from a redirect query that could
+                * trigger another redirect, keep the REDIRECT flag set
+                * so we can avoid looping; we'll clear it later.
+                * Otherwise, we're done with it now.
+                */
+               if (result != DNS_R_COVERINGNSEC && result != DNS_R_NXDOMAIN &&
+                   result != DNS_R_NCACHENXDOMAIN)
+               {
+                       qctx->client->query.attributes &=
+                               ~NS_QUERYATTR_REDIRECT;
+               }
        } else {
                result = qctx->fresp->result;
        }
@@ -9146,8 +9158,6 @@ query_nxdomain(query_ctx_t *qctx, isc_result_t result) {
 
        CALL_HOOK(NS_QUERY_NXDOMAIN_BEGIN, qctx);
 
-       INSIST(qctx->is_zone || REDIRECT(qctx->client));
-
        if (!empty_wild) {
                result = query_redirect(qctx, result);
                if (result != ISC_R_COMPLETE) {
@@ -9232,6 +9242,10 @@ cleanup:
  *
  * Any result code other than ISC_R_COMPLETE means redirection was
  * successful and the result code should be returned up the call stack.
+ * DNS_R_CONTINUE means we've initiated a recursive query to the
+ * redirect zone, and we'll resume processing with the answer to that
+ * in query_resume(); other results mean we have the redirected answer
+ * now.
  *
  * ISC_R_COMPLETE means we reached the end of this function without
  * redirecting, so query processing should continue past it.