BUG/MEDIUM: mux-h1: fail earlier on malloc in takeover()
This is the h1 equivalent of previous "BUG/MEDIUM: mux-h2: fail earlier
on malloc in takeover()".
Connection takeover was implemented for H1 in 2.2 by commit f12ca9f8f1
("MEDIUM: mux_h1: 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 h1_release() will call some unsubscribe
and and possibly other things whose safety is not guaranteed (and the
ambiguity here alone is sufficient to be careful). 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.
Contrary to mux-h2, a few tests were not sufficient to try to crash the
process, but there's nothing that indicates it couldn't happen based on
the description above.
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 95fd2d68018d749ff0ad92b4b69d4b3fd75a82e4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 54dd551a00e95731a9e3dd7554b5213cd7ccef80)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 4a936c60ed941823e4efdbf6cf0a3eecf733a242)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5c869aafc3438983385ec6ec8cc616c0316e87da)
[cf: task_new(tid_bit) is used instead of task_new_here()]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
1 file changed