Lock view while accessing its zone table

Commit 308bc46a59 introduced a change to
the view_flushanddetach() function which makes the latter access
view->zonetable without holding view->lock.  As confirmed by TSAN, this
enables races between threads for view->zonetable accesses.

Swap the view->zonetable pointer under view lock and then detach the
local swapped dns_zt_t later when the view lock is already unlocked.

This commit also changes the dns_zt interfaces, so the setting the
zonetable "flush" flag is separate operation to dns_zt_detach,
e.g. instead of doing:

    if (view->flush) {
        dns_zt_flushanddetach(&zt);
    } else {
        dns_zt_detach(&zt);
    }

the code is now:

    if (view->flush) {
        dns_zt_flush(zt);
    }
    dns_zt_detach(&zt);

making the code more consistent with how we handle flushing and
detaching dns_zone_t pointers from the view.

lib/dns/include/dns/zt.h

@@ -117,14 +117,13 @@ dns_zt_detach(dns_zt_t **ztp);
  */
 
 void
-dns_zt_flushanddetach(dns_zt_t **ztp);
+dns_zt_flush(dns_zt_t *ztp);
 /*%<
- * Detach the given zonetable, if the reference count goes to zero the
+ * Schedule flushing of the given zonetable, when reference count goes
- * zonetable will be flushed and then freed.  In either case 'ztp' is
+ * to zero.
- * set to NULL.
  *
  * Requires:
- * \li	'*ztp' to be valid
+ * \li	'ztp' to be valid
  */
 
 void

lib/dns/view.c

@@ -632,6 +632,7 @@ view_flushanddetach(dns_view_t **viewp, bool flush) {
 
 	if (isc_refcount_decrement(&view->references) == 1) {
 		dns_zone_t *mkzone = NULL, *rdzone = NULL;
+		dns_zt_t *zt = NULL;
 
 		isc_refcount_destroy(&view->references);
 
@@ -645,13 +646,16 @@ view_flushanddetach(dns_view_t **viewp, bool flush) {
 			dns_requestmgr_shutdown(view->requestmgr);
 		}
 
-		if (view->zonetable != NULL && view->flush) {
+		LOCK(&view->lock);
-			dns_zt_flushanddetach(&view->zonetable);
+
-		} else if (view->zonetable != NULL) {
+		if (view->zonetable != NULL) {
-			dns_zt_detach(&view->zonetable);
+			zt = view->zonetable;
+			view->zonetable = NULL;
+			if (view->flush) {
+				dns_zt_flush(zt);
+			}
 		}
 
-		LOCK(&view->lock);
 		if (view->managed_keys != NULL) {
 			mkzone = view->managed_keys;
 			view->managed_keys = NULL;
@@ -674,7 +678,11 @@ view_flushanddetach(dns_view_t **viewp, bool flush) {
 		}
 		UNLOCK(&view->lock);
 
-		/* Need to detach zones outside view lock */
+		/* Need to detach zt and zones outside view lock */
+		if (zt != NULL) {
+			dns_zt_detach(&zt);
+		}
+
 		if (mkzone != NULL) {
 			dns_zone_detach(&mkzone);
 		}

lib/dns/zt.c

@@ -225,14 +225,15 @@ zt_destroy(dns_zt_t *zt) {
 	if (atomic_load_acquire(&zt->flush)) {
 		(void)dns_zt_apply(zt, false, NULL, flush, NULL);
 	}
+
 	dns_rbt_destroy(&zt->table);
 	isc_rwlock_destroy(&zt->rwlock);
 	zt->magic = 0;
 	isc_mem_putanddetach(&zt->mctx, zt, sizeof(*zt));
 }
 
-static void
+void
-zt_flushanddetach(dns_zt_t **ztp, bool need_flush) {
+dns_zt_detach(dns_zt_t **ztp) {
 	dns_zt_t *zt;
 
 	REQUIRE(ztp != NULL && VALID_ZT(*ztp));
@@ -240,23 +241,15 @@ zt_flushanddetach(dns_zt_t **ztp, bool need_flush) {
 	zt = *ztp;
 	*ztp = NULL;
 
-	if (need_flush) {
-		atomic_store_release(&zt->flush, true);
-	}
-
 	if (isc_refcount_decrement(&zt->references) == 1) {
 		zt_destroy(zt);
 	}
 }
 
 void
-dns_zt_flushanddetach(dns_zt_t **ztp) {
+dns_zt_flush(dns_zt_t *zt) {
-	zt_flushanddetach(ztp, true);
+	REQUIRE(VALID_ZT(zt));
-}
+	atomic_store_release(&zt->flush, true);
-
-void
-dns_zt_detach(dns_zt_t **ztp) {
-	zt_flushanddetach(ztp, false);
 }
 
 isc_result_t