From 86eddebc836e82a993db2e5a1b13dccaa5b51614 Mon Sep 17 00:00:00 2001
From: Evan Hunt <each@isc.org>
Date: Wed, 22 May 2019 10:58:41 +0200
Subject: [PATCH] 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.
---
 lib/ns/query.c        | 44 ++++++++++++++++++++++++++++++++-----------
 lib/ns/tests/nstest.c |  4 +++-
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 6df111b9ea..91b1bd36fa 100644
--- 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 d276d47a28..473c7a55b7 100644
--- 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);