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 97498c51d830983db291e99eda04c3ea64bd4f24..cd12cb8472484ef4612ebd2435b081cb7529e5fc 100644 (file)
--- 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