From: Evan Hunt <each@isc.org>
Date: Thu, 14 May 2020 21:03:37 +0000 (-0700)
Subject: use an isc_task to execute rndc commands
X-Git-Tag: v9.17.4~52^2~4
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=45ab0603eb9e76f78d173b687fdf3cb8ab598913;p=thirdparty%2Fbind9.git

use an isc_task to execute rndc commands

- using an isc_task to execute all rndc functions makes it relatively
  simple for them to acquire task exclusive mode when needed
- control_recvmessage() has been separated into two functions,
  control_recvmessage() and control_respond(). the respond function
  can be called immediately from control_recvmessage() when processing
  a nonce, or it can be called after returning from the task event
  that ran the rndc command function.
---

diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c
index 37a19b57b6c..924b8a74f66 100644
--- a/bin/named/controlconf.c
+++ b/bin/named/controlconf.c
@@ -27,6 +27,7 @@
 #include <isc/result.h>
 #include <isc/stdtime.h>
 #include <isc/string.h>
+#include <isc/task.h>
 #include <isc/util.h>
 
 #include <dns/result.h>
@@ -70,9 +71,17 @@ struct controlconnection {
 	isccc_ccmsg_t ccmsg;
 	bool ccmsg_valid;
 	bool sending;
-	isc_buffer_t *buffer;
 	controllistener_t *listener;
+	isccc_sexpr_t *ctrl;
+	isc_buffer_t *buffer;
+	isc_buffer_t *text;
+	isccc_sexpr_t *request;
+	isccc_sexpr_t *response;
+	uint32_t alg;
+	isccc_region_t secret;
 	uint32_t nonce;
+	isc_stdtime_t now;
+	isc_result_t result;
 	ISC_LINK(controlconnection_t) link;
 };
 
@@ -206,16 +215,18 @@ 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);
 
 	REQUIRE(conn->sending);
 
 	conn->sending = false;
 
+	isc_nmhandle_unref(handle);
+
 	if (result == ISC_R_CANCELED) {
 		return;
 	} else if (result != ISC_R_SUCCESS) {
 		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,
@@ -247,33 +258,127 @@ log_invalid(isccc_ccmsg_t *ccmsg, isc_result_t result) {
 }
 
 static void
-control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
+control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	controlconnection_t *conn = (controlconnection_t *)arg;
-	controllistener_t *listener = NULL;
-	controlkey_t *key = NULL;
-	isccc_sexpr_t *request = NULL;
-	isccc_sexpr_t *response = NULL;
-	uint32_t algorithm;
-	isccc_region_t secret;
-	isc_stdtime_t now;
+	controllistener_t *listener = conn->listener;
+	isccc_sexpr_t *data = NULL;
 	isc_buffer_t b;
 	isc_region_t r;
-	isc_buffer_t *text = NULL;
-	isc_result_t eresult;
-	isccc_sexpr_t *_ctrl = NULL;
+
+	result = isccc_cc_createresponse(conn->request, conn->now,
+					 conn->now + 60, &conn->response);
+	if (result != ISC_R_SUCCESS) {
+		goto cleanup;
+	}
+
+	data = isccc_alist_lookup(conn->response, "_data");
+	if (data != NULL) {
+		if (isccc_cc_defineuint32(data, "result", conn->result) == NULL)
+		{
+			goto cleanup;
+		}
+	}
+
+	if (conn->result != ISC_R_SUCCESS) {
+		if (data != NULL) {
+			const char *estr = isc_result_totext(conn->result);
+			if (isccc_cc_definestring(data, "err", estr) == NULL) {
+				goto cleanup;
+			}
+		}
+	}
+
+	if (isc_buffer_usedlength(conn->text) > 0) {
+		if (data != NULL) {
+			char *str = (char *)isc_buffer_base(conn->text);
+			if (isccc_cc_definestring(data, "text", str) == NULL) {
+				goto cleanup;
+			}
+		}
+	}
+
+	conn->ctrl = isccc_alist_lookup(conn->response, "_ctrl");
+	if (conn->ctrl == NULL ||
+	    isccc_cc_defineuint32(conn->ctrl, "_nonce", conn->nonce) == NULL)
+	{
+		goto cleanup;
+	}
+
+	if (conn->buffer == NULL) {
+		isc_buffer_allocate(listener->mctx, &conn->buffer, 2 * 2048);
+	}
+
+	isc_buffer_clear(conn->buffer);
+	/* Skip the length field (4 bytes) */
+	isc_buffer_add(conn->buffer, 4);
+
+	result = isccc_cc_towire(conn->response, &conn->buffer, conn->alg,
+				 &conn->secret);
+	if (result != ISC_R_SUCCESS) {
+		goto cleanup;
+	}
+
+	isc_buffer_init(&b, conn->buffer->base, 4);
+	isc_buffer_putuint32(&b, conn->buffer->used - 4);
+
+	r.base = conn->buffer->base;
+	r.length = conn->buffer->used;
+
+	isc_nmhandle_ref(handle);
+	conn->sending = true;
+	result = isc_nm_send(handle, &r, control_senddone, conn);
+	if (result != ISC_R_SUCCESS) {
+		isc_nmhandle_unref(handle);
+		conn->sending = false;
+		goto cleanup;
+	}
+
+cleanup:
+	if (conn->response != NULL) {
+		isccc_sexpr_free(&conn->response);
+	}
+	if (conn->request != NULL) {
+		isccc_sexpr_free(&conn->request);
+	}
+
+	if (conn->secret.rstart != NULL) {
+		isc_mem_put(listener->mctx, conn->secret.rstart,
+			    REGION_SIZE(conn->secret));
+	}
+	if (conn->text != NULL) {
+		isc_buffer_free(&conn->text);
+	}
+}
+
+static void
+control_command(isc_task_t *task, isc_event_t *event) {
+	controlconnection_t *conn = event->ev_arg;
+	controllistener_t *listener = conn->listener;
+
+	UNUSED(task);
+
+	conn->result = named_control_docommand(conn->request,
+					       listener->readonly, &conn->text);
+	control_respond(conn->handle, conn->result, conn);
+	isc_nmhandle_unref(conn->handle);
+	isc_event_free(&event);
+}
+
+static void
+control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
+	controlconnection_t *conn = (controlconnection_t *)arg;
+	controllistener_t *listener = conn->listener;
+	controlkey_t *key = NULL;
+	isc_event_t *event = NULL;
 	isccc_time_t sent;
 	isccc_time_t exp;
 	uint32_t nonce;
-	isccc_sexpr_t *data = NULL;
 
-	listener = conn->listener;
-	algorithm = DST_ALG_UNKNOWN;
-	secret.rstart = NULL;
-	text = NULL;
+	conn->ccmsg_valid = false;
 
 	/* Is the server shutting down? */
 	if (listener->controls->shuttingdown) {
-		goto cleanup;
+		return;
 	}
 
 	if (result != ISC_R_SUCCESS) {
@@ -281,7 +386,7 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 			log_invalid(&conn->ccmsg, result);
 		}
 
-		goto cleanup;
+		return;
 	}
 
 	for (key = ISC_LIST_HEAD(listener->keys); key != NULL;
@@ -291,19 +396,22 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 
 		ccregion.rstart = isc_buffer_base(conn->ccmsg.buffer);
 		ccregion.rend = isc_buffer_used(conn->ccmsg.buffer);
-		secret.rstart = isc_mem_get(listener->mctx, key->secret.length);
-		memmove(secret.rstart, key->secret.base, key->secret.length);
-		secret.rend = secret.rstart + key->secret.length;
-		algorithm = key->algorithm;
-		result = isccc_cc_fromwire(&ccregion, &request, algorithm,
-					   &secret);
+		conn->secret.rstart = isc_mem_get(listener->mctx,
+						  key->secret.length);
+		memmove(conn->secret.rstart, key->secret.base,
+			key->secret.length);
+		conn->secret.rend = conn->secret.rstart + key->secret.length;
+		conn->alg = key->algorithm;
+		result = isccc_cc_fromwire(&ccregion, &conn->request, conn->alg,
+					   &conn->secret);
 		if (result == ISC_R_SUCCESS) {
 			break;
 		}
-		isc_mem_put(listener->mctx, secret.rstart, REGION_SIZE(secret));
+		isc_mem_put(listener->mctx, conn->secret.rstart,
+			    REGION_SIZE(conn->secret));
 		if (result != ISCCC_R_BADAUTH) {
 			log_invalid(&conn->ccmsg, result);
-			goto cleanup;
+			return;
 		}
 	}
 
@@ -313,157 +421,106 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	}
 
 	/* We shouldn't be getting a reply. */
-	if (isccc_cc_isreply(request)) {
+	if (isccc_cc_isreply(conn->request)) {
 		log_invalid(&conn->ccmsg, ISC_R_FAILURE);
-		goto cleanup_request;
+		goto cleanup;
 	}
 
-	isc_stdtime_get(&now);
+	isc_stdtime_get(&conn->now);
 
 	/*
 	 * Limit exposure to replay attacks.
 	 */
-	_ctrl = isccc_alist_lookup(request, "_ctrl");
-	if (!isccc_alist_alistp(_ctrl)) {
+	conn->ctrl = isccc_alist_lookup(conn->request, "_ctrl");
+	if (!isccc_alist_alistp(conn->ctrl)) {
 		log_invalid(&conn->ccmsg, ISC_R_FAILURE);
-		goto cleanup_request;
+		goto cleanup;
 	}
 
-	if (isccc_cc_lookupuint32(_ctrl, "_tim", &sent) == ISC_R_SUCCESS) {
-		if ((sent + CLOCKSKEW) < now || (sent - CLOCKSKEW) > now) {
+	if (isccc_cc_lookupuint32(conn->ctrl, "_tim", &sent) == ISC_R_SUCCESS) {
+		if ((sent + CLOCKSKEW) < conn->now ||
+		    (sent - CLOCKSKEW) > conn->now) {
 			log_invalid(&conn->ccmsg, ISCCC_R_CLOCKSKEW);
-			goto cleanup_request;
+			goto cleanup;
 		}
 	} else {
 		log_invalid(&conn->ccmsg, ISC_R_FAILURE);
-		goto cleanup_request;
+		goto cleanup;
 	}
 
 	/*
 	 * Expire messages that are too old.
 	 */
-	if (isccc_cc_lookupuint32(_ctrl, "_exp", &exp) == ISC_R_SUCCESS &&
-	    now > exp) {
+	if (isccc_cc_lookupuint32(conn->ctrl, "_exp", &exp) == ISC_R_SUCCESS &&
+	    conn->now > exp)
+	{
 		log_invalid(&conn->ccmsg, ISCCC_R_EXPIRED);
-		goto cleanup_request;
+		goto cleanup;
 	}
 
 	/*
 	 * Duplicate suppression (required for UDP).
 	 */
-	isccc_cc_cleansymtab(listener->controls->symtab, now);
-	result = isccc_cc_checkdup(listener->controls->symtab, request, now);
+	isccc_cc_cleansymtab(listener->controls->symtab, conn->now);
+	result = isccc_cc_checkdup(listener->controls->symtab, conn->request,
+				   conn->now);
 	if (result != ISC_R_SUCCESS) {
 		if (result == ISC_R_EXISTS) {
 			result = ISCCC_R_DUPLICATE;
 		}
 		log_invalid(&conn->ccmsg, result);
-		goto cleanup_request;
+		goto cleanup;
 	}
 
 	if (conn->nonce != 0 &&
-	    (isccc_cc_lookupuint32(_ctrl, "_nonce", &nonce) != ISC_R_SUCCESS ||
+	    (isccc_cc_lookupuint32(conn->ctrl, "_nonce", &nonce) !=
+		     ISC_R_SUCCESS ||
 	     conn->nonce != nonce))
 	{
 		log_invalid(&conn->ccmsg, ISCCC_R_BADAUTH);
-		goto cleanup_request;
+		goto cleanup;
 	}
 
-	isc_buffer_allocate(listener->mctx, &text, 2 * 2048);
+	isc_buffer_allocate(listener->mctx, &conn->text, 2 * 2048);
+
+	conn->ccmsg_valid = true;
 
-	/*
-	 * Establish nonce.
-	 */
 	if (conn->nonce == 0) {
+		/*
+		 * Establish nonce.
+		 */
 		while (conn->nonce == 0) {
 			isc_nonce_buf(&conn->nonce, sizeof(conn->nonce));
 		}
-		eresult = ISC_R_SUCCESS;
-	} else {
-		eresult = named_control_docommand(request, listener->readonly,
-						  &text);
-	}
-
-	result = isccc_cc_createresponse(request, now, now + 60, &response);
-	if (result != ISC_R_SUCCESS) {
-		goto cleanup_request;
-	}
-
-	data = isccc_alist_lookup(response, "_data");
-	if (data != NULL) {
-		if (isccc_cc_defineuint32(data, "result", eresult) == NULL) {
-			goto cleanup_response;
-		}
-	}
-
-	if (eresult != ISC_R_SUCCESS) {
-		if (data != NULL) {
-			const char *estr = isc_result_totext(eresult);
-			if (isccc_cc_definestring(data, "err", estr) == NULL) {
-				goto cleanup_response;
-			}
-		}
-	}
-
-	if (isc_buffer_usedlength(text) > 0) {
-		if (data != NULL) {
-			char *str = (char *)isc_buffer_base(text);
-			if (isccc_cc_definestring(data, "text", str) == NULL) {
-				goto cleanup_response;
-			}
-		}
+		conn->result = ISC_R_SUCCESS;
+		control_respond(handle, result, conn);
+		return;
 	}
 
-	_ctrl = isccc_alist_lookup(response, "_ctrl");
-	if (_ctrl == NULL ||
-	    isccc_cc_defineuint32(_ctrl, "_nonce", conn->nonce) == NULL)
-	{
-		goto cleanup_response;
-	}
+	/*
+	 * Trigger the command.
+	 */
+	isc_nmhandle_ref(handle);
+	event = isc_event_allocate(listener->mctx, conn, NAMED_EVENT_COMMAND,
+				   control_command, conn, sizeof(isc_event_t));
+	isc_task_send(named_g_server->task, &event);
+	return;
 
-	if (conn->buffer == NULL) {
-		isc_buffer_allocate(listener->mctx, &conn->buffer, 2 * 2048);
+cleanup:
+	if (conn->response != NULL) {
+		isccc_sexpr_free(&conn->response);
 	}
-
-	isc_buffer_clear(conn->buffer);
-	/* Skip the length field (4 bytes) */
-	isc_buffer_add(conn->buffer, 4);
-
-	result = isccc_cc_towire(response, &conn->buffer, algorithm, &secret);
-	if (result != ISC_R_SUCCESS) {
-		goto cleanup_response;
+	if (conn->request != NULL) {
+		isccc_sexpr_free(&conn->request);
 	}
 
-	isc_buffer_init(&b, conn->buffer->base, 4);
-	isc_buffer_putuint32(&b, conn->buffer->used - 4);
-
-	r.base = conn->buffer->base;
-	r.length = conn->buffer->used;
-
-	result = isc_nm_send(handle, &r, control_senddone, conn);
-	if (result != ISC_R_SUCCESS) {
-		goto cleanup_response;
+	if (conn->secret.rstart != NULL) {
+		isc_mem_put(listener->mctx, conn->secret.rstart,
+			    REGION_SIZE(conn->secret));
 	}
-	conn->sending = true;
-
-	isc_mem_put(listener->mctx, secret.rstart, REGION_SIZE(secret));
-	isccc_sexpr_free(&request);
-	isccc_sexpr_free(&response);
-	isc_buffer_free(&text);
-	return;
-
-cleanup_response:
-	isccc_sexpr_free(&response);
-
-cleanup_request:
-	isccc_sexpr_free(&request);
-	isc_mem_put(listener->mctx, secret.rstart, REGION_SIZE(secret));
-	if (text != NULL) {
-		isc_buffer_free(&text);
+	if (conn->text != NULL) {
+		isc_buffer_free(&conn->text);
 	}
-
-cleanup:
-	conn->ccmsg_valid = false;
 }
 
 static void
@@ -524,7 +581,8 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
 
 	*conn = (controlconnection_t){ .handle = handle,
 				       .listener = listener,
-				       .ccmsg_valid = true };
+				       .ccmsg_valid = true,
+				       .alg = DST_ALG_UNKNOWN };
 
 	isccc_ccmsg_init(listener->mctx, handle, &conn->ccmsg);
 
diff --git a/bin/named/include/named/server.h b/bin/named/include/named/server.h
index fdc89958a33..90c9980e17b 100644
--- a/bin/named/include/named/server.h
+++ b/bin/named/include/named/server.h
@@ -38,6 +38,7 @@
 #define NAMED_EVENTCLASS    ISC_EVENTCLASS(0x4E43)
 #define NAMED_EVENT_RELOAD  (NAMED_EVENTCLASS + 0)
 #define NAMED_EVENT_DELZONE (NAMED_EVENTCLASS + 1)
+#define NAMED_EVENT_COMMAND (NAMED_EVENTCLASS + 2)
 
 /*%
  * Name server state.  Better here than in lots of separate global variables.
@@ -80,8 +81,6 @@ struct named_server {
 	uint32_t interface_interval;
 	uint32_t heartbeat_interval;
 
-	isc_mutex_t    reload_event_lock;
-	isc_event_t *  reload_event;
 	named_reload_t reload_status;
 
 	bool flushonshutdown;
diff --git a/bin/named/server.c b/bin/named/server.c
index 0d3111c024b..da1566ebb86 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -9936,14 +9936,6 @@ named_server_create(isc_mem_t *mctx, named_server_t **serverp) {
 				     &server->in_roothints),
 		   "setting up root hints");
 
-	isc_mutex_init(&server->reload_event_lock);
-
-	server->reload_event = isc_event_allocate(
-		named_g_mctx, server, NAMED_EVENT_RELOAD, named_server_reload,
-		server, sizeof(isc_event_t));
-	CHECKFATAL(server->reload_event == NULL ? ISC_R_NOMEMORY
-						: ISC_R_SUCCESS,
-		   "allocating reload event");
 	server->reload_status = NAMED_RELOAD_IN_PROGRESS;
 
 	/*
@@ -10113,9 +10105,6 @@ named_server_destroy(named_server_t **serverp) {
 
 	dst_lib_destroy();
 
-	isc_event_free(&server->reload_event);
-	isc_mutex_destroy(&server->reload_event_lock);
-
 	INSIST(ISC_LIST_EMPTY(server->kasplist));
 	INSIST(ISC_LIST_EMPTY(server->viewlist));
 	INSIST(ISC_LIST_EMPTY(server->cachelist));
@@ -10293,7 +10282,7 @@ cleanup:
  */
 static void
 named_server_reload(isc_task_t *task, isc_event_t *event) {
-	named_server_t *server = (named_server_t *)event->ev_arg;
+	named_server_t *server = (named_server_t *)event->ev_sender;
 
 	INSIST(task == server->task);
 	UNUSED(task);
@@ -10303,19 +10292,15 @@ named_server_reload(isc_task_t *task, isc_event_t *event) {
 		      "received SIGHUP signal to reload zones");
 	(void)reload(server);
 
-	LOCK(&server->reload_event_lock);
-	INSIST(server->reload_event == NULL);
-	server->reload_event = event;
-	UNLOCK(&server->reload_event_lock);
+	isc_event_free(&event);
 }
 
 void
 named_server_reloadwanted(named_server_t *server) {
-	LOCK(&server->reload_event_lock);
-	if (server->reload_event != NULL) {
-		isc_task_send(server->task, &server->reload_event);
-	}
-	UNLOCK(&server->reload_event_lock);
+	isc_event_t *event = isc_event_allocate(
+		named_g_mctx, server, NAMED_EVENT_RELOAD, named_server_reload,
+		NULL, sizeof(isc_event_t));
+	isc_task_send(server->task, &event);
 }
 
 void
diff --git a/bin/rndc/rndc.c b/bin/rndc/rndc.c
index 7753f7f463e..ef6b31c5317 100644
--- a/bin/rndc/rndc.c
+++ b/bin/rndc/rndc.c
@@ -373,9 +373,9 @@ rndc_recvdone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	if (atomic_load_acquire(&sends) == 0 &&
 	    atomic_load_acquire(&recvs) == 0) {
 		shuttingdown = true;
+		isc_nmhandle_unref(handle);
 		isc_task_shutdown(rndc_task);
 		isc_app_shutdown();
-		isc_nmhandle_unref(handle);
 	}
 }
 
diff --git a/lib/isccc/ccmsg.c b/lib/isccc/ccmsg.c
index 5b79e6148db..72e79cd9ca4 100644
--- a/lib/isccc/ccmsg.c
+++ b/lib/isccc/ccmsg.c
@@ -49,13 +49,13 @@ recv_nonce(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region,
 	isccc_ccmsg_t *ccmsg = arg;
 	isc_result_t result;
 
+	INSIST(VALID_CCMSG(ccmsg));
+
 	if (eresult == ISC_R_CANCELED || eresult == ISC_R_EOF) {
 		ccmsg->result = eresult;
 		goto done;
 	}
 
-	INSIST(VALID_CCMSG(ccmsg));
-
 	if (region == NULL && eresult == ISC_R_SUCCESS) {
 		ccmsg->result = ISC_R_EOF;
 		goto done;
@@ -116,13 +116,13 @@ recv_message(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region,
 	isccc_ccmsg_t *ccmsg = arg;
 	size_t size;
 
+	INSIST(VALID_CCMSG(ccmsg));
+
 	if (eresult == ISC_R_CANCELED || eresult == ISC_R_EOF) {
 		ccmsg->result = eresult;
 		goto done;
 	}
 
-	INSIST(VALID_CCMSG(ccmsg));
-
 	if (region == NULL && eresult == ISC_R_SUCCESS) {
 		ccmsg->result = ISC_R_EOF;
 		goto done;