Merge branch '3560-fix-view-resolver-layer-weak-references-violation' into 'main'

Resolve violation of weak referencing dns_view

Closes #3560

See merge request isc-projects/bind9!6848

bin/named/server.c

@@ -4931,7 +4931,7 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	if (udpsize > 4096) {
 		udpsize = 4096;
 	}
-	dns_resolver_setudpsize(view->resolver, (uint16_t)udpsize);
+	dns_view_setudpsize(view, (uint16_t)udpsize);
 
 	/*
 	 * Set the maximum UDP response size.

lib/dns/include/dns/resolver.h

@@ -421,18 +421,6 @@ dns_resolver_addalternate(dns_resolver_t *resolver, const isc_sockaddr_t *alt,
  * \li	only one of 'name' or 'alt' to be valid.
  */
 
-void
-dns_resolver_setudpsize(dns_resolver_t *resolver, uint16_t udpsize);
-/*%<
- * Set the EDNS UDP buffer size advertised by the server.
- */
-
-uint16_t
-dns_resolver_getudpsize(dns_resolver_t *resolver);
-/*%<
- * Get the current EDNS UDP buffer size.
- */
-
 void
 dns_resolver_reset_algorithms(dns_resolver_t *resolver);
 /*%<

lib/dns/include/dns/view.h

@@ -185,6 +185,7 @@ struct dns_view {
 	dns_dlzdblist_t	  dlz_unsearched;
 	uint32_t	  fail_ttl;
 	dns_badcache_t	 *failcache;
+	unsigned int	  udpsize;
 
 	/*
 	 * Configurable data for server use only,
@@ -1299,4 +1300,22 @@ dns_view_sfd_find(dns_view_t *view, const dns_name_t *name,
  *\li	'foundname' to be valid with a buffer sufficient to hold the name.
  */
 
+isc_result_t
+dns_view_getresolver(dns_view_t *view, dns_resolver_t **resolverp);
+/*%<
+ * Return the resolver associated with the view.
+ */
+
+void
+dns_view_setudpsize(dns_view_t *view, uint16_t udpsize);
+/*%<
+ * Set the EDNS UDP buffer size advertised by the server.
+ */
+
+uint16_t
+dns_view_getudpsize(dns_view_t *view);
+/*%<
+ * Get the current EDNS UDP buffer size.
+ */
+
 ISC_LANG_ENDDECLS

lib/dns/nta.c

@@ -44,6 +44,7 @@
 struct dns_ntatable {
 	/* Unlocked. */
 	unsigned int magic;
+	isc_mem_t *mctx;
 	dns_view_t *view;
 	isc_rwlock_t rwlock;
 	isc_loopmgr_t *loopmgr;
@@ -52,7 +53,7 @@ struct dns_ntatable {
 	isc_refcount_t references;
 	/* Locked by rwlock. */
 	dns_rbt_t *table;
-	bool shuttingdown;
+	atomic_bool shuttingdown;
 };
 
 struct dns__nta {
@@ -118,16 +119,18 @@ dns_ntatable_create(dns_view_t *view, isc_taskmgr_t *taskmgr,
 	ntatable = isc_mem_get(view->mctx, sizeof(*ntatable));
 	*ntatable = (dns_ntatable_t){
 		.loopmgr = loopmgr,
-		.view = view,
 	};
 
+	isc_mem_attach(view->mctx, &ntatable->mctx);
+	dns_view_weakattach(view, &ntatable->view);
+
 	result = isc_task_create(taskmgr, &ntatable->task, 0);
 	if (result != ISC_R_SUCCESS) {
 		goto cleanup_ntatable;
 	}
 	isc_task_setname(ntatable->task, "ntatable", ntatable);
 
-	result = dns_rbt_create(view->mctx, dns__nta_free, NULL,
+	result = dns_rbt_create(ntatable->mctx, dns__nta_free, NULL,
 				&ntatable->table);
 	if (result != ISC_R_SUCCESS) {
 		goto cleanup_task;
@@ -146,7 +149,7 @@ cleanup_task:
 	isc_task_detach(&ntatable->task);
 
 cleanup_ntatable:
-	isc_mem_put(view->mctx, ntatable, sizeof(*ntatable));
+	isc_mem_putanddetach(&ntatable->mctx, ntatable, sizeof(*ntatable));
 
 	return (result);
 }
@@ -158,7 +161,8 @@ dns__ntatable_destroy(dns_ntatable_t *ntatable) {
 	dns_rbt_destroy(&ntatable->table);
 	isc_rwlock_destroy(&ntatable->rwlock);
 	isc_task_detach(&ntatable->task);
-	isc_mem_put(ntatable->view->mctx, ntatable, sizeof(*ntatable));
+	INSIST(ntatable->view == NULL);
+	isc_mem_putanddetach(&ntatable->mctx, ntatable, sizeof(*ntatable));
 }
 
 ISC_REFCOUNT_IMPL(dns_ntatable, dns__ntatable_destroy);
@@ -222,14 +226,13 @@ fetch_done(isc_task_t *task, isc_event_t *event) {
 	RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_read);
 
 	dns__nta_detach(&nta); /* for dns_resolver_createfetch() */
-	dns_view_weakdetach(&view);
 }
 
 static void
 checkbogus(void *arg) {
 	dns__nta_t *nta = arg;
 	dns_ntatable_t *ntatable = nta->ntatable;
-	dns_view_t *view = NULL;
+	dns_resolver_t *resolver = NULL;
 	isc_result_t result;
 
 	if (nta->fetch != NULL) {
@@ -243,17 +246,25 @@ checkbogus(void *arg) {
 		dns_rdataset_disassociate(&nta->sigrdataset);
 	}
 
+	if (atomic_load(&ntatable->shuttingdown)) {
+		isc_timer_stop(nta->timer);
+		return;
+	}
+
+	result = dns_view_getresolver(ntatable->view, &resolver);
+	if (result != ISC_R_SUCCESS) {
+		return;
+	}
+
 	dns__nta_ref(nta); /* for dns_resolver_createfetch */
-	dns_view_weakattach(ntatable->view, &view);
 	result = dns_resolver_createfetch(
-		view->resolver, nta->name, dns_rdatatype_nsec, NULL, NULL, NULL,
-		NULL, 0, DNS_FETCHOPT_NONTA, 0, NULL, ntatable->task,
-		fetch_done, nta, &nta->rdataset, &nta->sigrdataset,
-		&nta->fetch);
+		resolver, nta->name, dns_rdatatype_nsec, NULL, NULL, NULL, NULL,
+		0, DNS_FETCHOPT_NONTA, 0, NULL, ntatable->task, fetch_done, nta,
+		&nta->rdataset, &nta->sigrdataset, &nta->fetch);
 	if (result != ISC_R_SUCCESS) {
 		dns__nta_detach(&nta); /* for dns_resolver_createfetch() */
-		dns_view_weakdetach(&view);
 	}
+	dns_resolver_detach(&resolver);
 }
 
 static void
@@ -278,21 +289,18 @@ static void
 nta_create(dns_ntatable_t *ntatable, const dns_name_t *name,
 	   dns__nta_t **target) {
 	dns__nta_t *nta = NULL;
-	dns_view_t *view;
 
 	REQUIRE(VALID_NTATABLE(ntatable));
 	REQUIRE(target != NULL && *target == NULL);
 
-	view = ntatable->view;
-
-	nta = isc_mem_get(view->mctx, sizeof(dns__nta_t));
+	nta = isc_mem_get(ntatable->mctx, sizeof(dns__nta_t));
 	*nta = (dns__nta_t){
 		.ntatable = ntatable,
 		.loop = isc_loop_current(ntatable->loopmgr),
 		.magic = NTA_MAGIC,
 	};
 
-	isc_mem_attach(view->mctx, &nta->mctx);
+	isc_mem_attach(ntatable->mctx, &nta->mctx);
 
 	dns_rdataset_init(&nta->rdataset);
 	dns_rdataset_init(&nta->sigrdataset);
@@ -314,12 +322,12 @@ dns_ntatable_add(dns_ntatable_t *ntatable, const dns_name_t *name, bool force,
 
 	REQUIRE(VALID_NTATABLE(ntatable));
 
-	RWLOCK(&ntatable->rwlock, isc_rwlocktype_write);
-
-	if (ntatable->shuttingdown) {
-		goto unlock;
+	if (atomic_load(&ntatable->shuttingdown)) {
+		return (ISC_R_SUCCESS);
 	}
 
+	RWLOCK(&ntatable->rwlock, isc_rwlocktype_write);
+
 	nta_create(ntatable, name, &nta);
 
 	nta->expiry = now + lifetime;
@@ -350,7 +358,6 @@ dns_ntatable_add(dns_ntatable_t *ntatable, const dns_name_t *name, bool force,
 		break;
 	}
 
-unlock:
 	RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_write);
 
 	return (result);
@@ -683,5 +690,7 @@ dns_ntatable_shutdown(dns_ntatable_t *ntatable) {
 	}
 
 	dns_rbtnodechain_invalidate(&chain);
+
+	dns_view_weakdetach(&ntatable->view);
 	RWUNLOCK(&ntatable->rwlock, isc_rwlocktype_write);
 }

lib/dns/resolver.c

@@ -240,11 +240,6 @@ STATIC_ASSERT(NS_PROCESSING_LIMIT > NS_RR_LIMIT,
  */
 #define RECV_BUFFER_SIZE 4096 /* XXXRTH  Constant. */
 
-/*%
- * Default EDNS0 buffer size
- */
-#define DEFAULT_EDNS_BUFSIZE 1232
-
 /*%
  * This defines the maximum number of timeouts we will permit before we
  * disable EDNS0 on the query.
@@ -561,7 +556,6 @@ struct dns_resolver {
 	isc_task_t **tasks;
 	uint32_t lame_ttl;
 	ISC_LIST(alternate_t) alternates;
-	uint16_t udpsize;
 	dns_rbt_t *algorithms;
 	dns_rbt_t *digests;
 	dns_rbt_t *mustbesecure;
@@ -2661,7 +2655,7 @@ resquery_send(resquery_t *query) {
 			 * Set the default UDP size to what was
 			 * configured as 'edns-buffer-size'
 			 */
-			udpsize = res->udpsize;
+			udpsize = res->view->udpsize;
 
 			/*
 			 * This server timed out for the first time in
@@ -10274,7 +10268,6 @@ dns_resolver_create(dns_view_t *view, isc_loopmgr_t *loopmgr,
 		.taskmgr = taskmgr,
 		.dispatchmgr = dispatchmgr,
 		.options = options,
-		.udpsize = DEFAULT_EDNS_BUFSIZE,
 		.spillatmin = 10,
 		.spillat = 10,
 		.spillatmax = 100,
@@ -11040,18 +11033,6 @@ dns_resolver_addalternate(dns_resolver_t *resolver, const isc_sockaddr_t *alt,
 	ISC_LIST_APPEND(resolver->alternates, a, link);
 }
 
-void
-dns_resolver_setudpsize(dns_resolver_t *resolver, uint16_t udpsize) {
-	REQUIRE(VALID_RESOLVER(resolver));
-	resolver->udpsize = udpsize;
-}
-
-uint16_t
-dns_resolver_getudpsize(dns_resolver_t *resolver) {
-	REQUIRE(VALID_RESOLVER(resolver));
-	return (resolver->udpsize);
-}
-
 void
 dns_resolver_flushbadcache(dns_resolver_t *resolver, const dns_name_t *name) {
 	if (name != NULL) {

lib/dns/view.c

@@ -75,6 +75,11 @@
 #define DNS_VIEW_DELONLYHASH   111
 #define DNS_VIEW_FAILCACHESIZE 1021
 
+/*%
+ * Default EDNS0 buffer size
+ */
+#define DEFAULT_EDNS_BUFSIZE 1232
+
 isc_result_t
 dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
 		dns_view_t **viewp) {
@@ -110,6 +115,7 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
 		.synthfromdnssec = true,
 		.trust_anchor_telemetry = true,
 		.root_key_sentinel = true,
+		.udpsize = DEFAULT_EDNS_BUFSIZE,
 	};
 
 	isc_refcount_init(&view->references, 1);
@@ -507,23 +513,46 @@ dns_view_detach(dns_view_t **viewp) {
 	if (isc_refcount_decrement(&view->references) == 1) {
 		dns_zone_t *mkzone = NULL, *rdzone = NULL;
 		dns_zt_t *zt = NULL;
+		dns_resolver_t *resolver = NULL;
+		dns_adb_t *adb = NULL;
+		dns_requestmgr_t *requestmgr = NULL;
 
 		isc_refcount_destroy(&view->references);
 
+		/* Swap the pointers under the lock */
+		LOCK(&view->lock);
 		if (view->resolver != NULL) {
-			dns_resolver_shutdown(view->resolver);
-			dns_resolver_detach(&view->resolver);
-		}
-		if (view->adb != NULL) {
-			dns_adb_shutdown(view->adb);
-			dns_adb_detach(&view->adb);
-		}
-		if (view->requestmgr != NULL) {
-			dns_requestmgr_shutdown(view->requestmgr);
-			dns_requestmgr_detach(&view->requestmgr);
+			resolver = view->resolver;
+			view->resolver = NULL;
+			UNLOCK(&view->lock);
+
+			dns_resolver_shutdown(resolver);
+			dns_resolver_detach(&resolver);
+
+			LOCK(&view->lock);
 		}
 
-		LOCK(&view->lock);
+		if (view->adb != NULL) {
+			adb = view->adb;
+			view->adb = NULL;
+			UNLOCK(&view->lock);
+
+			dns_adb_shutdown(adb);
+			dns_adb_detach(&adb);
+
+			LOCK(&view->lock);
+		}
+
+		if (view->requestmgr != NULL) {
+			requestmgr = view->requestmgr;
+			view->requestmgr = NULL;
+			UNLOCK(&view->lock);
+
+			dns_requestmgr_shutdown(requestmgr);
+			dns_requestmgr_detach(&requestmgr);
+
+			LOCK(&view->lock);
+		}
 
 		if (view->zonetable != NULL) {
 			zt = view->zonetable;
@@ -2396,3 +2425,31 @@ dns_view_sfd_find(dns_view_t *view, const dns_name_t *name,
 		dns_name_copy(dns_rootname, foundname);
 	}
 }
+
+isc_result_t
+dns_view_getresolver(dns_view_t *view, dns_resolver_t **resolverp) {
+	isc_result_t result;
+	REQUIRE(DNS_VIEW_VALID(view));
+	REQUIRE(resolverp != NULL && *resolverp == NULL);
+	LOCK(&view->lock);
+	if (view->resolver != NULL) {
+		dns_resolver_attach(view->resolver, resolverp);
+		result = ISC_R_SUCCESS;
+	} else {
+		result = ISC_R_SHUTTINGDOWN;
+	}
+	UNLOCK(&view->lock);
+	return (result);
+}
+
+void
+dns_view_setudpsize(dns_view_t *view, uint16_t udpsize) {
+	REQUIRE(DNS_VIEW_VALID(view));
+	view->udpsize = udpsize;
+}
+
+uint16_t
+dns_view_getudpsize(dns_view_t *view) {
+	REQUIRE(DNS_VIEW_VALID(view));
+	return (view->udpsize);
+}

lib/dns/zone.c

@@ -11298,16 +11298,27 @@ zone_refreshkeys(dns_zone_t *zone) {
 #ifdef ENABLE_AFL
 		if (!dns_fuzzing_resolver) {
 #endif /* ifdef ENABLE_AFL */
+			/*
+			 * We need to unlock and lock zone here because view
+			 * gets locked in the dns_resolver_createfetch() via
+			 * dns_view_findzonecut() and this violates the locking
+			 * hierarchy (view -> zone).
+			 */
 			UNLOCK_ZONE(zone);
-			result = dns_resolver_createfetch(
-				zone->view->resolver, kname,
-				dns_rdatatype_dnskey, NULL, NULL, NULL, NULL, 0,
-				DNS_FETCHOPT_NOVALIDATE |
-					DNS_FETCHOPT_UNSHARED |
-					DNS_FETCHOPT_NOCACHED,
-				0, NULL, zone->task, keyfetch_done, kfetch,
-				&kfetch->dnskeyset, &kfetch->dnskeysigset,
-				&kfetch->fetch);
+			dns_resolver_t *resolver = NULL;
+			result = dns_view_getresolver(zone->view, &resolver);
+			if (result == ISC_R_SUCCESS) {
+				result = dns_resolver_createfetch(
+					resolver, kname, dns_rdatatype_dnskey,
+					NULL, NULL, NULL, NULL, 0,
+					DNS_FETCHOPT_NOVALIDATE |
+						DNS_FETCHOPT_UNSHARED |
+						DNS_FETCHOPT_NOCACHED,
+					0, NULL, zone->task, keyfetch_done,
+					kfetch, &kfetch->dnskeyset,
+					&kfetch->dnskeysigset, &kfetch->fetch);
+				dns_resolver_detach(&resolver);
+			}
 			LOCK_ZONE(zone);
 #ifdef ENABLE_AFL
 		} else {
@@ -14662,10 +14673,7 @@ again:
 			if (dscp != -1) {
 				have_xfrdscp = true;
 			}
-			if (zone->view->resolver != NULL) {
-				udpsize = dns_resolver_getudpsize(
-					zone->view->resolver);
-			}
+			udpsize = dns_view_getudpsize(zone->view);
 			(void)dns_peer_getudpsize(peer, &udpsize);
 			(void)dns_peer_getrequestnsid(peer, &reqnsid);
 			(void)dns_peer_getrequestexpire(peer, &reqexpire);
@@ -14961,10 +14969,7 @@ ns_query(dns_zone_t *zone, dns_rdataset_t *soardataset, dns_stub_t *stub) {
 			if (result == ISC_R_SUCCESS && dscp != -1) {
 				have_xfrdscp = true;
 			}
-			if (zone->view->resolver != NULL) {
-				udpsize = dns_resolver_getudpsize(
-					zone->view->resolver);
-			}
+			udpsize = dns_view_getudpsize(zone->view);
 			(void)dns_peer_getudpsize(peer, &udpsize);
 			(void)dns_peer_getrequestnsid(peer, &reqnsid);
 		}

lib/ns/client.c

@@ -963,7 +963,6 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message,
 	unsigned char cookie[COOKIE_SIZE];
 	isc_result_t result;
 	dns_view_t *view = NULL;
-	dns_resolver_t *resolver = NULL;
 	uint16_t udpsize;
 	dns_ednsopt_t ednsopts[DNS_EDNSOPTIONS];
 	int count = 0;
@@ -978,9 +977,8 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message,
 
 	env = client->manager->aclenv;
 	view = client->view;
-	resolver = (view != NULL) ? view->resolver : NULL;
-	if (resolver != NULL) {
-		udpsize = dns_resolver_getudpsize(resolver);
+	if (view != NULL) {
+		udpsize = dns_view_getudpsize(view);
 	} else {
 		udpsize = client->manager->sctx->udpsize;
 	}

tests/dns/keytable_test.c

@@ -211,6 +211,7 @@ create_tables(void) {
 static void
 destroy_tables(void) {
 	if (ntatable != NULL) {
+		dns_ntatable_shutdown(ntatable);
 		dns_ntatable_detach(&ntatable);
 	}
 	if (keytable != NULL) {