BUG/MEDIUM: mux-fcgi: fail earlier on malloc in takeover()
authorWilly Tarreau <w@1wt.eu>
Fri, 17 Nov 2023 09:56:33 +0000 (10:56 +0100)
committerWilly Tarreau <w@1wt.eu>
Fri, 17 Nov 2023 17:10:16 +0000 (18:10 +0100)
commitd069825c5f59a22ec14577cfbd3bce4af0968ee7
tree7312997dd0df810177278f222b681d3bf4d5f83a
parent95fd2d68018d749ff0ad92b4b69d4b3fd75a82e4
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!
src/mux_fcgi.c