Keep track of allow client detach

The stale-answer-client-timeout feature introduced a dependancy on
when a client may be detached from the handle. The dboption
DNS_DBFIND_STALEONLY was reused to track this attribute. This overloads
the meaning of this database option, and actually introduced a bug
because the option was checked in other places. In particular, in
'ns_query_done()' there is a check for 'RECURSING(qctx->client) &&
(!QUERY_STALEONLY(&qctx->client->query) || ...' and the condition is
satisfied because recursion has not completed yet and
DNS_DBFIND_STALEONLY is already cleared by that time (in
query_lookup()), because we found a useful answer and we should detach
the client from the handle after sending the response.

Add a new boolean to the client structure to keep track of client
detach from handle is allowed or not. It is only disallowed if we are
in a staleonly lookup and we didn't found a useful answer.

diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h
index fb7b86ffbfb0ee5a7becdba2897899df2871a841..737d5e417da0d3b8c45fad64be968ee6d59da706 100644 (file)
--- a/lib/ns/include/ns/client.h
+++ b/lib/ns/include/ns/client.h
@@ -178,6 +178,7 @@ struct ns_client {
        ns_clientmgr_t * manager;
        ns_clientstate_t state;
        int              nupdates;
+       bool             nodetach;
        bool             shuttingdown;
        unsigned int     attributes;
        isc_task_t *     task;

diff --git a/lib/ns/query.c b/lib/ns/query.c
index da4943250cee54b1682376454330fcee79749640..763a6fb0717d54151d7b7a52b372317f38cb902d 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -568,7 +568,7 @@ query_send(ns_client_t *client) {
        inc_stats(client, counter);
        ns_client_send(client);
 
-       if (!QUERY_STALEONLY(&client->query)) {
+       if (!client->nodetach) {
                isc_nmhandle_detach(&client->reqhandle);
        }
 }
@@ -598,7 +598,7 @@ query_error(ns_client_t *client, isc_result_t result, int line) {
 
        ns_client_error(client, result);
 
-       if (!QUERY_STALEONLY(&client->query)) {
+       if (!client->nodetach) {
                isc_nmhandle_detach(&client->reqhandle);
        }
 }
@@ -614,7 +614,7 @@ query_next(ns_client_t *client, isc_result_t result) {
        }
        ns_client_drop(client, result);
 
-       if (!QUERY_STALEONLY(&client->query)) {
+       if (!client->nodetach) {
                isc_nmhandle_detach(&client->reqhandle);
        }
 }
@@ -5196,7 +5196,7 @@ qctx_freedata(query_ctx_t *qctx) {
                dns_db_detach(&qctx->zdb);
        }
 
-       if (qctx->event != NULL && !QUERY_STALEONLY(&qctx->client->query)) {
+       if (qctx->event != NULL && !qctx->client->nodetach) {
                free_devent(qctx->client, ISC_EVENT_PTR(&qctx->event),
                            &qctx->event);
        }
@@ -5785,6 +5785,7 @@ query_lookup(query_ctx_t *qctx) {
                 * active RRset is not available.
                 */
                qctx->client->query.dboptions |= DNS_DBFIND_STALEONLY;
+               qctx->client->nodetach = true;
        }
 
        dboptions = qctx->client->query.dboptions;
@@ -5946,6 +5947,7 @@ query_lookup(query_ctx_t *qctx) {
                                                &qctx->db);
                                        qctx->client->query.dboptions &=
                                                ~DNS_DBFIND_STALEONLY;
+                                       qctx->client->nodetach = false;
                                        qctx->options &= ~DNS_GETDB_STALEFIRST;
                                        if (qctx->client->query.fetch != NULL) {
                                                dns_resolver_destroyfetch(
@@ -5992,7 +5994,7 @@ query_lookup(query_ctx_t *qctx) {
                 * actually not a 'stale-only' lookup. Clear the flag to
                 * allow the client to be detach the handle.
                 */
-               qctx->client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
+               qctx->client->nodetach = false;
        }
 
        result = query_gotanswer(qctx, result);
@@ -6027,6 +6029,7 @@ query_lookup_staleonly(ns_client_t *client) {
        dns_db_attach(client->view->cachedb, &qctx.db);
        client->query.attributes &= ~NS_QUERYATTR_RECURSIONOK;
        client->query.dboptions |= DNS_DBFIND_STALEONLY;
+       client->nodetach = true;
        (void)query_lookup(&qctx);
        if (qctx.node != NULL) {
                dns_db_detachnode(qctx.db, &qctx.node);
@@ -6068,10 +6071,11 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
                query_lookup_staleonly(client);
                isc_event_free(ISC_EVENT_PTR(&event));
                return;
-       } else {
-               client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
        }
 
+       client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
+       client->nodetach = false;
+
        LOCK(&client->query.fetchlock);
        if (client->query.fetch != NULL) {
                /*
@@ -11536,7 +11540,7 @@ isc_result_t
 ns_query_done(query_ctx_t *qctx) {
        isc_result_t result = ISC_R_UNSET;
        const dns_namelist_t *secs = qctx->client->message->sections;
-       bool query_stale_only;
+       bool nodetach;
 
        CCTRACE(ISC_LOG_DEBUG(3), "ns_query_done");
 
@@ -11648,10 +11652,9 @@ ns_query_done(query_ctx_t *qctx) {
         * Client may have been detached after query_send(), so
         * we test and store the flag state here, for safety.
         */
-       query_stale_only = QUERY_STALEONLY(&qctx->client->query);
+       nodetach = qctx->client->nodetach;
        query_send(qctx->client);
-
-       if (!query_stale_only) {
+       if (!nodetach) {
                qctx->detach_client = true;
        }
        return (qctx->result);