From 4f1f958d6d288641793aa924be8a728cca523280 Mon Sep 17 00:00:00 2001
From: Evan Hunt <each@isc.org>
Date: Tue, 21 Jan 2025 17:57:00 -0800
Subject: [PATCH 1/2] prevent a reference leak from the ns_query_done hooks

if the NS_QUERY_DONE_BEGIN or NS_QUERY_DONE_SEND hook is
used in a plugin and returns NS_HOOK_RETURN, some of the
cleanup in ns_query_done() can be skipped over, leading
to reference leaks that can cause named to hang on shut
down.

this has been addressed by adding more housekeeping
code after the cleanup: tag in ns_query_done().

(cherry picked from commit c2e43582678ec5d0c40e19c671d60012b36ac312)
---
 lib/ns/include/ns/query.h |  1 +
 lib/ns/query.c            | 21 +++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h
index b340c59e3a..c38561fa21 100644
--- a/lib/ns/include/ns/query.h
+++ b/lib/ns/include/ns/query.h
@@ -212,6 +212,7 @@ struct query_ctx {
 
 	ns_client_t *client;	    /* client object */
 	bool	     detach_client; /* client needs detaching */
+	bool	     async;	    /* asynchronous hook running */
 
 	dns_fetchresponse_t *fresp; /* recursion response */
 
diff --git a/lib/ns/query.c b/lib/ns/query.c
index b65d178ddd..e0fb69e4d8 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5480,6 +5480,9 @@ qctx_clean(query_ctx_t *qctx) {
 	if (qctx->db != NULL && qctx->node != NULL) {
 		dns_db_detachnode(qctx->db, &qctx->node);
 	}
+	if (qctx->client != NULL && qctx->client->query.gluedb != NULL) {
+		dns_db_detach(&qctx->client->query.gluedb);
+	}
 }
 
 /*%
@@ -6978,6 +6981,9 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync,
 		goto cleanup_and_detach_from_quota;
 	}
 
+	/* Record that an asynchronous copy of the qctx has been started */
+	qctx->async = true;
+
 	/*
 	 * Typically the runasync() function will trigger recursion, but
 	 * there is no need to set NS_QUERYATTR_RECURSING. The calling hook
@@ -11678,10 +11684,6 @@ ns_query_done(query_ctx_t *qctx) {
 	qctx_clean(qctx);
 	qctx_freedata(qctx);
 
-	if (qctx->client->query.gluedb != NULL) {
-		dns_db_detach(&qctx->client->query.gluedb);
-	}
-
 	/*
 	 * Clear the AA bit if we're not authoritative.
 	 */
@@ -11820,6 +11822,17 @@ ns_query_done(query_ctx_t *qctx) {
 	return qctx->result;
 
 cleanup:
+	/*
+	 * We'd only get here if one of the hooks above
+	 * (NS_QUERY_DONE_BEGIN or NS_QUERY_DONE_SEND) returned
+	 * NS_HOOK_RETURN. Some housekeeping may be needed.
+	 */
+	qctx_clean(qctx);
+	qctx_freedata(qctx);
+	if (!qctx->async) {
+		qctx->detach_client = true;
+		query_error(qctx->client, DNS_R_SERVFAIL, __LINE__);
+	}
 	return result;
 }
 

From cc0fc9824497b7ccc33d50440a59ffcd0f1dd2e2 Mon Sep 17 00:00:00 2001
From: Evan Hunt <each@isc.org>
Date: Tue, 21 Jan 2025 17:57:00 -0800
Subject: [PATCH 2/2] wrap ns_client_error() for unit testing

When testing, the client object doesn't have a proper
netmgr handle, so ns_client_error() needs to be a no-op.

(cherry picked from commit ae37ef45ff45db4919842ad29d3f8dfe0c77c76c)
---
 tests/ns/netmgr_wrap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/ns/netmgr_wrap.c b/tests/ns/netmgr_wrap.c
index 725392fc5a..38dbf076c6 100644
--- a/tests/ns/netmgr_wrap.c
+++ b/tests/ns/netmgr_wrap.c
@@ -97,3 +97,9 @@ isc_nmhandle_detach(isc_nmhandle_t **handlep) {
 
 	return;
 }
+
+void
+ns_client_error(ns_client_t *client ISC_ATTR_UNUSED,
+		isc_result_t result ISC_ATTR_UNUSED) {
+	return;
+}