From: Willy Tarreau <w@1wt.eu>
Date: Wed, 10 Aug 2022 15:08:17 +0000 (+0200)
Subject: BUG/MEDIUM: poller: use fd_delete() to release the poller pipes
X-Git-Tag: v2.7-dev4~63
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e6ca435c04a344e2a48b11a64b4faf16c9a3b024;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: poller: use fd_delete() to release the poller pipes

The poller pipes needed to communicate between multiple threads are
allocated in init_pollers_per_thread() and released in
deinit_pollers_per_thread(). The former adds them via fd_insert()
so that they are known, but the former only closes them using a
regular close().

This asymmetry represents a problem, because we have in the fdtab[]
an entry for something that may disappear when one thread leaves, and
since these FD numbers are very low, there is a very high likelihood
that they are immediately reassigned to another thread trying to
connect() to a server or just sending a health check. In this case,
the other thread is going to fd_insert() the fd and the recently
added consistency checks will notive that ->owner is not NULL and
will crash. We just need to use fd_delete() here to match fd_insert().

Note that this test was added in 2.7-dev2 by commit 36d9097cf
("MINOR: fd: Add BUG_ON checks on fd_insert()") which was backported
to 2.4 as a safety measure (since it allowed to catch particularly
serious issues). The patch in itself isn't wrong, it just revealed
a long-dormant bug (been there since 1.9-dev1, 4 years ago). As such
the current patch needs to be backported wherever the commit above
is backported.

Many thanks to Christian Ruppert for providing detailed traces in
github issue #1807 and Cedric Paillet for bringing his complementary
analysis that helped to understand the required conditions for this
issue to happen (fast health checks @100ms + randomly long connections
~7s + fast reloads every second + hard-stop-after 5s were necessary
on the dev's machine to trigger it from time to time).
---

diff --git a/src/fd.c b/src/fd.c
index ba864deed8..65036c74f3 100644
--- a/src/fd.c
+++ b/src/fd.c
@@ -974,9 +974,9 @@ static void deinit_pollers_per_thread()
 	/* rd and wr are init at the same place, but only rd is init to -1, so
 	  we rely to rd to close.   */
 	if (poller_rd_pipe > -1) {
-		close(poller_rd_pipe);
+		fd_delete(poller_rd_pipe);
 		poller_rd_pipe = -1;
-		close(poller_wr_pipe[tid]);
+		fd_delete(poller_wr_pipe[tid]);
 		poller_wr_pipe[tid] = -1;
 	}
 }