From: Ondřej Surý <ondrej@isc.org>
Date: Thu, 30 Apr 2026 05:39:23 +0000 (+0200)
Subject: Dispatch ratelimiter events under the lock
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4d465f4fa58abb79b90d9019900e7d543fb511ea;p=thirdparty%2Fbind9.git

Dispatch ratelimiter events under the lock

isc__ratelimiter_tick() and isc_ratelimiter_shutdown() each pulled
events out of rl->pending into a function-local list, dropped the
mutex, and then iterated.  ISC_LIST_APPEND leaves the link in the
LINKED state, so a concurrent isc_ratelimiter_dequeue() saw an
event as still queued, called ISC_LIST_UNLINK against rl->pending —
which patched the prev/next of the local list — and freed the
event before dispatch finished, producing either an INSIST in the
unlink macro or a use-after-free in the dispatch loop.

isc_async_run() is a non-blocking wfcq enqueue, so there is no
benefit to dropping the mutex around it.  Unlink each event and
hand it to isc_async_run() while still holding rl->lock; the
existing ISC_LINK_LINKED check in dequeue then correctly
distinguishes "still queued and cancellable" from "already taken".

Assisted-by: Claude:claude-opus-4-7
---

diff --git a/lib/isc/ratelimiter.c b/lib/isc/ratelimiter.c
index f8b7b95d9fc..b6fd4215c58 100644
--- a/lib/isc/ratelimiter.c
+++ b/lib/isc/ratelimiter.c
@@ -219,12 +219,9 @@ static void
 isc__ratelimiter_tick(void *arg) {
 	isc_ratelimiter_t *rl = (isc_ratelimiter_t *)arg;
 	uint32_t pertic;
-	ISC_LIST(isc_rlevent_t) pending;
 
 	REQUIRE(VALID_RATELIMITER(rl));
 
-	ISC_LIST_INIT(pending);
-
 	LOCK(&rl->lock);
 
 	REQUIRE(rl->timer != NULL);
@@ -237,11 +234,7 @@ isc__ratelimiter_tick(void *arg) {
 	pertic = rl->pertic;
 	while (pertic != 0) {
 		isc_rlevent_t *rle = ISC_LIST_HEAD(rl->pending);
-		if (rle != NULL) {
-			/* There is work to do.  Let's do it after unlocking. */
-			ISC_LIST_UNLINK(rl->pending, rle, link);
-			ISC_LIST_APPEND(pending, rle, link);
-		} else {
+		if (rle == NULL) {
 			/*
 			 * We processed all the scheduled work, but there's a
 			 * room for at least one more event (we haven't consumed
@@ -252,6 +245,15 @@ isc__ratelimiter_tick(void *arg) {
 			rl->state = isc_ratelimiter_idle;
 			break;
 		}
+		/*
+		 * Unlink and dispatch under the lock: isc_async_run() is a
+		 * non-blocking enqueue, so this stays cheap, and once the
+		 * link is TOMBSTONEd a concurrent isc_ratelimiter_dequeue()
+		 * sees ISC_LINK_LINKED == false and returns ISC_R_NOTFOUND
+		 * cleanly instead of racing with our dispatch.
+		 */
+		ISC_LIST_UNLINK(rl->pending, rle, link);
+		isc_async_run(rle->loop, rle->cb, rle->arg);
 		pertic--;
 	}
 
@@ -261,11 +263,6 @@ isc__ratelimiter_tick(void *arg) {
 	}
 unlock:
 	UNLOCK(&rl->lock);
-
-	ISC_LIST_FOREACH(pending, rle, link) {
-		ISC_LIST_UNLINK(pending, rle, link);
-		isc_async_run(rle->loop, rle->cb, rle->arg);
-	}
 }
 
 void
@@ -287,26 +284,20 @@ isc__ratelimiter_doshutdown(void *arg) {
 
 void
 isc_ratelimiter_shutdown(isc_ratelimiter_t *restrict rl) {
-	ISC_LIST(isc_rlevent_t) pending;
-
 	REQUIRE(VALID_RATELIMITER(rl));
 
-	ISC_LIST_INIT(pending);
-
 	LOCK(&rl->lock);
 	if (rl->state != isc_ratelimiter_shuttingdown) {
 		rl->state = isc_ratelimiter_shuttingdown;
-		ISC_LIST_MOVE(pending, rl->pending);
+		ISC_LIST_FOREACH(rl->pending, rle, link) {
+			ISC_LIST_UNLINK(rl->pending, rle, link);
+			rle->canceled = true;
+			isc_async_run(rl->loop, rle->cb, rle->arg);
+		}
 		isc_ratelimiter_ref(rl);
 		isc_async_run(rl->loop, isc__ratelimiter_doshutdown, rl);
 	}
 	UNLOCK(&rl->lock);
-
-	ISC_LIST_FOREACH(pending, rle, link) {
-		ISC_LIST_UNLINK(pending, rle, link);
-		rle->canceled = true;
-		isc_async_run(rl->loop, rle->cb, rle->arg);
-	}
 }
 
 static void