Resolve violation of weak referencing dns_view

The dns_view implements weak and strong reference counting.  When strong
reference counting reaches zero, the adb, ntatable and resolver objects
are shut down and detached.

In dns_zone and dns_nta the dns_view was weakly attached, but the
view->resolver reference was accessed directly leading to dereferencing
the NULL pointer.

Add dns_view_getresolver() method which attaches to view->resolver
object under the lock (if it still exists) ensuring the dns_resolver
will be kept referenced until not needed.

lib/dns/include/dns/view.h

@@ -1299,4 +1299,7 @@ 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);
+
 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,
+		resolver, nta->name, dns_rdatatype_nsec, NULL, NULL, NULL, NULL,
-		NULL, 0, DNS_FETCHOPT_NONTA, 0, NULL, ntatable->task,
+		0, DNS_FETCHOPT_NONTA, 0, NULL, ntatable->task, fetch_done, nta,
-		fetch_done, nta, &nta->rdataset, &nta->sigrdataset,
+		&nta->rdataset, &nta->sigrdataset, &nta->fetch);
-		&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(ntatable->mctx, sizeof(dns__nta_t));
-
-	nta = isc_mem_get(view->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 (atomic_load(&ntatable->shuttingdown)) {
-
+		return (ISC_R_SUCCESS);
-	if (ntatable->shuttingdown) {
-		goto unlock;
 	}
 
+	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/view.c

@@ -507,23 +507,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);
+			resolver = view->resolver;
-			dns_resolver_detach(&view->resolver);
+			view->resolver = NULL;
-		}
+			UNLOCK(&view->lock);
-		if (view->adb != NULL) {
+
-			dns_adb_shutdown(view->adb);
+			dns_resolver_shutdown(resolver);
-			dns_adb_detach(&view->adb);
+			dns_resolver_detach(&resolver);
-		}
+
-		if (view->requestmgr != NULL) {
+			LOCK(&view->lock);
-			dns_requestmgr_shutdown(view->requestmgr);
-			dns_requestmgr_detach(&view->requestmgr);
 		}
 
-		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 +2419,19 @@ 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);
+}

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(
+			dns_resolver_t *resolver = NULL;
-				zone->view->resolver, kname,
+			result = dns_view_getresolver(zone->view, &resolver);
-				dns_rdatatype_dnskey, NULL, NULL, NULL, NULL, 0,
+			if (result == ISC_R_SUCCESS) {
-				DNS_FETCHOPT_NOVALIDATE |
+				result = dns_resolver_createfetch(
-					DNS_FETCHOPT_UNSHARED |
+					resolver, kname, dns_rdatatype_dnskey,
-					DNS_FETCHOPT_NOCACHED,
+					NULL, NULL, NULL, NULL, 0,
-				0, NULL, zone->task, keyfetch_done, kfetch,
+					DNS_FETCHOPT_NOVALIDATE |
-				&kfetch->dnskeyset, &kfetch->dnskeysigset,
+						DNS_FETCHOPT_UNSHARED |
-				&kfetch->fetch);
+						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 {
@@ -14649,6 +14660,7 @@ again:
 		result = dns_peerlist_peerbyaddr(zone->view->peers, &primaryip,
 						 &peer);
 		if (result == ISC_R_SUCCESS) {
+			dns_resolver_t *resolver = NULL;
 			result = dns_peer_getsupportedns(peer, &edns);
 			if (result == ISC_R_SUCCESS && !edns) {
 				DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NOEDNS);
@@ -14662,9 +14674,10 @@ again:
 			if (dscp != -1) {
 				have_xfrdscp = true;
 			}
-			if (zone->view->resolver != NULL) {
+			result = dns_view_getresolver(zone->view, &resolver);
-				udpsize = dns_resolver_getudpsize(
+			if (result == ISC_R_SUCCESS) {
-					zone->view->resolver);
+				udpsize = dns_resolver_getudpsize(resolver);
+				dns_resolver_detach(&resolver);
 			}
 			(void)dns_peer_getudpsize(peer, &udpsize);
 			(void)dns_peer_getrequestnsid(peer, &reqnsid);
@@ -14948,6 +14961,7 @@ ns_query(dns_zone_t *zone, dns_rdataset_t *soardataset, dns_stub_t *stub) {
 		result = dns_peerlist_peerbyaddr(zone->view->peers, &primaryip,
 						 &peer);
 		if (result == ISC_R_SUCCESS) {
+			dns_resolver_t *resolver;
 			result = dns_peer_getsupportedns(peer, &edns);
 			if (result == ISC_R_SUCCESS && !edns) {
 				DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NOEDNS);
@@ -14961,9 +14975,10 @@ 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) {
+			result = dns_view_getresolver(zone->view, &resolver);
-				udpsize = dns_resolver_getudpsize(
+			if (result == ISC_R_SUCCESS) {
-					zone->view->resolver);
+				udpsize = dns_resolver_getudpsize(resolver);
+				dns_resolver_detach(&resolver);
 			}
 			(void)dns_peer_getudpsize(peer, &udpsize);
 			(void)dns_peer_getrequestnsid(peer, &reqnsid);

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) {