BUG/MEIDUM: mworker: fix fd leak from master to worker

During re-execution master keeps always opened "reload" sockpair FDs and
shared sockpair ipc_fd[0], the latter is using to transfert listeners sockets
from the previously forked worker to the new one. So, these master's FDs are
inherited in the newly forked worker and must be closed in its context.

"reload" sockpair inherited FDs and shared sockpair FD (ipc_fd[0]) are closed
separately, becase master doesn't recreate "reload" sockpair each time after
its re-exec. It always keeps the same FDs for this "reload" sockpair. So in
worker context it can be closed immediately after the fork.

At contrast, shared sockpair is created each time after reload, when the new
worker will be forked. So, if N previous workers are still exist at this moment,
the new worker will inherit N ipc_fd[0] from master. So, it's more save to
close all these FDs after get_listeners_fd() and bind_listeners() calls.
Otherwise, early closed FDs in the worker context will be immediately bound to
listeners and we could potentially have some bugs.

diff --git a/src/haproxy.c b/src/haproxy.c
index 2a75ecc98a5686c9914247d34ac3b759d5e8d6fc..0b4bab472b5b724575970399f63797826b1054a7 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2162,6 +2162,18 @@ static void apply_master_worker_mode()
 
                                break;
                        }
+
+                       /* need to close reload sockpair fds, inherited after master's execvp and fork(),
+                        * we can't close these fds in master before the fork(), as ipc_fd[1] serves after
+                        * the mworker_reexec to obtain the MCLI client connection fd, like this we can
+                        * write to this connection fd the content of the startup_logs ring.
+                        */
+                       if (child->options & PROC_O_TYPE_MASTER) {
+                               if (child->ipc_fd[0] > 0)
+                                       close(child->ipc_fd[0]);
+                               if (child->ipc_fd[1] > 0)
+                                       close(child->ipc_fd[1]);
+                       }
                }
                break;
        default:
@@ -3672,6 +3684,7 @@ int main(int argc, char **argv)
        int intovf = (unsigned char)argc + 1; /* let the compiler know it's strictly positive */
        struct cfgfile *cfg, *cfg_tmp;
        struct ring *tmp_startup_logs = NULL;
+       struct mworker_proc *proc;
 
        /* Catch broken toolchains */
        if (sizeof(long) != sizeof(void *) || (intovf + 0x7FFFFFFF >= intovf)) {
@@ -3861,6 +3874,27 @@ int main(int argc, char **argv)
 
        bind_listeners();
 
+       /* worker context: now listeners fds were transferred from the previous
+        * worker, all listeners fd are bound. So we can close ipc_fd[0]s of all
+        * previous workers, which are still referenced in the proc_list, i.e.
+        * they are not exited yet at the moment, when this current worker was
+        * forked. Thus the current worker inherits ipc_fd[0]s from the previous
+        * ones by it's parent, master, because we have to keep shared sockpair
+        * ipc_fd[0] always opened in master (master CLI server is listening on
+        * this fd). It's safe to call close() at this point on these inhereted
+        * ipc_fd[0]s, as they are inhereted after master re-exec unbound, we
+        * keep them like this during bind_listeners() call. So, these fds were
+        * never referenced in the current worker's fdtab.
+        */
+       if ((global.mode & MODE_MWORKER) && !master) {
+               list_for_each_entry(proc, &proc_list, list) {
+                       if ((proc->options & PROC_O_TYPE_WORKER) && (proc->options & PROC_O_LEAVING)) {
+                               close(proc->ipc_fd[0]);
+                               proc->ipc_fd[0] = -1;
+                       }
+               }
+       }
+
        /* Exit in standalone mode, if no listeners found */
        if (!(global.mode & MODE_MWORKER) && listeners == 0) {
                ha_alert("[%s.main()] No enabled listener found (check for 'bind' directives) ! Exiting.\n", argv[0]);

diff --git a/src/sock.c b/src/sock.c
index 4b872d15ed6efa41534fbc44cf16df7430eb1546..5fb25f46ee912eed3a9a8ab01d4baec86c1c9359 100644 (file)
--- a/src/sock.c
+++ b/src/sock.c
@@ -440,6 +440,17 @@ int sock_get_old_sockets(const char *unixsocket)
                int sv[2];
                int dst_fd;
 
+               /* dst_fd is always open in the worker process context because
+                * it's inherited from the master via -x cmd option. It's closed
+                * futher in main (after bind_listeners()) and not here for the
+                * simplicity. In main(), after bind_listeners(), it's safe just
+                * to loop over all workers list, launched before this reload and
+                * to close its ipc_fd[0], thus we also close this fd. If we
+                * would close dst_fd here, it might be potentially "reused" in
+                * bind_listeners() followed this call, thus it would be difficult
+                * to exclude it, in the case if it was bound again when we will
+                * filter the previous workers list.
+                */
                dst_fd = strtoll(unixsocket + strlen("sockpair@"), NULL, 0);
 
                if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {