From: Ondřej Surý <ondrej@isc.org>
Date: Thu, 11 Sep 2025 08:53:47 +0000 (+0200)
Subject: Fix datarace between unlocking fctx lock and shuttingdown fctx
X-Git-Tag: v9.21.14~33^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=09762bdc44657eba11c51c4550f0bb1144740a88;p=thirdparty%2Fbind9.git

Fix datarace between unlocking fctx lock and shuttingdown fctx

There was a data race where new fetch response could be added to the
fetch context after we unlock the fetch context and before we shut it
down.  This could cause assertion failure when fctx__done() was called
with ISC_R_SUCCESS because there was originally no fetch response, but
new fetch response without associated dataset was added before we had a
chance to shutdown the fetch context.  This manifested in the
validated() callback, where cache_rrset() now returns ISC_R_SUCCESS
instead of DNS_R_UNCHANGED when cache was not changed.  However the data
race was wrong on a general level.

When the fctx__done() is called with ISC_R_SUCCESS as result is expects
the fctx->lock to be already acquired to prevent these data races.
---

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index ef848ef3840..fe0c36fd081 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -1664,7 +1664,9 @@ fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func,
 	UNUSED(func);
 #endif
 
-	LOCK(&fctx->lock);
+	if (result != ISC_R_SUCCESS) {
+		LOCK(&fctx->lock);
+	}
 	/* We need to do this under the lock for intra-thread synchronization */
 	if (fctx->state == fetchstate_done) {
 		UNLOCK(&fctx->lock);
@@ -2899,11 +2901,8 @@ fctx_finddone(void *arg) {
 			}
 		}
 	}
-
 	UNLOCK(&fctx->lock);
 
-	dns_adb_destroyfind(&find);
-
 	if (want_done) {
 		FCTXTRACE("fetch failed in finddone(); return "
 			  "ISC_R_FAILURE");
@@ -2913,6 +2912,8 @@ fctx_finddone(void *arg) {
 		fctx_try(fctx, true);
 	}
 
+	dns_adb_destroyfind(&find);
+
 	fetchctx_detach(&fctx);
 }
 
@@ -4108,7 +4109,6 @@ resume_qmin(void *arg) {
 	dns_rdataset_t sigrdataset;
 	dns_db_t *db = NULL;
 	dns_dbnode_t *node = NULL;
-	bool fixup_result = false;
 
 	REQUIRE(VALID_FCTX(fctx));
 
@@ -4265,14 +4265,16 @@ resume_qmin(void *arg) {
 					dns_db_attachnode(node, &resp->node);
 				}
 				dns_name_copy(fname, resp->foundname);
+				clone_results(fctx);
 				if (result == DNS_R_CNAME &&
 				    dns_rdataset_isassociated(&rdataset) &&
 				    fctx->type == dns_rdatatype_cname)
 				{
-					fixup_result = true;
+					fctx_success_unref(fctx);
+					result = ISC_R_SUCCESS;
+				} else {
+					UNLOCK(&fctx->lock);
 				}
-				clone_results(fctx);
-				UNLOCK(&fctx->lock);
 				goto cleanup;
 			}
 			UNLOCK(&fctx->lock);
@@ -4370,7 +4372,7 @@ cleanup:
 	}
 	if (result != ISC_R_SUCCESS) {
 		/* An error occurred, tear down whole fctx */
-		fctx_failure_unref(fctx, fixup_result ? ISC_R_SUCCESS : result);
+		fctx_failure_unref(fctx, result);
 	}
 	fetchctx_detach(&fctx);
 }
@@ -5603,9 +5605,19 @@ answer_response:
 	done = true;
 
 cleanup:
-	UNLOCK(&fctx->lock);
-cleanup_unlocked:
+	if (!done || result != ISC_R_SUCCESS) {
+		UNLOCK(&fctx->lock);
+	}
+
+	if (done) {
+		if (result == ISC_R_SUCCESS) {
+			fctx_success_unref(fctx);
+		} else {
+			fctx_failure_unref(fctx, result);
+		}
+	}
 
+cleanup_unlocked:
 	if (node != NULL) {
 		dns_db_detachnode(&node);
 	}
@@ -5614,12 +5626,6 @@ cleanup_unlocked:
 		dns_validator_send(nextval);
 	}
 
-	if (done && result == ISC_R_SUCCESS) {
-		fctx_success_unref(fctx);
-	} else if (done) {
-		fctx_failure_unref(fctx, result);
-	}
-
 	/*
 	 * val->name points to name on a message on one of the
 	 * queries on the fetch context so the name has to be
@@ -5628,7 +5634,6 @@ cleanup_unlocked:
 	dns_validator_shutdown(val);
 	dns_validator_detach(&val);
 	fetchctx_detach(&fctx);
-	INSIST(node == NULL);
 }
 
 static void
@@ -9204,15 +9209,18 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
 		rctx->next_server = false;
 		rctx->resend = false;
 	}
-	UNLOCK(&fctx->lock);
 
 	if (rctx->next_server) {
+		UNLOCK(&fctx->lock);
 		rctx_nextserver(rctx, message, addrinfo, result);
 	} else if (rctx->resend) {
+		UNLOCK(&fctx->lock);
 		rctx_resend(rctx, addrinfo);
 	} else if (result == DNS_R_CHASEDSSERVERS) {
+		UNLOCK(&fctx->lock);
 		rctx_chaseds(rctx, message, addrinfo, result);
 	} else if (result == ISC_R_SUCCESS && !HAVE_ANSWER(fctx)) {
+		UNLOCK(&fctx->lock);
 		/*
 		 * All has gone well so far, but we are waiting for the DNSSEC
 		 * validator to validate the answer.
@@ -9220,10 +9228,11 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
 		FCTXTRACE("wait for validator");
 		fctx_cancelqueries(fctx, true, false);
 	} else if (result == ISC_R_SUCCESS) {
-		/* We are done. */
+		/* Ended successfully. */
 		fctx_success_detach(&rctx->fctx);
 	} else {
-		/* Done with an error. */
+		UNLOCK(&fctx->lock);
+		/* Ended with failure. */
 		fctx_failure_detach(&rctx->fctx, result);
 	}