From: Colin Vidal <colin@isc.org>
Date: Mon, 8 Sep 2025 08:10:37 +0000 (+0200)
Subject: replace QCTX_INIT/_DESTROY hooks with QUERY_SETUP/_RESET
X-Git-Tag: v9.21.14~56^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=95c71c27396721c90452902cc9adabc18131ea83;p=thirdparty%2Fbind9.git

replace QCTX_INIT/_DESTROY hooks with QUERY_SETUP/_RESET

The hook NS_QUERY_QCTX_DESTROY is problematic with zone plugins because
it can be called in some contexts where `qctx->client` is invalid (the
pointer is dangling); which would lead to a use-after-free (spotted by
TSAN build) as `qctx->client` is used to get the zone hooktable, to find
out whether there is an authoritive zone which would have
NS_QUERY_QCTX_DESTROY registered.

This can't easily be fixed, because there is no easy way to know from
query.c code if `client` is still a valid object: `client->reqhandle`,
representing the request from a client, is refcounted, and the `client`
object is freed from memory once its refcounter gets to 0. While
`reqhandle` is attached from query.c code, it can be attached more than
once from asynchronous code and there is no clear path where detaching
it would lead to a client free. Hence, there is no way to know for sure
when to set `qctx->client = NULL` (this is why the pointer remains
dangling).

Back to the original problem; this is why the NS_QUERY_QCTX_DESTROY hook
is incompatible with zone plugins. `qctx->detach_client`, which is used
to tell a plugin that the `client` object is either free or about to be
free can't be use either, because in some cases the client is still
there, and should be used.

Code issue aside, the `qctx` object is really just an aggregate of
various data to pass easily in the various functions and callbacks,
initially stored on the stack, but allocated in some cases (for some
asynchronous flow, when recursion is needed), so the point it gets
created/"destroyed" is really just an implementation "detail", and
providing a higher level hook for the plugin would be beneficial. Hence,
NS_QUERY_RESET and NS_QUERY_INIT are removed, and instead, the existing
NS_QUERY_SETUP can be used as well as the newly introduced
NS_QUERY_RESET (which replaces NS_QUERY_QCTX_DESTROY). The advanage is
that NS_QUERY_RESET is called _only_ when the client object is _always_
about to be freed, which avoids usage of the extra `qctx->detach_client`
usage from the plugin.

The way NS_QUERY_RESET works is that when the `client` is freed, a
callback (from `query.c`) is called. This callback creates a transient
qctx object on the stack with a pointer to the view, and uses that
to call the hook.
---

diff --git a/lib/ns/include/ns/hooks.h b/lib/ns/include/ns/hooks.h
index 363e1ac3a52..3b1b4b60573 100644
--- a/lib/ns/include/ns/hooks.h
+++ b/lib/ns/include/ns/hooks.h
@@ -384,9 +384,8 @@
  */
 typedef enum {
 	/* hookpoints from query.c */
-	NS_QUERY_QCTX_INITIALIZED,
-	NS_QUERY_QCTX_DESTROYED,
 	NS_QUERY_SETUP,
+	NS_QUERY_RESET,
 	NS_QUERY_START_BEGIN,
 	NS_QUERY_LOOKUP_BEGIN,
 	NS_QUERY_RESUME_BEGIN,
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 123a0a0a327..77419b77c7e 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -822,6 +822,10 @@ ns_query_cancel(ns_client_t *client) {
 
 static void
 query_reset(ns_client_t *client, bool everything) {
+	query_ctx_t qctx = { .view = client->inner.view, .client = client };
+
+	CALL_HOOK_NORETURN(NS_QUERY_RESET, &qctx);
+
 	CTRACE(ISC_LOG_DEBUG(3), "query_reset");
 
 	/*%
@@ -5132,8 +5136,6 @@ qctx_init(ns_client_t *client, dns_fetchresponse_t **frespp,
 	if (dns_rdatatype_issig(qctx->qtype)) {
 		qctx->type = dns_rdatatype_any;
 	}
-
-	CALL_HOOK_NORETURN(NS_QUERY_QCTX_INITIALIZED, qctx);
 }
 
 /*%