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 f8b7b95d9fc5f18474f6377ef8b9df19e72b25c5..b6fd4215c587205ac4bc43cd466321c9d649eb67 100644 (file)
--- 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