From 82cd028d712a7cc54be047e1bcfa60fa9379a051 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 23 Sep 2020 16:33:00 +0200 Subject: [PATCH] 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). --- src/haproxy.c | 3 ++- src/proxy.c | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/haproxy.c b/src/haproxy.c index 22095175a..7458dee36 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 21d11ee24..18cdf426e 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; } }