tree b8552da53ab6c19264b6a851915c436fce67e474
parent 261b916ab5db387bedc5ae1854514c2be4f42888
author Willy Tarreau <w@1wt.eu> 1700214993 +0100
committer Christopher Faulet <cfaulet@haproxy.com> 1702542497 +0100

BUG/MEDIUM: mux-fcgi: fail earlier on malloc in takeover()

This is the equivalent of the previous "BUG/MEDIUM: mux-h1: fail earlier
on malloc in takeover()".

Connection takeover was implemented for fcgi in 2.2 by commit a41bb0b6c
("MEDIUM: mux_fcgi: Implement the takeover() method."). It does have one
corner case related to memory allocation failure: in case the task or
tasklet allocation fails, the connection gets released synchronously.

Unfortunately the situation is bad there, because the lower layers are
already switched to the new thread while the tasklet is either NULL or
still the old one, and calling fcgi_release() will also result in
touching the thread-local list of buffer waiters, calling unsubscribe(),
There are even code paths where the thread will try to grab the lock of
its own idle conns list, believing the connection is there while it has
no useful effect. However, if the owner thread was doing the same at the
same moment, and ended up trying to pick from the current thread (which
could happen if picking a connection for a different name), the two
could even deadlock.

No tests were made to try to reproduce the problem, but the description
above is sufficient to see that nothing can guarantee against it.

This patch takes a simple but radically different approach. Instead of
starting to migrate the connection before risking to face allocation
failures, it first pre-allocates a new task and tasklet, then assigns
them to the connection if the migration succeeds, otherwise it just
frees them. This way it's no longer needed to manipulate the connection
until it's fully migrated, and as a bonus this means the connection will
continue to exist and the use-after-free condition is solved at the same
time.

This should be backported to 2.2. Thanks to Fred for the initial analysis
of the problem!

(cherry picked from commit d069825c5f59a22ec14577cfbd3bce4af0968ee7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit e0c6ef4038b51bf172a8b8518f9080fc0f09dac9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 2311379ec13ad43963bdf001f21a309e98a49032)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit fe567d871b0bd61caf7457fe31ede8b93c434f68)
[cf: task_new(tid_bit) is used instead of task_new_here()]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>