Purge memory pool upon plugin destruction

The typical sequence of events for AAAA queries which trigger recursion
for an A RRset at the same name is as follows:

 1. Original query context is created.
 2. An AAAA RRset is found in cache.
 3. Client-specific data is allocated from the filter-aaaa memory pool.
 4. Recursion is triggered for an A RRset.
 5. Original query context is torn down.

 6. Recursion for an A RRset completes.
 7. A second query context is created.
 8. Client-specific data is retrieved from the filter-aaaa memory pool.
 9. The response to be sent is processed according to configuration.
10. The response is sent.
11. Client-specific data is returned to the filter-aaaa memory pool.
12. The second query context is torn down.

However, steps 6-12 are not executed if recursion for an A RRset is
canceled.  Thus, if named is in the process of recursing for A RRsets
when a shutdown is requested, the filter-aaaa memory pool will have
outstanding allocations which will never get released.  This in turn
leads to a crash since every memory pool must not have any outstanding
allocations by the time isc_mempool_destroy() is called.

Fix by creating a stub query context whenever fetch_callback() is called,
including cancellation events. When the qctx is destroyed, it will ensure
the client is detached and the plugin memory is freed.

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 6df111b9ea7fe24d1c7c14908c0f32811bd2fa63..91b1bd36fad5e0532facc6490fbced2016e3e4f7 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -365,7 +365,7 @@ static void
 query_trace(query_ctx_t *qctx);
 
 static void
-qctx_init(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype,
+qctx_init(ns_client_t *client, dns_fetchevent_t **eventp, dns_rdatatype_t qtype,
          query_ctx_t *qctx);
 
 static isc_result_t
@@ -5065,7 +5065,7 @@ nxrrset:
  * when leaving the scope or freeing the qctx.
  */
 static void
-qctx_init(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype,
+qctx_init(ns_client_t *client, dns_fetchevent_t **eventp, dns_rdatatype_t qtype,
          query_ctx_t *qctx) {
        REQUIRE(qctx != NULL);
        REQUIRE(client != NULL);
@@ -5079,7 +5079,12 @@ qctx_init(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype,
 
        CCTRACE(ISC_LOG_DEBUG(3), "qctx_init");
 
-       qctx->event = event;
+       if (eventp != NULL) {
+               qctx->event = *eventp;
+               *eventp = NULL;
+       } else {
+               qctx->event = NULL;
+       }
        qctx->qtype = qctx->type = qtype;
        qctx->result = ISC_R_SUCCESS;
        qctx->findcoveringnsec = qctx->view->synthfromdnssec;
@@ -5642,6 +5647,7 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
        isc_result_t result;
        isc_logcategory_t *logcategory = NS_LOGCATEGORY_QUERY_ERRORS;
        int errorloglevel;
+       query_ctx_t qctx;
 
        UNUSED(task);
 
@@ -5709,26 +5715,42 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
        client->state = NS_CLIENTSTATE_WORKING;
 
        /*
-        * If this client is shutting down, or this transaction
-        * has timed out, do not resume the find.
+        * Initialize a new qctx and use it to either resume from
+        * recursion or clean up after cancelation.  Transfer
+        * ownership of devent to the new qctx in the process.
         */
+       qctx_init(client, &devent, 0, &qctx);
+
        client_shuttingdown = ns_client_shuttingdown(client);
        if (fetch_canceled || client_shuttingdown) {
-               free_devent(client, &event, &devent);
+               /*
+                * We've timed out or are shutting down. We can now
+                * free the event and other resources held by qctx, but
+                * don't call qctx_destroy() yet: it might destroy the
+                * client, which we still need for a moment.
+                */
+               qctx_freedata(&qctx);
+
+               /*
+                * Return an error to the client, or just drop.
+                */
                if (fetch_canceled) {
                        CTRACE(ISC_LOG_ERROR, "fetch cancelled");
                        query_error(client, DNS_R_SERVFAIL, __LINE__);
                } else {
                        query_next(client, ISC_R_CANCELED);
                }
-       } else {
-               query_ctx_t qctx;
 
                /*
-                * Initialize a new qctx and use it to resume
-                * from recursion.
+                * Free any persistent plugin data that was allocated to
+                * service the client, then detach the client object.
+                */
+               qctx.detach_client = true;
+               qctx_destroy(&qctx);
+       } else {
+               /*
+                * Resume the find process.
                 */
-               qctx_init(client, devent, 0, &qctx);
                query_trace(&qctx);
 
                result = query_resume(&qctx);

diff --git a/lib/ns/tests/nstest.c b/lib/ns/tests/nstest.c
index d276d47a28a5b42b63c9d6bad5f6fb541c574804..473c7a55b70f061e815f865606b20f58a19be3d2 100644 (file)
--- a/lib/ns/tests/nstest.c
+++ b/lib/ns/tests/nstest.c
@@ -818,7 +818,7 @@ ns_test_qctx_create(const ns_test_qctx_create_params_t *params,
        result = attach_query_msg_to_client(client, params->qname,
                                            params->qtype, params->qflags);
        if (result != ISC_R_SUCCESS) {
-               goto detach_client;
+               goto detach_view;
        }
 
        /*
@@ -849,6 +849,8 @@ ns_test_qctx_create(const ns_test_qctx_create_params_t *params,
 
 destroy_query:
        dns_message_destroy(&client->message);
+detach_view:
+       dns_view_detach(&client->view);
 detach_client:
        isc_nmhandle_detach(&client->handle);