From: Willy Tarreau <w@1wt.eu>
Date: Wed, 23 Sep 2020 14:33:00 +0000 (+0200)
Subject: BUG/MINOR: listeners: properly close listener FDs
X-Git-Tag: v2.3-dev5~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=82cd028d712a7cc54be047e1bcfa60fa9379a051;p=thirdparty%2Fhaproxy.git

BUG/MINOR: listeners: properly close listener FDs

The code dealing with zombie proxies in soft_stop() is bogus, it uses
close() instead of fd_delete(), leaving a live entry in the fdtab with
a dangling pointer to a free memory location. The FD might be reassigned
for an outgoing connection for the time it takes the proxy to completely
stop, or could be dumped on the CLI's "show fd" command. In addition,
the listener's FD was not even reset, leaving doubts about whether or
not it will happen again in deinit().

And in deinit(), the loop in charge of closing zombie FDs is particularly
unsafe because it closes the fd then calls unbind_listener() then
delete_listener() hoping none of them will touch it again. Since it
requires some mental efforts to figure what's done there, let's correctly
reset the fd here as well and close it using fd_delete() to eliminate any
remaining doubts.

It's uncertain whether this should be backported. Zombie proxies are rare
and the situations capable of triggering such issues are not trivial to
setup. However it's easy to imagine how things could go wrong if backported
too far. Better wait for any matching report if at all (this code has been
there since 1.8 without anobody noticing).
---

diff --git a/src/haproxy.c b/src/haproxy.c
index 22095175ac..7458dee363 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2599,7 +2599,8 @@ void deinit(void)
 			 * Close it and give the listener its real state.
 			 */
 			if (p->state == PR_STSTOPPED && l->state >= LI_ZOMBIE) {
-				close(l->rx.fd);
+				fd_delete(l->rx.fd);
+				l->rx.fd = -1;
 				l->state = LI_INIT;
 			}
 			unbind_listener(l);
diff --git a/src/proxy.c b/src/proxy.c
index 21d11ee247..18cdf426e0 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1244,8 +1244,10 @@ void soft_stop(void)
 		    struct listener *, by_fe)->state > LI_ASSIGNED) {
 			struct listener *l;
 			list_for_each_entry(l, &p->conf.listeners, by_fe) {
-				if (l->state > LI_ASSIGNED)
-					close(l->rx.fd);
+				if (l->state > LI_ASSIGNED) {
+					fd_delete(l->rx.fd);
+					l->rx.fd = -1;
+				}
 				l->state = LI_INIT;
 			}
 		}