From 058a2e7d4437f383c5cda3a44921e49ad272f9fb Mon Sep 17 00:00:00 2001
From: Aram Sargsyan <aram@isc.org>
Date: Fri, 10 Jun 2022 14:44:52 +0000
Subject: [PATCH] Fix a race between resolver query timeout and validation

The `resolver.c:validated()` function unlinks the current validator from
the fetch's validators list, which can leave it empty, then unlocks
the bucket lock. If, by a chance, the fetch was timed out just before
the `validated()` call, the final timeout callback running in parallel
with `validated()` can find the fetch context with no active fetches
and with an empty validators list and destroy it, which is unexpected
for the `validated()` function and can lead to a crash.

Increase the fetch context's reference count in the beginning of
`validated()` and decrease it when it finishes its work to avoid the
unexpected destruction of the fetch context.
Conflict: NA
Reference: https://gitlab.isc.org/isc-projects/bind9/-/commit/058a2e7d4437f383c5cda3a44921e49ad272f9fb
---
 lib/dns/resolver.c | 63 +++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index f34b9e318e..e8899d2457 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -5746,12 +5746,13 @@ validated(isc_task_t *task, isc_event_t *event) {
 	dns_rdataset_t *rdataset;
 	dns_rdataset_t *sigrdataset;
 	dns_resolver_t *res;
-	dns_valarg_t *valarg;
+	dns_valarg_t *valarg = event->ev_arg;
 	dns_validatorevent_t *vevent;
 	fetchctx_t *fctx;
 	bool chaining;
 	bool negative;
 	bool sentresponse;
+	bool bucket_empty;
 	isc_result_t eresult = ISC_R_SUCCESS;
 	isc_result_t result = ISC_R_SUCCESS;
 	isc_stdtime_t now;
@@ -5765,14 +5766,15 @@ validated(isc_task_t *task, isc_event_t *event) {
 	UNUSED(task); /* for now */
 
 	REQUIRE(event->ev_type == DNS_EVENT_VALIDATORDONE);
-	valarg = event->ev_arg;
+	REQUIRE(VALID_FCTX(valarg->fctx));
+	REQUIRE(!ISC_LIST_EMPTY(valarg->fctx->validators));
+
 	fctx = valarg->fctx;
+	fctx_increference(fctx);
 	dns_message_attach(valarg->message, &message);
 
-	REQUIRE(VALID_FCTX(fctx));
 	res = fctx->res;
 	addrinfo = valarg->addrinfo;
-	REQUIRE(!ISC_LIST_EMPTY(fctx->validators));
 
 	vevent = (dns_validatorevent_t *)event;
 	fctx->vresult = vevent->result;
@@ -5810,12 +5812,8 @@ validated(isc_task_t *task, isc_event_t *event) {
 	 * so, destroy the fctx.
 	 */
 	if (SHUTTINGDOWN(fctx) && !sentresponse) {
-		bool bucket_empty;
-		bucket_empty = maybe_destroy(fctx, true);
+		maybe_destroy(fctx, true);
 		UNLOCK(&res->buckets[bucketnum].lock);
-		if (bucket_empty) {
-			empty_bucket(res);
-		}
 		goto cleanup_event;
 	}
 
@@ -5877,18 +5875,15 @@ validated(isc_task_t *task, isc_event_t *event) {
 				(void)dns_db_deleterdataset(fctx->cache, node,
 							    NULL, vevent->type,
 							    0);
-			}
-			if (result == ISC_R_SUCCESS &&
-			    vevent->sigrdataset != NULL) {
-				(void)dns_db_deleterdataset(
-					fctx->cache, node, NULL,
-					dns_rdatatype_rrsig, vevent->type);
-			}
-			if (result == ISC_R_SUCCESS) {
+				if (vevent->sigrdataset != NULL) {
+					(void)dns_db_deleterdataset(
+						fctx->cache, node, NULL,
+						dns_rdatatype_rrsig,
+						vevent->type);
+				}
 				dns_db_detachnode(fctx->cache, &node);
 			}
-		}
-		if (fctx->vresult == DNS_R_BROKENCHAIN && !negative) {
+		} else if (!negative) {
 			/*
 			 * Cache the data as pending for later validation.
 			 */
@@ -5901,20 +5896,16 @@ validated(isc_task_t *task, isc_event_t *event) {
 				(void)dns_db_addrdataset(
 					fctx->cache, node, NULL, now,
 					vevent->rdataset, 0, NULL);
-			}
-			if (result == ISC_R_SUCCESS &&
-			    vevent->sigrdataset != NULL) {
-				(void)dns_db_addrdataset(
-					fctx->cache, node, NULL, now,
-					vevent->sigrdataset, 0, NULL);
-			}
-			if (result == ISC_R_SUCCESS) {
+				if (vevent->sigrdataset != NULL) {
+					(void)dns_db_addrdataset(
+						fctx->cache, node, NULL, now,
+						vevent->sigrdataset, 0, NULL);
+				}
 				dns_db_detachnode(fctx->cache, &node);
 			}
 		}
 		result = fctx->vresult;
 		add_bad(fctx, message, addrinfo, result, badns_validation);
-		isc_event_free(&event);
 		UNLOCK(&res->buckets[bucketnum].lock);
 		INSIST(fctx->validator == NULL);
 		fctx->validator = ISC_LIST_HEAD(fctx->validators);
@@ -5942,8 +5933,7 @@ validated(isc_task_t *task, isc_event_t *event) {
 			fctx_try(fctx, true, true); /* Locks bucket. */
 		}
 
-		dns_message_detach(&message);
-		return;
+		goto cleanup_event;
 	}
 
 	if (negative) {
@@ -6057,19 +6047,15 @@ validated(isc_task_t *task, isc_event_t *event) {
 	}
 
 	if (sentresponse) {
-		bool bucket_empty = false;
 		/*
 		 * If we only deferred the destroy because we wanted to cache
 		 * the data, destroy now.
 		 */
 		dns_db_detachnode(fctx->cache, &node);
 		if (SHUTTINGDOWN(fctx)) {
-			bucket_empty = maybe_destroy(fctx, true);
+			maybe_destroy(fctx, true);
 		}
 		UNLOCK(&res->buckets[bucketnum].lock);
-		if (bucket_empty) {
-			empty_bucket(res);
-		}
 		goto cleanup_event;
 	}
 
@@ -6210,6 +6196,13 @@ cleanup_event:
 	INSIST(node == NULL);
 	dns_message_detach(&message);
 	isc_event_free(&event);
+
+	LOCK(&res->buckets[fctx->bucketnum].lock);
+	bucket_empty = fctx_decreference(fctx);
+	UNLOCK(&res->buckets[fctx->bucketnum].lock);
+	if (bucket_empty) {
+		empty_bucket(res);
+	}
 }
 
 static void
-- 
2.23.0