From 1f3597742335cf43a0bc08a75691a96faae02e4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= <ondrej@isc.org>
Date: Wed, 23 Mar 2022 16:16:53 +0100
Subject: [PATCH] Remove ns_client_t .shuttingdown member

The way the ns_client_t .shuttingdown member was practically dead code.
The .shuttingdown would be set to true only in ns__client_put() function
meaning that we have detached from all ns_client_t .*handles and the
ns_client_t object being freed:

    client->magic = 0;
    client->shuttingdown = true;
    [...]
    isc_mem_put(manager->ctx, client, sizeof(*client))

Meanwhile the ns_client_t object is accessed like this:

    isc_nmhandle_detach(&client->fetchhandle);

    client->query.attributes &= ~NS_QUERYATTR_RECURSING;
    client->state = NS_CLIENTSTATE_WORKING;

    qctx_init(client, &devent, 0, &qctx);

    client_shuttingdown = ns_client_shuttingdown(client);
    if (fetch_canceled || fetch_answered || client_shuttingdown) {
        [...]
    }

Even if the isc_nmhandle_detach(...) was the last handle detach, it
would mean that immediatelly, after calling the isc_nmhandle_detach(),
we would be causing use-after-free, because the ns_client_t is
immediatelly destroyed after setting .shuttingdown to true.

The similar code in the query_hookresume() already noticed this:

    /*
     * This event is running under a client task, so it's safe to detach
     * the fetch handle.  And it should be done before resuming query
     * processing below, since that may trigger another recursion or
     * asynchronous hook event.
     */
---
 lib/ns/client.c            | 6 ------
 lib/ns/include/ns/client.h | 7 -------
 lib/ns/query.c             | 4 +---
 3 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/lib/ns/client.c b/lib/ns/client.c
index 2f5c1dd2a1..7fa7386bba 100644
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -1672,7 +1672,6 @@ ns__client_put_cb(void *client0) {
 	client_extendederror_reset(client);
 
 	client->magic = 0;
-	client->shuttingdown = true;
 
 	isc_mem_put(manager->mctx, client->sendbuf, NS_CLIENT_SEND_BUFFER_SIZE);
 	if (client->opt != NULL) {
@@ -2364,11 +2363,6 @@ cleanup:
 	return (result);
 }
 
-bool
-ns_client_shuttingdown(ns_client_t *client) {
-	return (client->shuttingdown);
-}
-
 /***
  *** Client Manager
  ***/
diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h
index efd98a9912..e26f14ff9d 100644
--- a/lib/ns/include/ns/client.h
+++ b/lib/ns/include/ns/client.h
@@ -172,7 +172,6 @@ struct ns_client {
 	ns_clientstate_t state;
 	int		 nupdates;
 	bool		 nodetach;
-	bool		 shuttingdown;
 	unsigned int	 attributes;
 	dns_view_t	   *view;
 	dns_dispatch_t  *dispatch;
@@ -316,12 +315,6 @@ ns_client_drop(ns_client_t *client, isc_result_t result);
  * will be sent.
  */
 
-bool
-ns_client_shuttingdown(ns_client_t *client);
-/*%<
- * Return true iff the client is currently shutting down.
- */
-
 isc_result_t
 ns_client_replace(ns_client_t *client);
 /*%<
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 3ec00258ad..cb1a579307 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -6121,7 +6121,6 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 	ns_client_t *client = NULL;
 	bool fetch_canceled = false;
 	bool fetch_answered = false;
-	bool client_shuttingdown = false;
 	isc_logcategory_t *logcategory = NS_LOGCATEGORY_QUERY_ERRORS;
 	isc_result_t result;
 	int errorloglevel;
@@ -6218,8 +6217,7 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 	 */
 	qctx_init(client, &devent, 0, &qctx);
 
-	client_shuttingdown = ns_client_shuttingdown(client);
-	if (fetch_canceled || fetch_answered || client_shuttingdown) {
+	if (fetch_canceled || fetch_answered) {
 		/*
 		 * We've timed out or are shutting down. We can now
 		 * free the event and other resources held by qctx, but