From: Willy Tarreau <w@1wt.eu>
Date: Tue, 25 Aug 2026 15:53:45 +0000 (+0200)
Subject: BUG/MEDIUM: sink: do not hold the sft lock around ring_dispatch_messages()
X-Git-Tag: v3.5-dev6~74
X-Git-Url: https://git.kaiwu.me/?a=commitdiff_plain;h=d7ad27e4e48e4b28b06e415f12e5330fd12b720d;p=haproxy.git

BUG/MEDIUM: sink: do not hold the sft lock around ring_dispatch_messages()

Attaching a reader to a ring is done under thread_isolate(), which was
added by commit 31b93b40b ("MEDIUM: ring: protect the initialization of
the initial reader offset"). But _sink_forward_io_handler() calls
ring_dispatch_messages() with the sft lock held, where on the very first
call for a given sft, a thread isolates itself while holding that lock!
Needles to say that if a second thread tries to emit a lock during that
tiny time frame, it's a guaranted deadlock!

The lock is not needed around that call: sft->ofs and sft->e_processed
are only ever accessed from this applet, which is bound to a single
thread, and both lists involved are mt_lists that protect themselves.
What the lock really protects is sft->appctx, whose publication by
process_sink_forward() is asynchronous, and that is already dealt with
by sink_forward_session_init(), which runs before the first call to the
I/O handler. So let's simply take the lock after the dispatch. In fact
that lock is a leftover from 2.2's commit 494c505703 ("MEDIUM: ring:
add server statement to forward messages from a ring") which duplicated
the dispatching code inside the function's body, so when this was cleaned
up in 3.0 with commit c262442b1a ("MEDIUM: sink: move the generic ring
forwarder code use ring_dispatch_messages()"), it was not noticed that
this lock could now cause harm.

This should be backported to 3.0.
---

diff --git a/src/sink.c b/src/sink.c
index 97498c51d..cd12cb847 100644
--- a/src/sink.c
+++ b/src/sink.c
@@ -452,13 +452,22 @@ static void _sink_forward_io_handler(struct appctx *appctx,
 		goto out;
 	}
 
-	HA_SPIN_LOCK(SFT_LOCK, &sft->lock);
-	BUG_ON(appctx != sft->appctx);
-
 	MT_LIST_DELETE(&appctx->wait_entry);
 
+	/* The lock must not be held across ring_dispatch_messages(), which may
+	 * isolate the thread when attaching to the ring. A thread spinning on
+	 * a lock never becomes harmless, so isolating while holding this lock
+	 * would deadlock against process_sink_forward(). It is not needed
+	 * either: sft->ofs and sft->e_processed are only ever accessed from
+	 * this applet, which is bound to a single thread, and the lists are
+	 * mt_lists which protect themselves.
+	 */
 	ret = ring_dispatch_messages(ring, appctx, &sft->ofs, &last_ofs, 0,
 	                             msg_handler, '\n', &processed);
+
+	HA_SPIN_LOCK(SFT_LOCK, &sft->lock);
+	BUG_ON(appctx != sft->appctx);
+
 	sft->e_processed += processed;
 
 	/* if server's max-reuse is set (>= 0), destroy the applet once the