From d5103b742b23c3961fb48daae180f530417a08e4 Mon Sep 17 00:00:00 2001
From: Mark Andrews <marka@isc.org>
Date: Wed, 10 Jan 2024 14:35:36 +1100
Subject: [PATCH] Defer control channel message invalidation

The conn_shutdown() function is called whenever a control channel
connection is supposed to be closed, e.g. after a response to the client
is sent or when named is being shut down.  That function calls
isccc_ccmsg_invalidate(), which resets the magic number in the structure
holding the messages exchanged over a given control channel connection
(isccc_ccmsg_t).  The expectation here is that all operations related to
the given control channel connection will have been completed by the
time the connection needs to be shut down.

However, if named shutdown is initiated while a control channel message
is still in flight, some netmgr callbacks might still be pending when
conn_shutdown() is called and isccc_ccmsg_t invalidated.  This causes
the REQUIRE assertion checking the magic number in ccmsg_senddone() to
fail when the latter function is eventually called, resulting in a
crash.

Fix by splitting up isccc_ccmsg_invalidate() into two separate
functions:

  - isccc_ccmsg_disconnect(), which initiates TCP connection shutdown,
  - isccc_ccmsg_invalidate(), which cleans up magic number and buffer,

and then:

  - replacing all existing uses of isccc_ccmsg_invalidate() with calls
    to isccc_ccmsg_disconnect(),

  - only calling isccc_ccmsg_invalidate() when all netmgr callbacks are
    guaranteed to have been run.

Adjust function comments accordingly.
---
 bin/named/controlconf.c         |  8 +++++---
 bin/rndc/rndc.c                 |  4 +++-
 lib/isccc/ccmsg.c               | 14 ++++++++++----
 lib/isccc/include/isccc/ccmsg.h | 19 +++++++++++++------
 4 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c
index ae69d22ee0..46b7ab8928 100644
--- a/bin/named/controlconf.c
+++ b/bin/named/controlconf.c
@@ -419,10 +419,10 @@ conn_shutdown(controlconnection_t *conn) {
 	conn->shuttingdown = true;
 
 	/*
-	 * Calling invalidate on ccmsg will shutdown the TCP connection, thus
-	 * we are making sure that no read callback will be called ever again.
+	 * Close the TCP connection to make sure that no read callback will be
+	 * called for it ever again.
 	 */
-	isccc_ccmsg_invalidate(&conn->ccmsg);
+	isccc_ccmsg_disconnect(&conn->ccmsg);
 
 	/* Detach the reading reference */
 	controlconnection_detach(&conn);
@@ -575,6 +575,8 @@ conn_free(controlconnection_t *conn) {
 
 	controllistener_t *listener = conn->listener;
 
+	isccc_ccmsg_invalidate(&conn->ccmsg);
+
 	conn_cleanup(conn);
 
 	if (conn->buffer != NULL) {
diff --git a/bin/rndc/rndc.c b/bin/rndc/rndc.c
index e129fe8434..645cef5ed0 100644
--- a/bin/rndc/rndc.c
+++ b/bin/rndc/rndc.c
@@ -348,7 +348,7 @@ rndc_recvdone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 
 	isccc_sexpr_free(&response);
 
-	isccc_ccmsg_invalidate(ccmsg);
+	isccc_ccmsg_disconnect(ccmsg);
 	isc_loopmgr_shutdown(loopmgr);
 }
 
@@ -1003,6 +1003,8 @@ main(int argc, char **argv) {
 
 	isc_loopmgr_run(loopmgr);
 
+	isccc_ccmsg_invalidate(&rndc_ccmsg);
+
 	isc_log_destroy(&log);
 	isc_log_setcontext(NULL);
 
diff --git a/lib/isccc/ccmsg.c b/lib/isccc/ccmsg.c
index f52b039152..80d622da79 100644
--- a/lib/isccc/ccmsg.c
+++ b/lib/isccc/ccmsg.c
@@ -179,6 +179,16 @@ isccc_ccmsg_sendmessage(isccc_ccmsg_t *ccmsg, isc_region_t *region,
 	isc_nm_send(ccmsg->handle, region, ccmsg_senddone, ccmsg);
 }
 
+void
+isccc_ccmsg_disconnect(isccc_ccmsg_t *ccmsg) {
+	REQUIRE(VALID_CCMSG(ccmsg));
+
+	if (ccmsg->handle != NULL) {
+		isc_nmhandle_close(ccmsg->handle);
+		isc_nmhandle_detach(&ccmsg->handle);
+	}
+}
+
 void
 isccc_ccmsg_invalidate(isccc_ccmsg_t *ccmsg) {
 	REQUIRE(VALID_CCMSG(ccmsg));
@@ -188,10 +198,6 @@ isccc_ccmsg_invalidate(isccc_ccmsg_t *ccmsg) {
 	if (ccmsg->buffer != NULL) {
 		isc_buffer_free(&ccmsg->buffer);
 	}
-	if (ccmsg->handle != NULL) {
-		isc_nmhandle_close(ccmsg->handle);
-		isc_nmhandle_detach(&ccmsg->handle);
-	}
 }
 
 void
diff --git a/lib/isccc/include/isccc/ccmsg.h b/lib/isccc/include/isccc/ccmsg.h
index 8404252a56..7a20a7c2e1 100644
--- a/lib/isccc/include/isccc/ccmsg.h
+++ b/lib/isccc/include/isccc/ccmsg.h
@@ -120,18 +120,25 @@ isccc_ccmsg_sendmessage(isccc_ccmsg_t *ccmsg, isc_region_t *region,
  */
 
 void
-isccc_ccmsg_invalidate(isccc_ccmsg_t *ccmsg);
+isccc_ccmsg_disconnect(isccc_ccmsg_t *ccmsg);
 /*%
- * Clean up all allocated state, and invalidate the structure.
+ * Disconnect from the connected netmgr handle associated with a command
+ * channel message.
  *
  * Requires:
  *
- *\li	"ccmsg" be valid.
+ *\li	"ccmsg" to be valid.
+ */
+
+void
+isccc_ccmsg_invalidate(isccc_ccmsg_t *ccmsg);
+/*%
+ * Clean up the magic number and the dynamic buffer associated with a command
+ * channel message.
  *
- * Ensures:
+ * Requires:
  *
- *\li	"ccmsg" is invalidated and disassociated with all memory contexts,
- *	sockets, etc.
+ *\li	"ccmsg" to be valid.
  */
 
 void