From 90989bfdfb702641b852be11c6c49b1b021cb775 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 c090754526..2392baf13f 100644
--- a/lib/ns/include/ns/query.h
+++ b/lib/ns/include/ns/query.h
@@ -159,6 +159,7 @@ struct query_ctx {
 
 	ns_client_t *client;	    /* client object */
 	bool	     detach_client; /* client needs detaching */
+	bool	     async;	    /* asynchronous hook running */
 
 	dns_fetchevent_t *event; /* recursion event */
 
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 8bdb33b088..5a75601160 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5308,6 +5308,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);
+	}
 }
 
 /*%
@@ -7037,6 +7040,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
@@ -11972,10 +11978,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.
 	 */
@@ -12112,6 +12114,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 6b22c9a989893874daa9ced49e6483883a15ade5 Mon Sep 17 00:00:00 2001
From: Evan Hunt <each@isc.org>
Date: Tue, 25 Feb 2025 16:23:14 -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/Makefile.am   |  4 ++++
 tests/ns/netmgr_wrap.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 tests/ns/netmgr_wrap.c

diff --git a/tests/ns/Makefile.am b/tests/ns/Makefile.am
index 33f91d8c7b..81e36629e4 100644
--- a/tests/ns/Makefile.am
+++ b/tests/ns/Makefile.am
@@ -20,6 +20,10 @@ check_PROGRAMS =		\
 	plugin_test		\
 	query_test
 
+query_test_SOURCES =		\
+	query_test.c		\
+	netmgr_wrap.c
+
 EXTRA_DIST = testdata
 
 include $(top_srcdir)/Makefile.tests
diff --git a/tests/ns/netmgr_wrap.c b/tests/ns/netmgr_wrap.c
new file mode 100644
index 0000000000..6737e53013
--- /dev/null
+++ b/tests/ns/netmgr_wrap.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+/*! \file */
+
+#include <isc/atomic.h>
+#include <isc/netmgr.h>
+#include <isc/util.h>
+
+#include <dns/view.h>
+
+#include <ns/client.h>
+
+void
+ns_client_error(ns_client_t *client ISC_ATTR_UNUSED,
+		isc_result_t result ISC_ATTR_UNUSED) {
+	return;
+}