Fix the fetch context hash table lock ordering

The order of the fetch context hash table rwlock and the individual
fetch context was reversed when calling the release_fctx() function.
This was causing a problem when iterating the hash table, and thus the
ordering has been corrected in a way that the hash table rwlock is now
always locked on the outside and the fctx lock is the interior lock.

lib/dns/resolver.c

@@ -350,7 +350,6 @@ struct fetchctx {
 	/*% Locked by lock. */
 	isc_mutex_t lock;
 	fetchstate_t state;
-	bool hashed;
 	bool cloned;
 	bool spilled;
 	ISC_LIST(dns_fetchresponse_t) resps;
@@ -588,9 +587,6 @@ struct dns_resolver {
 	/* Locked by primelock. */
 	dns_fetch_t *primefetch;
 
-	/* Atomic. */
-	atomic_uint_fast32_t nfctx;
-
 	uint32_t nloops;
 
 	isc_mempool_t **namepools;
@@ -706,8 +702,6 @@ get_attached_fctx(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name,
 		  dns_rdataset_t *nameservers, const isc_sockaddr_t *client,
 		  unsigned int options, unsigned int depth, isc_counter_t *qc,
 		  isc_counter_t *gqc, fetchctx_t **fctxp, bool *new_fctx);
-static void
-release_fctx(fetchctx_t *fctx);
 
 /*%
  * The structure and functions defined below implement the resolver
@@ -1641,6 +1635,25 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) {
 	}
 }
 
+static uint32_t
+fctx_hash(fetchctx_t *fctx) {
+	isc_hash32_t hash32;
+	isc_hash32_init(&hash32);
+	isc_hash32_hash(&hash32, fctx->name->ndata, fctx->name->length, false);
+	isc_hash32_hash(&hash32, &fctx->options, sizeof(fctx->options), true);
+	isc_hash32_hash(&hash32, &fctx->type, sizeof(fctx->type), true);
+	return isc_hash32_finalize(&hash32);
+}
+
+static bool
+fctx_match(void *node, const void *key) {
+	const fetchctx_t *fctx0 = node;
+	const fetchctx_t *fctx1 = key;
+
+	return fctx0->options == fctx1->options && fctx0->type == fctx1->type &&
+	       dns_name_equal(fctx0->name, fctx1->name);
+}
+
 static bool
 fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func,
 	   const char *file, unsigned int line) {
@@ -1668,11 +1681,15 @@ fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func,
 		return false;
 	}
 	fctx->state = fetchstate_done;
-	release_fctx(fctx);
-
 	FCTX_ATTR_CLR(fctx, FCTX_ATTR_ADDRWAIT);
 	UNLOCK(&fctx->lock);
 
+	/* The fctx will get deleted either here or in get_attached_fctx() */
+	RWLOCK(&fctx->res->fctxs_lock, isc_rwlocktype_write);
+	(void)isc_hashmap_delete(fctx->res->fctxs, fctx_hash(fctx), match_ptr,
+				 fctx);
+	RWUNLOCK(&fctx->res->fctxs_lock, isc_rwlocktype_write);
+
 	if (result == ISC_R_SUCCESS) {
 		if (fctx->qmin_warning != ISC_R_SUCCESS) {
 			isc_log_write(DNS_LOGCATEGORY_LAME_SERVERS,
@@ -4335,7 +4352,6 @@ fctx_destroy(fetchctx_t *fctx) {
 	dns_resolver_t *res = NULL;
 	isc_sockaddr_t *sa = NULL, *next_sa = NULL;
 	struct tried *tried = NULL;
-	uint_fast32_t nfctx;
 
 	REQUIRE(VALID_FCTX(fctx));
 	REQUIRE(ISC_LIST_EMPTY(fctx->resps));
@@ -4354,9 +4370,6 @@ fctx_destroy(fetchctx_t *fctx) {
 
 	dec_stats(res, dns_resstatscounter_nfetch);
 
-	nfctx = atomic_fetch_sub_release(&res->nfctx, 1);
-	INSIST(nfctx > 0);
-
 	/* Free bad */
 	for (sa = ISC_LIST_HEAD(fctx->bad); sa != NULL; sa = next_sa) {
 		next_sa = ISC_LIST_NEXT(sa, link);
@@ -4544,7 +4557,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name,
 	isc_interval_t interval;
 	unsigned int findoptions = 0;
 	char buf[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE + 1];
-	uint_fast32_t nfctx;
 	isc_mem_t *mctx = isc_loop_getmctx(loop);
 	size_t p;
 
@@ -4796,9 +4808,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name,
 		fctx_minimize_qname(fctx);
 	}
 
-	nfctx = atomic_fetch_add_relaxed(&res->nfctx, 1);
-	INSIST(nfctx < UINT32_MAX);
-
 	inc_stats(res, dns_resstatscounter_nfetch);
 
 	isc_timer_create(fctx->loop, fctx_expired, fctx, &fctx->timer);
@@ -7031,43 +7040,6 @@ ISC_REFCOUNT_TRACE_IMPL(fetchctx, fctx_destroy);
 ISC_REFCOUNT_IMPL(fetchctx, fctx_destroy);
 #endif
 
-static uint32_t
-fctx_hash(fetchctx_t *fctx) {
-	isc_hash32_t hash32;
-	isc_hash32_init(&hash32);
-	isc_hash32_hash(&hash32, fctx->name->ndata, fctx->name->length, false);
-	isc_hash32_hash(&hash32, &fctx->options, sizeof(fctx->options), true);
-	isc_hash32_hash(&hash32, &fctx->type, sizeof(fctx->type), true);
-	return isc_hash32_finalize(&hash32);
-}
-
-static bool
-fctx_match(void *node, const void *key) {
-	const fetchctx_t *fctx0 = node;
-	const fetchctx_t *fctx1 = key;
-
-	return fctx0->options == fctx1->options && fctx0->type == fctx1->type &&
-	       dns_name_equal(fctx0->name, fctx1->name);
-}
-
-/* Must be fctx locked */
-static void
-release_fctx(fetchctx_t *fctx) {
-	isc_result_t result;
-	dns_resolver_t *res = fctx->res;
-
-	if (!fctx->hashed) {
-		return;
-	}
-
-	RWLOCK(&res->fctxs_lock, isc_rwlocktype_write);
-	result = isc_hashmap_delete(res->fctxs, fctx_hash(fctx), match_ptr,
-				    fctx);
-	INSIST(result == ISC_R_SUCCESS);
-	fctx->hashed = false;
-	RWUNLOCK(&res->fctxs_lock, isc_rwlocktype_write);
-}
-
 static void
 resume_dslookup(void *arg) {
 	dns_fetchresponse_t *resp = (dns_fetchresponse_t *)arg;
@@ -9939,8 +9911,6 @@ dns_resolver__destroy(dns_resolver_t *res) {
 
 	RTRACE("destroy");
 
-	REQUIRE(atomic_load_acquire(&res->nfctx) == 0);
-
 	res->magic = 0;
 
 	dns_nametree_detach(&res->algorithms);
@@ -10415,7 +10385,8 @@ again:
 		result = fctx_create(res, loop, name, type, domain, nameservers,
 				     client, options, depth, qc, gqc, &fctx);
 		if (result != ISC_R_SUCCESS) {
-			goto unlock;
+			RWUNLOCK(&res->fctxs_lock, locktype);
+			return result;
 		}
 
 		UPGRADELOCK(&res->fctxs_lock, locktype);
@@ -10425,22 +10396,33 @@ again:
 					 fctx, &found);
 		if (result == ISC_R_SUCCESS) {
 			*new_fctx = true;
-			fctx->hashed = true;
 		} else {
-			fctx_done_detach(&fctx, result);
+			/*
+			 * The fctx_done() tries to acquire the fctxs_lock.
+			 * Destroy the newly created fetchctx directly.
+			 */
+			fctx->state = fetchstate_done;
+			isc_timer_destroy(&fctx->timer);
+
+			fetchctx_detach(&fctx);
 			fctx = found;
 			result = ISC_R_SUCCESS;
 		}
-		INSIST(result == ISC_R_SUCCESS);
 		break;
 	default:
 		UNREACHABLE();
 	}
+	INSIST(result == ISC_R_SUCCESS);
 	fetchctx_ref(fctx);
-unlock:
+
-	RWUNLOCK(&res->fctxs_lock, locktype);
+	/*
-	if (result == ISC_R_SUCCESS) {
+	 * We need to lock the fetch context before unlocking the hash table to
+	 * prevent other threads from looking up this thread before it has been
+	 * properly initialized and started.
+	 */
 	LOCK(&fctx->lock);
+	RWUNLOCK(&res->fctxs_lock, locktype);
+
 	if (SHUTTINGDOWN(fctx) || fctx->cloned) {
 		/*
 		 * This is the single place where fctx might get
@@ -10448,15 +10430,22 @@ unlock:
 		 * double check whether fctxs is done (or cloned) and
 		 * help with the release if the fctx has been cloned.
 		 */
-			release_fctx(fctx);
 		UNLOCK(&fctx->lock);
+
+		/* The fctx will get deleted either here or in fctx__done() */
+		RWLOCK(&res->fctxs_lock, isc_rwlocktype_write);
+		(void)isc_hashmap_delete(res->fctxs, fctx_hash(fctx), match_ptr,
+					 fctx);
+		RWUNLOCK(&res->fctxs_lock, isc_rwlocktype_write);
+
 		fetchctx_detach(&fctx);
 		goto again;
 	}
 
-		INSIST(!SHUTTINGDOWN(fctx));
+	/*
+	 * The function returns a locked fetch context,
+	 */
 	*fctxp = fctx;
-	}
 
 	return result;
 }
@@ -10560,7 +10549,7 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 		result = fctx_create(res, loop, name, type, domain, nameservers,
 				     client, options, depth, qc, gqc, &fctx);
 		if (result != ISC_R_SUCCESS) {
-			goto unlock;
+			goto fail;
 		}
 		new_fctx = true;
 	}