Merge branch '3643-dont-use-dns_zone_attach-in-zone_refreshkeys' into 'main'

Don't use dns_zone_attach() in zone_refreshkeys()

Closes #3643

See merge request isc-projects/bind9!7022

lib/dns/zone.c

@@ -812,6 +812,7 @@ struct dns_keymgmt {
  */
 
 struct dns_keyfetch {
+	isc_mem_t *mctx;
 	dns_fixedname_t name;
 	dns_rdataset_t keydataset;
 	dns_rdataset_t dnskeyset;
@@ -5797,8 +5798,11 @@ dns_zone_detach(dns_zone_t **zonep) {
 		isc_refcount_destroy(&zone->erefs);
 
 		/*
-		 * We just detached the last external reference.
+		 * Stop things being restarted after we cancel them below.
 		 */
+		DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_EXITING);
+		dns_zone_log(zone, ISC_LOG_INFO, "final reference detached");
+
 		if (zone->loop != NULL) {
 			/*
 			 * This zone has a loop; it can clean
@@ -5810,24 +5814,18 @@ dns_zone_detach(dns_zone_t **zonep) {
 
 		/*
 		 * This zone is unmanaged; we're probably running in
-		 * named-checkzone or a unit test. There's no loop, so we need
+		 * named-checkzone or a unit test. There's no loop, so we
-		 * to free it immediately.
+		 * need to free it immediately.
 		 *
-		 * Unmanaged zones must not have null views;
+		 * Unmanaged zones must not have null views; we have no way
-		 * we have no way of detaching from the view here
+		 * of detaching from the view here without causing deadlock
-		 * without causing deadlock because this code is
+		 * because this code is called with the view already
-		 * called with the view already locked.
+		 * locked.
 		 */
 		INSIST(isc_tid() == ISC_TID_UNKNOWN);
 		INSIST(zone->view == NULL);
 
-		if (zone->raw != NULL) {
+		zone_shutdown(zone);
-			dns_zone_detach(&zone->raw);
-		}
-		if (zone->secure != NULL) {
-			dns_zone_idetach(&zone->secure);
-		}
-		zone_free(zone);
 	}
 }
 
@@ -5872,7 +5870,6 @@ zone_idetach(dns_zone_t **zonep) {
 void
 dns_zone_idetach(dns_zone_t **zonep) {
 	dns_zone_t *zone;
-	bool free_needed;
 
 	REQUIRE(zonep != NULL && DNS_ZONE_VALID(*zonep));
 
@@ -5880,6 +5877,7 @@ dns_zone_idetach(dns_zone_t **zonep) {
 	*zonep = NULL;
 
 	if (isc_refcount_decrement(&zone->irefs) == 1) {
+		bool free_needed;
 		LOCK_ZONE(zone);
 		free_needed = exit_check(zone);
 		UNLOCK_ZONE(zone);
@@ -10552,7 +10550,7 @@ keyfetch_done(isc_task_t *task, isc_event_t *event) {
 
 	kfetch = event->ev_arg;
 	zone = kfetch->zone;
-	isc_mem_attach(zone->mctx, &mctx);
+	mctx = kfetch->mctx;
 	keyname = dns_fixedname_name(&kfetch->name);
 	dnskeys = &kfetch->dnskeyset;
 	dnskeysigs = &kfetch->dnskeysigset;
@@ -11145,11 +11143,9 @@ failure:
 cleanup:
 	dns_db_detach(&kfetch->db);
 
-	isc_refcount_decrement(&zone->irefs);
-
 	/* The zone must be managed */
 	INSIST(kfetch->zone->loop != NULL);
-	dns_zone_detach(&kfetch->zone);
+	isc_refcount_decrement(&zone->irefs);
 
 	if (dns_rdataset_isassociated(keydataset)) {
 		dns_rdataset_disassociate(keydataset);
@@ -11162,7 +11158,7 @@ cleanup:
 	}
 
 	dns_name_free(keyname, mctx);
-	isc_mem_putanddetach(&mctx, kfetch, sizeof(dns_keyfetch_t));
+	isc_mem_putanddetach(&kfetch->mctx, kfetch, sizeof(dns_keyfetch_t));
 
 	if (secroots != NULL) {
 		dns_keytable_detach(&secroots);
@@ -11181,30 +11177,42 @@ cleanup:
 static void
 retry_keyfetch(dns_keyfetch_t *kfetch, dns_name_t *kname) {
 	isc_time_t timenow, timethen;
-	char timebuf[80];
 	dns_zone_t *zone = kfetch->zone;
+	bool free_needed;
 
 	/*
 	 * Error during a key fetch; cancel and retry in an hour.
 	 */
+	LOCK_ZONE(zone);
 	zone->refreshkeycount--;
 	isc_refcount_decrement(&zone->irefs);
 	dns_db_detach(&kfetch->db);
 	dns_rdataset_disassociate(&kfetch->keydataset);
 	dns_name_free(kname, zone->mctx);
-	isc_mem_put(zone->mctx, kfetch, sizeof(dns_keyfetch_t));
+	isc_mem_putanddetach(&kfetch->mctx, kfetch, sizeof(*kfetch));
 	dnssec_log(zone, ISC_LOG_WARNING,
 		   "Failed to create fetch for DNSKEY update");
 
-	TIME_NOW(&timenow);
+	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) {
-	DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen);
+		/* Don't really retry if we are exiting */
-	zone->refreshkeytime = timethen;
+		char timebuf[80];
-	zone_settimer(zone, &timenow);
 
-	isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80);
+		TIME_NOW(&timenow);
-	dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s", timebuf);
+		DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen);
+		zone->refreshkeytime = timethen;
+		zone_settimer(zone, &timenow);
 
-	dns_zone_detach(&zone);
+		isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80);
+		dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s",
+			   timebuf);
+	}
+
+	free_needed = exit_check(zone);
+	UNLOCK_ZONE(zone);
+
+	if (free_needed) {
+		zone_free(zone);
+	}
 }
 
 static void
@@ -11217,11 +11225,24 @@ do_keyfetch(void *arg) {
 	unsigned int options = DNS_FETCHOPT_NOVALIDATE | DNS_FETCHOPT_UNSHARED |
 			       DNS_FETCHOPT_NOCACHED;
 
+	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) {
+		goto retry;
+	}
+
 	result = dns_view_getresolver(zone->view, &resolver);
 	if (result != ISC_R_SUCCESS) {
 		goto retry;
 	}
 
+	/*
+	 * Use of DNS_FETCHOPT_NOCACHED is essential here.  If it is not
+	 * set and the cache still holds a non-expired, validated version
+	 * of the RRset being queried for by the time the response is
+	 * received, the cached RRset will be passed to keyfetch_done()
+	 * instead of the one received in the response as the latter will
+	 * have a lower trust level due to not being validated until
+	 * keyfetch_done() is called.
+	 */
 	result = dns_resolver_createfetch(
 		resolver, kname, dns_rdatatype_dnskey, NULL, NULL, NULL, NULL,
 		0, options, 0, NULL, zone->task, keyfetch_done, kfetch,
@@ -11284,7 +11305,6 @@ zone_refreshkeys(dns_zone_t *zone) {
 		isc_stdtime_t timer = 0xffffffff;
 		dns_name_t *name = NULL, *kname = NULL;
 		dns_rdataset_t *kdset = NULL;
-		dns_keyfetch_t *kfetch;
 		uint32_t ttl;
 
 		dns_rriterator_current(&rrit, &name, &ttl, &kdset, NULL);
@@ -11336,55 +11356,39 @@ zone_refreshkeys(dns_zone_t *zone) {
 
 		dns_rriterator_pause(&rrit);
 
-		kfetch = isc_mem_get(zone->mctx, sizeof(dns_keyfetch_t));
-		*kfetch = (dns_keyfetch_t){ .zone = NULL };
-
-		zone->refreshkeycount++;
-		dns_zone_attach(zone, &kfetch->zone);
-		isc_refcount_increment0(&zone->irefs);
-		kname = dns_fixedname_initname(&kfetch->name);
-		dns_name_dup(name, zone->mctx, kname);
-		dns_rdataset_init(&kfetch->dnskeyset);
-		dns_rdataset_init(&kfetch->dnskeysigset);
-		dns_rdataset_init(&kfetch->keydataset);
-		dns_rdataset_clone(kdset, &kfetch->keydataset);
-		kfetch->db = NULL;
-		dns_db_attach(db, &kfetch->db);
-		kfetch->fetch = NULL;
-
-		if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) {
-			char namebuf[DNS_NAME_FORMATSIZE];
-			dns_name_format(kname, namebuf, sizeof(namebuf));
-			dnssec_log(zone, ISC_LOG_DEBUG(3),
-				   "Creating key fetch in "
-				   "zone_refreshkeys() for '%s'",
-				   namebuf);
-		}
-
-		/*
-		 * Use of DNS_FETCHOPT_NOCACHED is essential here.  If it is
-		 * not set and the cache still holds a non-expired, validated
-		 * version of the RRset being queried for by the time the
-		 * response is received, the cached RRset will be passed to
-		 * keyfetch_done() instead of the one received in the response
-		 * as the latter will have a lower trust level due to not being
-		 * validated until keyfetch_done() is called.
-		 */
-
 #ifdef ENABLE_AFL
 		if (!dns_fuzzing_resolver) {
 #endif /* ifdef ENABLE_AFL */
+			dns_keyfetch_t *kfetch = NULL;
+
+			kfetch = isc_mem_get(zone->mctx,
+					     sizeof(dns_keyfetch_t));
+			*kfetch = (dns_keyfetch_t){ .zone = zone };
+			isc_mem_attach(zone->mctx, &kfetch->mctx);
+
+			zone->refreshkeycount++;
+			isc_refcount_increment0(&zone->irefs);
+			kname = dns_fixedname_initname(&kfetch->name);
+			dns_name_dup(name, zone->mctx, kname);
+			dns_rdataset_init(&kfetch->dnskeyset);
+			dns_rdataset_init(&kfetch->dnskeysigset);
+			dns_rdataset_init(&kfetch->keydataset);
+			dns_rdataset_clone(kdset, &kfetch->keydataset);
+			dns_db_attach(db, &kfetch->db);
+
+			if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) {
+				char namebuf[DNS_NAME_FORMATSIZE];
+				dns_name_format(kname, namebuf,
+						sizeof(namebuf));
+				dnssec_log(zone, ISC_LOG_DEBUG(3),
+					   "Creating key fetch in "
+					   "zone_refreshkeys() for '%s'",
+					   namebuf);
+			}
+
 			isc_async_run(zone->loop, do_keyfetch, kfetch);
 			fetching = true;
 #ifdef ENABLE_AFL
-		} else {
-			zone->refreshkeycount--;
-			isc_refcount_decrement(&zone->irefs);
-			dns_db_detach(&kfetch->db);
-			dns_rdataset_disassociate(&kfetch->keydataset);
-			dns_name_free(kname, zone->mctx);
-			dns_zone_detach(&kfetch->zone);
-			isc_mem_put(zone->mctx, kfetch, sizeof(dns_keyfetch_t));
 		}
 #endif /* ifdef ENABLE_AFL */
 	}
@@ -11422,34 +11426,24 @@ static void
 zone_maintenance(dns_zone_t *zone) {
 	isc_time_t now;
 	isc_result_t result;
-	bool dumping, load_pending, viewok, notify, refreshkeys;
+	bool load_pending, exiting, dumping, viewok, notify;
-	bool sign, resign, rekey, chain, warn_expire;
+	bool refreshkeys, sign, resign, rekey, chain, warn_expire;
 
 	REQUIRE(DNS_ZONE_VALID(zone));
 	ENTER;
 
 	/*
-	 * Are we pending load/reload?
+	 * Are we pending load/reload, exiting, or unconfigured
+	 * (e.g. because of a syntax failure in the config file)?
+	 * If so, don't attempt maintenance.
 	 */
 	LOCK_ZONE(zone);
 	load_pending = DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADPENDING);
-	UNLOCK_ZONE(zone);
+	exiting = DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING);
-
-	if (load_pending) {
-		return;
-	}
-
-	/*
-	 * Configuring the view of this zone may have
-	 * failed, for example because the config file
-	 * had a syntax error.	In that case, the view
-	 * adb or resolver will be NULL, and we had better not try
-	 * to do further maintenance on it.
-	 */
-	LOCK_ZONE(zone);
 	viewok = (zone->view != NULL && zone->view->adb != NULL);
 	UNLOCK_ZONE(zone);
-	if (!viewok) {
+
+	if (load_pending || exiting || !viewok) {
 		return;
 	}
 
@@ -15117,8 +15111,7 @@ unlock:
 }
 
 /*
- * Handle the control event.  Note that although this event causes the zone
+ * Shut the zone down.
- * to shut down, it is not a shutdown event in the sense of the task library.
  */
 static void
 zone_shutdown(void *arg) {
@@ -15131,13 +15124,6 @@ zone_shutdown(void *arg) {
 
 	zone_debuglog(zone, __func__, 3, "shutting down");
 
-	/*
-	 * Stop things being restarted after we cancel them below.
-	 */
-	LOCK_ZONE(zone);
-	DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_EXITING);
-	UNLOCK_ZONE(zone);
-
 	/*
 	 * If we were waiting for xfrin quota, step out of
 	 * the queue.
@@ -15285,7 +15271,6 @@ zone_timer_set(dns_zone_t *zone, isc_time_t *next, isc_time_t *now) {
 static void
 zone__settimer(void *arg) {
 	zone_settimer_t *data = arg;
-
 	dns_zone_t *zone = data->zone;
 	isc_time_t *now = &data->now;
 	isc_time_t next;
@@ -15443,6 +15428,10 @@ free:
 
 static void
 zone_settimer(dns_zone_t *zone, isc_time_t *now) {
+	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) {
+		return;
+	}
+
 	zone_settimer_t *arg = isc_mem_get(zone->mctx, sizeof(*arg));
 	*arg = (zone_settimer_t){
 		.zone = zone,