Remove INSIST on NS_QUERYATTR_ANSWERED

The NS_QUERYATTR_ANSWERED attribute is to prevent sending a response
twice. Without the attribute, this may happen if a staleonly lookup
found a useful answer and sends a response to the client, and later
recursion ends and also tries to send a response.

The attribute was also used to mask adding a duplicate RRset. This is
considered harmful. When we created a response to the client with a
stale only lookup (regardless if we actually have send the response),
we should clear the rdatasets that were added during that lookup.

Mark such rdatasets with the a new attribute,
DNS_RDATASETATTR_STALE_ADDED. Set a query attribute
NS_QUERYATTR_STALEOK if we may have added rdatasets during a stale
only lookup. Before creating a response on a normal lookup, check if
we can expect rdatasets to have been added during a staleonly lookup.
If so, clear the rdatasets from the message with the attribute
DNS_RDATASETATTR_STALE_ADDED set.

diff --git a/lib/dns/include/dns/rdataset.h b/lib/dns/include/dns/rdataset.h
index 9f493e191cef2853a71c38b544da7d8a9b865a57..f12419bcac48616edf92bc3f0f67cfc222b628b1 100644 (file)
--- a/lib/dns/include/dns/rdataset.h
+++ b/lib/dns/include/dns/rdataset.h
@@ -190,6 +190,7 @@ struct dns_rdataset {
 #define DNS_RDATASETATTR_STALE       0x01000000
 #define DNS_RDATASETATTR_ANCIENT      0x02000000
 #define DNS_RDATASETATTR_STALE_WINDOW 0x04000000
+#define DNS_RDATASETATTR_STALE_ADDED  0x08000000
 
 /*%
  * _OMITDNSSEC:

diff --git a/lib/ns/query.c b/lib/ns/query.c
index c7fa531b1e71a5510079008dba319d3563d341ab..7e23a087a55bdc2e0891a8874fab612ab5aab590 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -136,6 +136,9 @@
 /*% Was the query already answered due to stale-answer-client-timeout? */
 #define QUERY_ANSWERED(q) (((q)->attributes & NS_QUERYATTR_ANSWERED) != 0)
 
+/*% Does the query allow stale data in the response? */
+#define QUERY_STALEOK(q) (((q)->attributes & NS_QUERYATTR_STALEOK) != 0)
+
 /*% Does the query only wants to check for stale RRset? */
 #define QUERY_STALEONLY(q) (((q)->dboptions & DNS_DBFIND_STALEONLY) != 0)
 
@@ -495,6 +498,9 @@ query_addwildcardproof(query_ctx_t *qctx, bool ispositive, bool nodata);
 static void
 query_addauth(query_ctx_t *qctx);
 
+static void
+query_clear_staleonly(ns_client_t *client);
+
 /*
  * Increment query statistics counters.
  */
@@ -2193,6 +2199,10 @@ query_addrrset(query_ctx_t *qctx, dns_name_t **namep,
                if ((rdataset->attributes & DNS_RDATASETATTR_REQUIRED) != 0) {
                        mrdataset->attributes |= DNS_RDATASETATTR_REQUIRED;
                }
+               if ((rdataset->attributes & DNS_RDATASETATTR_STALE_ADDED) != 0)
+               {
+                       mrdataset->attributes |= DNS_RDATASETATTR_STALE_ADDED;
+               }
                return;
        } else if (result == DNS_R_NXDOMAIN) {
                /*
@@ -5858,9 +5868,7 @@ query_lookup(query_ctx_t *qctx) {
         */
        stale_only = ((dboptions & DNS_DBFIND_STALEONLY) != 0);
 
-       if (dbfind_stale ||
-           (STALE(qctx->rdataset) && (stale_only || stale_refresh_window)))
-       {
+       if (dbfind_stale || stale_refresh_window || stale_only) {
                dns_name_format(qctx->client->query.qname, namebuf,
                                sizeof(namebuf));
 
@@ -5910,6 +5918,9 @@ query_lookup(query_ctx_t *qctx) {
                        return (ns_query_done(qctx));
                }
        } else if (stale_only) {
+               qctx->client->query.attributes |= NS_QUERYATTR_STALEOK;
+               qctx->rdataset->attributes |= DNS_RDATASETATTR_STALE_ADDED;
+
                if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
                        if (!stale_found || result != ISC_R_SUCCESS) {
                                /*
@@ -5961,6 +5972,15 @@ query_lookup(query_ctx_t *qctx) {
                }
        }
 
+       if (stale_timeout && stale_found) {
+               /*
+                * Mark RRsets that we are adding to the client message on a
+                * lookup during 'stale-answer-client-timeout', so we can
+                * clean it up if needed when we resume from recursion.
+                */
+               qctx->client->query.attributes |= NS_QUERYATTR_STALEOK;
+               qctx->rdataset->attributes |= DNS_RDATASETATTR_STALE_ADDED;
+       }
        result = query_gotanswer(qctx, result);
 
        if (refresh_rrset) {
@@ -5978,6 +5998,60 @@ cleanup:
        return (result);
 }
 
+/*
+ * Clear all rdatasets from the message that are in the given section and
+ * that have the 'attr' attribute set.
+ */
+static void
+message_clearrdataset(dns_message_t *msg, unsigned int attr) {
+       unsigned int i;
+       dns_name_t *name, *next_name;
+       dns_rdataset_t *rds, *next_rds;
+
+       /*
+        * Clean up name lists by calling the rdataset disassociate function.
+        */
+       for (i = DNS_SECTION_ANSWER; i < DNS_SECTION_MAX; i++) {
+               name = ISC_LIST_HEAD(msg->sections[i]);
+               while (name != NULL) {
+                       next_name = ISC_LIST_NEXT(name, link);
+
+                       rds = ISC_LIST_HEAD(name->list);
+                       while (rds != NULL) {
+                               next_rds = ISC_LIST_NEXT(rds, link);
+                               if ((rds->attributes & attr) != attr) {
+                                       rds = next_rds;
+                                       continue;
+                               }
+                               ISC_LIST_UNLINK(name->list, rds, link);
+                               INSIST(dns_rdataset_isassociated(rds));
+                               dns_rdataset_disassociate(rds);
+                               isc_mempool_put(msg->rdspool, rds);
+                               rds = next_rds;
+                       }
+
+                       if (ISC_LIST_EMPTY(name->list)) {
+                               ISC_LIST_UNLINK(msg->sections[i], name, link);
+                               if (dns_name_dynamic(name)) {
+                                       dns_name_free(name, msg->mctx);
+                               }
+                               isc_mempool_put(msg->namepool, name);
+                       }
+
+                       name = next_name;
+               }
+       }
+}
+
+/*
+ * Clear any rdatasets from the client's message that were added on a
+ * stale-only lookup.
+ */
+static void
+query_clear_staleonly(ns_client_t *client) {
+       message_clearrdataset(client->message, DNS_RDATASETATTR_STALE_ADDED);
+}
+
 /*
  * Create a new query context with the sole intent
  * of looking up for a stale RRset in cache.
@@ -7927,6 +8001,21 @@ query_addanswer(query_ctx_t *qctx) {
 
        CALL_HOOK(NS_QUERY_ADDANSWER_BEGIN, qctx);
 
+       /*
+        * On normal lookups, clear any rdatasets that were added on a
+        * staleonly lookup.
+        */
+       if (QUERY_STALEOK(&qctx->client->query) &&
+           !QUERY_STALEONLY(&qctx->client->query))
+       {
+               query_clear_staleonly(qctx->client);
+               /*
+                * We can clear the attribute to prevent redundant clearing
+                * in subsequent lookups.
+                */
+               qctx->client->query.attributes &= ~DNS_RDATASETATTR_STALE_ADDED;
+       }
+
        if (qctx->dns64) {
                result = query_dns64(qctx);
                qctx->noqname = NULL;
@@ -8070,7 +8159,7 @@ query_respond(query_ctx_t *qctx) {
         * We shouldn't ever fail to add 'rdataset'
         * because it's already in the answer.
         */
-       INSIST(qctx->rdataset == NULL || QUERY_ANSWERED(&qctx->client->query));
+       INSIST(qctx->rdataset == NULL);
 
        query_addauth(qctx);
 
@@ -11587,10 +11676,8 @@ ns_query_done(query_ctx_t *qctx) {
         * to the AA bit if the auth-nxdomain config option
         * says so, then render and send the response.
         */
-       if (!QUERY_ANSWERED(qctx->client)) {
-               query_setup_sortlist(qctx);
-               query_glueanswer(qctx);
-       }
+       query_setup_sortlist(qctx);
+       query_glueanswer(qctx);
 
        if (qctx->client->message->rcode == dns_rcode_nxdomain &&
            qctx->view->auth_nxdomain)