Merge branch '4228-fix-heap-use-after-free-in-dns_dispatch_createtcp' into 'main'

Attach to the dns_dispatchmgr in the dns_view object

Closes #4228

See merge request isc-projects/bind9!8203

CHANGES

@@ -1,3 +1,6 @@
+6226.	[bug]		Attach dispatchmgr in the dns_view object to prevent
+			use-after-free when shutting down. [GL #4228]
+
 6225.	[func]		Convert dns_nta, dns_forward and dns_keytable units
 			to use QP trie instead of an RBT. [GL !7811]
 

bin/delv/delv.c

@@ -2148,7 +2148,8 @@ run_server(void *arg) {
 	CHECK(ns_interfacemgr_create(mctx, sctx, loopmgr, netmgr, dispatchmgr,
 				     NULL, false, &interfacemgr));
 
-	CHECK(dns_view_create(mctx, dns_rdataclass_in, "_default", &view));
+	CHECK(dns_view_create(mctx, dispatchmgr, dns_rdataclass_in, "_default",
+			      &view));
 	CHECK(dns_cache_create(loopmgr, dns_rdataclass_in, "", &cache));
 	dns_view_setcache(view, cache, false);
 	dns_cache_detach(&cache);
@@ -2164,7 +2165,6 @@ run_server(void *arg) {
 	dns_view_initsecroots(view);
 	CHECK(setup_dnsseckeys(NULL, view));
 
-	dns_view_setdispatchmgr(view, dispatchmgr);
 	CHECK(dns_view_createresolver(view, loopmgr, 1, netmgr, 0,
 				      tlsctx_client_cache, dispatch, NULL));
 

bin/named/server.c

@@ -6423,13 +6423,12 @@ create_view(const cfg_obj_t *vconfig, dns_viewlist_t *viewlist,
 	}
 	INSIST(view == NULL);
 
-	result = dns_view_create(named_g_mctx, viewclass, viewname, &view);
+	result = dns_view_create(named_g_mctx, named_g_dispatchmgr, viewclass,
+				 viewname, &view);
 	if (result != ISC_R_SUCCESS) {
 		return (result);
 	}
 
-	dns_view_setdispatchmgr(view, named_g_dispatchmgr);
-
 	isc_nonce_buf(view->secret, sizeof(view->secret));
 
 	ISC_LIST_APPEND(*viewlist, view, link);

bin/tests/system/pipelined/pipequeries.c

@@ -282,7 +282,7 @@ main(int argc, char *argv[]) {
 	RUNCHECK(dns_requestmgr_create(mctx, loopmgr, dispatchmgr, dispatchv4,
 				       NULL, &requestmgr));
 
-	RUNCHECK(dns_view_create(mctx, 0, "_test", &view));
+	RUNCHECK(dns_view_create(mctx, NULL, 0, "_test", &view));
 
 	isc_loopmgr_setup(loopmgr, sendqueries, NULL);
 	isc_loopmgr_teardown(loopmgr, teardown_view, view);

bin/tools/mdig.c

@@ -2167,7 +2167,7 @@ main(int argc, char *argv[]) {
 		mctx, loopmgr, dispatchmgr, have_ipv4 ? dispatchvx : NULL,
 		have_ipv6 ? dispatchvx : NULL, &requestmgr));
 
-	RUNCHECK(dns_view_create(mctx, 0, "_mdig", &view));
+	RUNCHECK(dns_view_create(mctx, NULL, 0, "_mdig", &view));
 
 	query = ISC_LIST_HEAD(queries);
 	isc_loopmgr_setup(loopmgr, sendqueries, query);

fuzz/dns_message_checksig.c

@@ -183,7 +183,7 @@ LLVMFuzzerInitialize(int *argc ISC_ATTR_UNUSED, char ***argv ISC_ATTR_UNUSED) {
 
 	isc_loopmgr_create(mctx, 1, &loopmgr);
 
-	result = dns_view_create(mctx, dns_rdataclass_in, "view", &view);
+	result = dns_view_create(mctx, NULL, dns_rdataclass_in, "view", &view);
 	if (result != ISC_R_SUCCESS) {
 		fprintf(stderr, "dns_view_create failed: %s\n",
 			isc_result_totext(result));

lib/dns/client.c

@@ -206,13 +206,12 @@ createview(isc_mem_t *mctx, dns_rdataclass_t rdclass, isc_loopmgr_t *loopmgr,
 	isc_result_t result;
 	dns_view_t *view = NULL;
 
-	result = dns_view_create(mctx, rdclass, DNS_CLIENTVIEW_NAME, &view);
+	result = dns_view_create(mctx, dispatchmgr, rdclass,
+				 DNS_CLIENTVIEW_NAME, &view);
 	if (result != ISC_R_SUCCESS) {
 		return (result);
 	}
 
-	dns_view_setdispatchmgr(view, dispatchmgr);
-
 	/* Initialize view security roots */
 	dns_view_initsecroots(view);
 

lib/dns/include/dns/view.h

@@ -259,8 +259,8 @@ struct dns_view {
 #endif /* HAVE_LMDB */
 
 isc_result_t
-dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
-		dns_view_t **viewp);
+dns_view_create(isc_mem_t *mctx, dns_dispatchmgr_t *dispmgr,
+		dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp);
 /*%<
  * Create a view.
  *
@@ -378,9 +378,6 @@ dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr,
  *
  *\li	'view' does not have a resolver already.
  *
- *\li	A dispatch manager has been associated with the view by calling
- *	dns_view_setdispatchmgr().
- *
  *\li	The requirements of dns_resolver_create() apply to 'ndisp',
  *	'netmgr', 'options', 'tlsctx_cache', 'dispatchv4', and 'dispatchv6'.
  *
@@ -1254,12 +1251,10 @@ dns_view_getudpsize(dns_view_t *view);
  * Get the current EDNS UDP buffer size.
  */
 
-void
-dns_view_setdispatchmgr(dns_view_t *view, dns_dispatchmgr_t *dispatchmgr);
 dns_dispatchmgr_t *
 dns_view_getdispatchmgr(dns_view_t *view);
 /*%<
- * Set/get the dispatch manager for the view; this wil be used
+ * Get the attached dispatch manager for the view; this will be used
  * by the resolver and request managers to send and receive DNS
  * messages.
  */

lib/dns/view.c

@@ -79,7 +79,8 @@
 #define DEFAULT_EDNS_BUFSIZE 1232
 
 isc_result_t
-dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
+dns_view_create(isc_mem_t *mctx, dns_dispatchmgr_t *dispatchmgr,
+		dns_rdataclass_t rdclass, const char *name,
 		dns_view_t **viewp) {
 	dns_view_t *view = NULL;
 	isc_result_t result;
@@ -129,6 +130,10 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
 
 	isc_mem_attach(mctx, &view->mctx);
 
+	if (dispatchmgr != NULL) {
+		dns_dispatchmgr_attach(dispatchmgr, &view->dispatchmgr);
+	}
+
 	isc_mutex_init(&view->lock);
 
 	isc_rwlock_init(&view->sfd_lock);
@@ -442,6 +447,7 @@ dns_view_detach(dns_view_t **viewp) {
 		dns_resolver_t *resolver = NULL;
 		dns_adb_t *adb = NULL;
 		dns_requestmgr_t *requestmgr = NULL;
+		dns_dispatchmgr_t *dispatchmgr = NULL;
 
 		isc_refcount_destroy(&view->references);
 
@@ -477,6 +483,7 @@ dns_view_detach(dns_view_t **viewp) {
 			}
 		}
 		adb = rcu_xchg_pointer(&view->adb, NULL);
+		dispatchmgr = rcu_xchg_pointer(&view->dispatchmgr, NULL);
 		rcu_read_unlock();
 
 		if (view->requestmgr != NULL) {
@@ -510,14 +517,15 @@ dns_view_detach(dns_view_t **viewp) {
 		if (resolver != NULL) {
 			dns_resolver_detach(&resolver);
 		}
-		if (adb != NULL || zonetable != NULL) {
-			synchronize_rcu();
-			if (adb != NULL) {
-				dns_adb_detach(&adb);
-			}
-			if (zonetable != NULL) {
-				dns_zt_detach(&zonetable);
-			}
+		synchronize_rcu();
+		if (dispatchmgr != NULL) {
+			dns_dispatchmgr_detach(&dispatchmgr);
+		}
+		if (adb != NULL) {
+			dns_adb_detach(&adb);
+		}
+		if (zonetable != NULL) {
+			dns_zt_detach(&zonetable);
 		}
 		if (requestmgr != NULL) {
 			dns_requestmgr_detach(&requestmgr);
@@ -2411,16 +2419,18 @@ dns_view_getudpsize(dns_view_t *view) {
 	return (view->udpsize);
 }
 
-void
-dns_view_setdispatchmgr(dns_view_t *view, dns_dispatchmgr_t *dispatchmgr) {
-	REQUIRE(DNS_VIEW_VALID(view));
-	view->dispatchmgr = dispatchmgr;
-}
-
 dns_dispatchmgr_t *
 dns_view_getdispatchmgr(dns_view_t *view) {
 	REQUIRE(DNS_VIEW_VALID(view));
-	return (view->dispatchmgr);
+
+	rcu_read_lock();
+	dns_dispatchmgr_t *dispatchmgr = rcu_dereference(view->dispatchmgr);
+	if (dispatchmgr != NULL) {
+		dns_dispatchmgr_ref(dispatchmgr);
+	}
+	rcu_read_unlock();
+
+	return (dispatchmgr);
 }
 
 isc_result_t

lib/dns/xfrin.c

@@ -928,9 +928,15 @@ xfrin_start(dns_xfrin_t *xfr) {
 	if (xfr->transport == NULL ||
 	    dns_transport_get_type(xfr->transport) == DNS_TRANSPORT_TCP)
 	{
-		result = dns_dispatch_gettcp(dns_view_getdispatchmgr(xfr->view),
-					     &xfr->primaryaddr,
-					     &xfr->sourceaddr, &xfr->disp);
+		dns_dispatchmgr_t *dispmgr = dns_view_getdispatchmgr(xfr->view);
+		if (dispmgr == NULL) {
+			result = ISC_R_SHUTTINGDOWN;
+		} else {
+			result = dns_dispatch_gettcp(dispmgr, &xfr->primaryaddr,
+						     &xfr->sourceaddr,
+						     &xfr->disp);
+			dns_dispatchmgr_detach(&dispmgr);
+		}
 	}
 	if (result == ISC_R_SUCCESS) {
 		char peer[ISC_SOCKADDR_FORMATSIZE];
@@ -938,9 +944,16 @@ xfrin_start(dns_xfrin_t *xfr) {
 		xfrin_log(xfr, ISC_LOG_DEBUG(1),
 			  "attached to TCP connection to %s", peer);
 	} else {
-		CHECK(dns_dispatch_createtcp(dns_view_getdispatchmgr(xfr->view),
-					     &xfr->sourceaddr,
-					     &xfr->primaryaddr, &xfr->disp));
+		dns_dispatchmgr_t *dispmgr = dns_view_getdispatchmgr(xfr->view);
+		if (dispmgr == NULL) {
+			result = ISC_R_SHUTTINGDOWN;
+		} else {
+			result = dns_dispatch_createtcp(
+				dispmgr, &xfr->sourceaddr, &xfr->primaryaddr,
+				&xfr->disp);
+			dns_dispatchmgr_detach(&dispmgr);
+		}
+		CHECK(result);
 	}
 
 	/* Set the maximum timer */

tests/dns/dnstap_test.c

@@ -146,7 +146,7 @@ ISC_RUN_TEST_IMPL(dns_dt_send) {
 	isc_time_t p, f;
 	struct fstrm_iothr_options *fopt;
 
-	result = dns_test_makeview("test", false, &view);
+	result = dns_test_makeview("test", false, false, &view);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	fopt = fstrm_iothr_options_init();

tests/dns/keytable_test.c

@@ -170,7 +170,7 @@ create_tables(void) {
 	dns_name_t *keyname = dns_fixedname_name(&fn);
 	isc_stdtime_t now = isc_stdtime_now();
 
-	assert_int_equal(dns_test_makeview("view", false, &view),
+	assert_int_equal(dns_test_makeview("view", false, false, &view),
 			 ISC_R_SUCCESS);
 
 	dns_keytable_create(view, &keytable);
@@ -602,7 +602,7 @@ ISC_LOOP_TEST_IMPL(nta) {
 	dns_view_t *myview = NULL;
 	isc_stdtime_t now = isc_stdtime_now();
 
-	result = dns_test_makeview("view", false, &myview);
+	result = dns_test_makeview("view", false, false, &myview);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	dns_view_initsecroots(myview);

tests/dns/resolver_test.c

@@ -35,7 +35,6 @@
 
 #include <tests/dns.h>
 
-static dns_dispatchmgr_t *dispatchmgr = NULL;
 static dns_dispatch_t *dispatch = NULL;
 static dns_view_t *view = NULL;
 static isc_tlsctx_cache_t *tlsctx_cache = NULL;
@@ -44,21 +43,22 @@ static int
 setup_test(void **state) {
 	isc_result_t result;
 	isc_sockaddr_t local;
+	dns_dispatchmgr_t *dispatchmgr = NULL;
 
 	setup_managers(state);
 
-	result = dns_dispatchmgr_create(mctx, netmgr, &dispatchmgr);
+	result = dns_test_makeview("view", true, false, &view);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
-	result = dns_test_makeview("view", false, &view);
-	assert_int_equal(result, ISC_R_SUCCESS);
-
-	dns_view_setdispatchmgr(view, dispatchmgr);
+	dispatchmgr = dns_view_getdispatchmgr(view);
+	assert_non_null(dispatchmgr);
 
 	isc_sockaddr_any(&local);
 	result = dns_dispatch_createudp(dispatchmgr, &local, &dispatch);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
+	dns_dispatchmgr_detach(&dispatchmgr);
+
 	return (0);
 }
 
@@ -66,7 +66,6 @@ static int
 teardown_test(void **state) {
 	dns_dispatch_detach(&dispatch);
 	dns_view_detach(&view);
-	dns_dispatchmgr_detach(&dispatchmgr);
 	teardown_managers(state);
 
 	return (0);

tests/include/tests/dns.h

@@ -50,7 +50,8 @@ typedef struct {
 	}
 
 isc_result_t
-dns_test_makeview(const char *name, bool with_cache, dns_view_t **viewp);
+dns_test_makeview(const char *name, bool with_dispatchmgr, bool with_cache,
+		  dns_view_t **viewp);
 
 /*%
  * Create a zone with origin 'name', return a pointer to the zone object in

tests/libtest/dns.c

@@ -45,6 +45,7 @@
 
 #include <dns/callbacks.h>
 #include <dns/db.h>
+#include <dns/dispatch.h>
 #include <dns/fixedname.h>
 #include <dns/log.h>
 #include <dns/name.h>
@@ -59,12 +60,27 @@ dns_zonemgr_t *zonemgr = NULL;
  * Create a view.
  */
 isc_result_t
-dns_test_makeview(const char *name, bool with_cache, dns_view_t **viewp) {
+dns_test_makeview(const char *name, bool with_dispatchmgr, bool with_cache,
+		  dns_view_t **viewp) {
 	isc_result_t result;
 	dns_view_t *view = NULL;
 	dns_cache_t *cache = NULL;
+	dns_dispatchmgr_t *dispatchmgr = NULL;
+
+	if (with_dispatchmgr) {
+		result = dns_dispatchmgr_create(mctx, netmgr, &dispatchmgr);
+		if (result != ISC_R_SUCCESS) {
+			return (result);
+		}
+	}
+
+	result = dns_view_create(mctx, dispatchmgr, dns_rdataclass_in, name,
+				 &view);
+
+	if (dispatchmgr != NULL) {
+		dns_dispatchmgr_detach(&dispatchmgr);
+	}
 
-	result = dns_view_create(mctx, dns_rdataclass_in, name, &view);
 	if (result != ISC_R_SUCCESS) {
 		return (result);
 	}
@@ -124,7 +140,7 @@ dns_test_makezone(const char *name, dns_zone_t **zonep, dns_view_t *view,
 	 * If requested, create a view.
 	 */
 	if (createview) {
-		result = dns_test_makeview("view", false, &view);
+		result = dns_test_makeview("view", false, false, &view);
 		if (result != ISC_R_SUCCESS) {
 			goto detach_zone;
 		}

tests/libtest/ns.c

@@ -439,7 +439,8 @@ ns_test_qctx_create(const ns_test_qctx_create_params_t *params,
 	/*
 	 * Every client needs to belong to a view.
 	 */
-	result = dns_test_makeview("view", params->with_cache, &client->view);
+	result = dns_test_makeview("view", false, params->with_cache,
+				   &client->view);
 	if (result != ISC_R_SUCCESS) {
 		goto detach_client;
 	}

tests/ns/notify_test.c

@@ -70,7 +70,7 @@ ISC_LOOP_TEST_IMPL(notify_start) {
 	result = ns_test_getclient(NULL, false, &client);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
-	result = dns_test_makeview("view", false, &client->view);
+	result = dns_test_makeview("view", false, false, &client->view);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	result = ns_test_serve_zone("example.com",