Make sure we shutdown the controlconf listeners and connections once

It was possible that controlconf connections could be shutdown twice
when shutting down the server, because they would receive the
signal (ISC_R_SHUTTINGDOWN result) from netmgr and then the shutdown
procedure would be called second time via controls_shutdown().

Split the shutdown procedure from control_recvmessage(), so we can call
it independently from netmgr callbacks and make sure it will be called
only once.  Do the similar thing for the listeners.

bin/named/control.c

@@ -177,6 +177,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly,
 		/* Do not flush master files */
 		named_server_flushonshutdown(named_g_server, false);
 		named_os_shutdownmsg(cmdline, *text);
+		isc_loopmgr_shutdown(named_g_loopmgr);
 		result = ISC_R_SHUTTINGDOWN;
 	} else if (command_compare(command, NAMED_COMMAND_STOP)) {
 		/*
@@ -194,6 +195,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly,
 #endif /* ifdef HAVE_LIBSCF */
 		named_server_flushonshutdown(named_g_server, true);
 		named_os_shutdownmsg(cmdline, *text);
+		isc_loopmgr_shutdown(named_g_loopmgr);
 		result = ISC_R_SHUTTINGDOWN;
 	} else if (command_compare(command, NAMED_COMMAND_ADDZONE) ||
 		   command_compare(command, NAMED_COMMAND_MODZONE))

bin/named/controlconf.c

@@ -91,7 +91,7 @@ struct controllistener {
 	isc_sockaddr_t address;
 	isc_nmsocket_t *sock;
 	dns_acl_t *acl;
-	bool exiting;
+	bool shuttingdown;
 	isc_refcount_t references;
 	controlkeylist_t keys;
 	controlconnectionlist_t connections;
@@ -119,6 +119,8 @@ static void
 conn_cleanup(controlconnection_t *conn);
 static void
 conn_free(controlconnection_t *conn);
+static void
+conn_shutdown(controlconnection_t *conn);
 
 #if NAMED_CONTROLCONF_TRACE
 #define controllistener_ref(ptr) \
@@ -147,6 +149,14 @@ ISC_REFCOUNT_DECL(controlconnection);
 
 #define CLOCKSKEW 300
 
+#define CHECK(x)                               \
+	{                                      \
+		result = (x);                  \
+		if (result != ISC_R_SUCCESS) { \
+			goto cleanup;          \
+		}                              \
+	}
+
 static void
 free_controlkey(controlkey_t *key, isc_mem_t *mctx) {
 	if (key->keyname != NULL) {
@@ -169,8 +179,7 @@ free_controlkeylist(controlkeylist_t *keylist, isc_mem_t *mctx) {
 
 static void
 free_listener(controllistener_t *listener) {
-	REQUIRE(isc_tid() == 0);
+	REQUIRE(listener->shuttingdown);
-	REQUIRE(listener->exiting);
 	REQUIRE(ISC_LIST_EMPTY(listener->connections));
 	REQUIRE(listener->sock == NULL);
 
@@ -193,28 +202,25 @@ ISC_REFCOUNT_IMPL(controlconnection, conn_free);
 
 static void
 shutdown_listener(controllistener_t *listener) {
-	REQUIRE(isc_tid() == 0);
+	/* Don't shutdown the same listener twice */
-	if (!listener->exiting) {
+	if (listener->shuttingdown) {
-		char socktext[ISC_SOCKADDR_FORMATSIZE];
+		return;
-
-		for (controlconnection_t *conn =
-			     ISC_LIST_HEAD(listener->connections);
-		     conn != NULL; conn = ISC_LIST_HEAD(listener->connections))
-		{
-			control_recvmessage(conn->ccmsg.handle,
-					    ISC_R_SHUTTINGDOWN, conn);
-		}
-
-		ISC_LIST_UNLINK(listener->controls->listeners, listener, link);
-
-		isc_sockaddr_format(&listener->address, socktext,
-				    sizeof(socktext));
-		isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
-			      NAMED_LOGMODULE_CONTROL, ISC_LOG_NOTICE,
-			      "stopping command channel on %s", socktext);
-
-		listener->exiting = true;
 	}
+	listener->shuttingdown = true;
+
+	for (controlconnection_t *conn = ISC_LIST_HEAD(listener->connections);
+	     conn != NULL; conn = ISC_LIST_HEAD(listener->connections))
+	{
+		conn_shutdown(conn);
+	}
+
+	ISC_LIST_UNLINK(listener->controls->listeners, listener, link);
+
+	char socktext[ISC_SOCKADDR_FORMATSIZE];
+	isc_sockaddr_format(&listener->address, socktext, sizeof(socktext));
+	isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
+		      NAMED_LOGMODULE_CONTROL, ISC_LOG_NOTICE,
+		      "stopping command channel on %s", socktext);
 
 	isc_nm_stoplistening(listener->sock);
 	isc_nmsocket_close(&listener->sock);
@@ -239,35 +245,35 @@ address_ok(isc_sockaddr_t *sockaddr, controllistener_t *listener) {
 static void
 control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	controlconnection_t *conn = (controlconnection_t *)arg;
-	controllistener_t *listener = conn->listener;
-	isc_sockaddr_t peeraddr = isc_nmhandle_peeraddr(handle);
 
-	if (conn->result == ISC_R_SHUTTINGDOWN) {
+	if (conn->shuttingdown) {
-		isc_loopmgr_shutdown(named_g_loopmgr);
+		/* The connection is shuttingdown */
-		goto cleanup_sendhandle;
+		result = ISC_R_SHUTTINGDOWN;
 	}
 
-	if (listener->controls->shuttingdown || result == ISC_R_SHUTTINGDOWN) {
+	if (result == ISC_R_SUCCESS) {
-		goto cleanup_sendhandle;
+		/* Everything is peachy, continue reading from the socket */
-	} else if (result != ISC_R_SUCCESS) {
+		isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage,
+					conn);
+		goto done;
+	}
+
+	/* This is the error path */
+	if (result != ISC_R_SHUTTINGDOWN) {
 		char socktext[ISC_SOCKADDR_FORMATSIZE];
+		isc_sockaddr_t peeraddr = isc_nmhandle_peeraddr(handle);
 
 		isc_sockaddr_format(&peeraddr, socktext, sizeof(socktext));
 		isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
 			      NAMED_LOGMODULE_CONTROL, ISC_LOG_WARNING,
 			      "error sending command response to %s: %s",
 			      socktext, isc_result_totext(result));
-		goto cleanup_sendhandle;
 	}
 
-	isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn);
+	conn_shutdown(conn);
 
-cleanup_sendhandle:
+done:
-	if (result != ISC_R_SUCCESS) {
+	/* Detach the sending reference */
-		control_recvmessage(handle, result, conn);
-	}
-
-	REQUIRE(isc_tid() == 0);
 	controlconnection_detach(&conn);
 }
 
@@ -374,7 +380,7 @@ control_respond(controlconnection_t *conn) {
 	r.base = conn->buffer->base;
 	r.length = conn->buffer->used;
 
-	REQUIRE(isc_tid() == 0);
+	/* Attach the sending reference */
 	controlconnection_ref(conn);
 	isccc_ccmsg_sendmessage(&conn->ccmsg, &r, control_senddone, conn);
 
@@ -385,15 +391,33 @@ cleanup:
 static void
 control_command(void *arg) {
 	controlconnection_t *conn = (controlconnection_t *)arg;
-	controllistener_t *listener = conn->listener;
 
-	if (!listener->controls->shuttingdown) {
+	/* Don't run the command if we already started the shutdown */
+	if (!conn->shuttingdown) {
 		conn->result = named_control_docommand(
-			conn->request, listener->readonly, &conn->text);
+			conn->request, conn->listener->readonly, &conn->text);
 		control_respond(conn);
 	}
 
-	REQUIRE(isc_tid() == 0);
+	/* Detach the control command reference */
+	controlconnection_detach(&conn);
+}
+
+static void
+conn_shutdown(controlconnection_t *conn) {
+	/* Don't shutdown the same controlconnection twice */
+	if (conn->shuttingdown) {
+		return;
+	}
+	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.
+	 */
+	isccc_ccmsg_invalidate(&conn->ccmsg);
+
+	/* Detach the reading reference */
 	controlconnection_detach(&conn);
 }
 
@@ -407,19 +431,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 	isccc_time_t exp;
 	uint32_t nonce;
 
-	INSIST(!conn->shuttingdown);
-
-	/* Is the server shutting down? */
-	if (listener->controls->shuttingdown) {
-		result = ISC_R_SHUTTINGDOWN;
-	}
-
 	if (result != ISC_R_SUCCESS) {
-		if (result == ISC_R_SHUTTINGDOWN) {
-			listener->controls->shuttingdown = true;
-		} else if (result != ISC_R_EOF) {
-			log_invalid(&conn->ccmsg, result);
-		}
 		goto cleanup;
 	}
 
@@ -445,13 +457,13 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 	}
 
 	if (key == NULL) {
-		log_invalid(&conn->ccmsg, ISCCC_R_BADAUTH);
+		result = ISCCC_R_BADAUTH;
 		goto cleanup;
 	}
 
 	/* We shouldn't be getting a reply. */
 	if (isccc_cc_isreply(conn->request)) {
-		log_invalid(&conn->ccmsg, ISC_R_FAILURE);
+		result = ISC_R_FAILURE;
 		goto cleanup;
 	}
 
@@ -462,7 +474,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 	 */
 	conn->ctrl = isccc_alist_lookup(conn->request, "_ctrl");
 	if (!isccc_alist_alistp(conn->ctrl)) {
-		log_invalid(&conn->ccmsg, ISC_R_FAILURE);
+		result = ISC_R_FAILURE;
 		goto cleanup;
 	}
 
@@ -470,11 +482,11 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 		if ((sent + CLOCKSKEW) < conn->now ||
 		    (sent - CLOCKSKEW) > conn->now)
 		{
-			log_invalid(&conn->ccmsg, ISCCC_R_CLOCKSKEW);
+			result = ISCCC_R_CLOCKSKEW;
 			goto cleanup;
 		}
 	} else {
-		log_invalid(&conn->ccmsg, ISC_R_FAILURE);
+		result = ISC_R_FAILURE;
 		goto cleanup;
 	}
 
@@ -484,7 +496,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 	if (isccc_cc_lookupuint32(conn->ctrl, "_exp", &exp) == ISC_R_SUCCESS &&
 	    conn->now > exp)
 	{
-		log_invalid(&conn->ccmsg, ISCCC_R_EXPIRED);
+		result = ISCCC_R_EXPIRED;
 		goto cleanup;
 	}
 
@@ -500,7 +512,6 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 		if (result == ISC_R_EXISTS) {
 			result = ISCCC_R_DUPLICATE;
 		}
-		log_invalid(&conn->ccmsg, result);
 		goto cleanup;
 	}
 
@@ -509,7 +520,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 		     ISC_R_SUCCESS ||
 	     conn->nonce != nonce))
 	{
-		log_invalid(&conn->ccmsg, ISCCC_R_BADAUTH);
+		result = ISCCC_R_BADAUTH;
 		goto cleanup;
 	}
 
@@ -527,28 +538,33 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result,
 		return;
 	}
 
-	/*
+	/* Attach the command reference */
-	 * Trigger the command.
-	 */
-	REQUIRE(isc_tid() == 0);
 	controlconnection_ref(conn);
+
+	/* Trigger the command asynchronously. */
 	isc_async_run(named_g_mainloop, control_command, conn);
 
 	return;
 
 cleanup:
-	/* Make sure no read callbacks are called again */
+	switch (result) {
-	isccc_ccmsg_invalidate(&conn->ccmsg);
+	case ISC_R_SHUTTINGDOWN:
+	case ISC_R_EOF:
+		break;
+	default:
+		/* We can't get here on normal path */
+		INSIST(result != ISC_R_SUCCESS);
 
-	conn->shuttingdown = true;
+		log_invalid(&conn->ccmsg, result);
+	}
 
-	REQUIRE(isc_tid() == 0);
+	conn_shutdown(conn);
-	controlconnection_detach(&conn);
 }
 
 static void
 conn_free(controlconnection_t *conn) {
-	REQUIRE(isc_tid() == 0);
+	/* Make sure that the connection was shutdown first */
+	REQUIRE(conn->shuttingdown);
 
 	controllistener_t *listener = conn->listener;
 
@@ -576,17 +592,26 @@ conn_free(controlconnection_t *conn) {
 
 static void
 newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
-	REQUIRE(isc_tid() == 0);
+	/* Don't create new connection if we are shutting down */
+	if (listener->shuttingdown) {
+		isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
+			      NAMED_LOGMODULE_CONTROL, ISC_LOG_DEBUG(3),
+			      "rejected new control connection: %s",
+			      isc_result_totext(ISC_R_SHUTTINGDOWN));
+		return;
+	}
 
 	controlconnection_t *conn = isc_mem_get(listener->mctx, sizeof(*conn));
 	isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
 		      NAMED_LOGMODULE_CONTROL, ISC_LOG_DEBUG(3),
 		      "allocate new control connection");
 
-	*conn = (controlconnection_t){ .alg = DST_ALG_UNKNOWN };
+	*conn = (controlconnection_t){
-
+		.alg = DST_ALG_UNKNOWN,
-	isc_refcount_init(&conn->references, 1);
+		.references = ISC_REFCOUNT_INITIALIZER(1),
-	controllistener_attach(listener, &conn->listener);
+		.listener = controllistener_ref(listener),
+		.link = ISC_LINK_INITIALIZER,
+	};
 
 	/* isccc_ccmsg_init() attaches to the handle */
 	isccc_ccmsg_init(listener->mctx, handle, &conn->ccmsg);
@@ -594,8 +619,9 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
 	/* Set a 32 KiB upper limit on incoming message. */
 	isccc_ccmsg_setmaxsize(&conn->ccmsg, 32768);
 
-	ISC_LIST_INITANDAPPEND(listener->connections, conn, link);
+	ISC_LIST_APPEND(listener->connections, conn, link);
 
+	/* The reading reference has been initialized in the initializer */
 	isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn);
 }
 
@@ -635,8 +661,7 @@ controls_shutdown(named_controls_t *controls) {
 	     listener = next)
 	{
 		/*
-		 * This is asynchronous.  As listeners shut down, they will
+		 * As listeners shut down, they will call their callbacks.
-		 * call their callbacks.
 		 */
 		next = ISC_LIST_NEXT(listener, link);
 		shutdown_listener(listener);
@@ -645,8 +670,20 @@ controls_shutdown(named_controls_t *controls) {
 
 void
 named_controls_shutdown(named_controls_t *controls) {
-	controls_shutdown(controls);
+	/*
+	 * Don't ever shutdown the controls twice.
+	 *
+	 * NOTE: This functions is called when the server is shutting down, but
+	 * controls_shutdown() can and will be called multiple times - on each
+	 * reconfiguration, the listeners will be torn down and recreated again,
+	 * see named_controls_configure() for details.
+	 */
+	if (controls->shuttingdown) {
+		return;
+	}
 	controls->shuttingdown = true;
+
+	controls_shutdown(controls);
 }
 
 static isc_result_t
@@ -775,14 +812,6 @@ register_keys(const cfg_obj_t *control, const cfg_obj_t *keylist,
 	}
 }
 
-#define CHECK(x)                               \
-	do {                                   \
-		result = (x);                  \
-		if (result != ISC_R_SUCCESS) { \
-			goto cleanup;          \
-		}                              \
-	} while (0)
-
 static isc_result_t
 get_rndckey(isc_mem_t *mctx, controlkeylist_t *keyids) {
 	isc_result_t result;
@@ -1042,6 +1071,12 @@ add_listener(named_controls_t *cp, controllistener_t **listenerp,
 	isc_result_t result = ISC_R_SUCCESS;
 	int pf;
 
+	/* Don't create new listener if we are shutting down */
+	if (cp->shuttingdown) {
+		result = ISC_R_SHUTTINGDOWN;
+		goto shuttingdown;
+	}
+
 	listener = isc_mem_get(mctx, sizeof(*listener));
 	*listener = (controllistener_t){ .controls = cp,
 					 .address = *addr,
@@ -1112,9 +1147,10 @@ add_listener(named_controls_t *cp, controllistener_t **listenerp,
 
 cleanup:
 	isc_refcount_decrement(&listener->references);
-	listener->exiting = true;
+	listener->shuttingdown = true;
 	free_listener(listener);
 
+shuttingdown:
 	if (control != NULL) {
 		cfg_obj_log(control, named_g_lctx, ISC_LOG_WARNING,
 			    "couldn't add command channel %s: %s", socktext,
@@ -1332,6 +1368,7 @@ named_controls_destroy(named_controls_t **ctrlsp) {
 	named_controls_t *controls = *ctrlsp;
 	*ctrlsp = NULL;
 
+	REQUIRE(controls->shuttingdown);
 	REQUIRE(ISC_LIST_EMPTY(controls->listeners));
 
 	LOCK(&controls->symtab_lock);