From: William Lallemand <wlallemand@haproxy.org>
Date: Wed, 21 Jun 2023 07:44:18 +0000 (+0200)
Subject: BUG/MINOR: mworker: leak of a socketpair during startup failure
X-Git-Tag: v2.9-dev1~33
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=117b03ff4af5a93bf4ed68fa1b71bd35333866b8;p=thirdparty%2Fhaproxy.git

BUG/MINOR: mworker: leak of a socketpair during startup failure

Aurelien Darragon found a case of leak when working on ticket #2184.

When a reexec_on_failure() happens *BEFORE* protocol_bind_all(), the
worker is not fork and the mworker_proc struct is still there with
its 2 socketpairs.

The socketpair that is supposed to be in the master is already closed in
mworker_cleanup_proc(), the one for the worker was suppposed to
be cleaned up in mworker_cleanlisteners().

However, since the fd is not bound during this failure, the fd is never
closed.

This patch fixes the problem by setting the fd to -1 in the mworker_proc
after the fork, so we ensure that this it won't be close if everything
was done right, and then we try to close it in mworker_cleanup_proc()
when it's not set to -1.

This could be triggered with the script in ticket #2184 and a `ulimit -H
-n 300`. This will fail before the protocol_bind_all() when trying to
increase the nofile setrlimit.

In recent version of haproxy, there is a BUG_ON() in fd_insert() that
could be triggered by this bug because of the global.maxsock check.

Must be backported as far as 2.6.

The problem could exist in previous version but the code is different
and this won't be triggered easily without other consequences in the
master.
---

diff --git a/src/haproxy.c b/src/haproxy.c
index 93b0780108..b20974199b 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -3610,6 +3610,9 @@ int main(int argc, char **argv)
 							child->timestamp = date.tv_sec;
 							child->pid = ret;
 							child->version = strdup(haproxy_version);
+							/* at this step the fd is bound for the worker, set it to -1 so
+							 * it could be close in case of errors in mworker_cleanup_proc() */
+							child->ipc_fd[1] = -1;
 							break;
 						}
 					}
diff --git a/src/mworker.c b/src/mworker.c
index c4275a0fbb..6bace6b468 100644
--- a/src/mworker.c
+++ b/src/mworker.c
@@ -541,14 +541,11 @@ void mworker_cleanup_proc()
 	list_for_each_entry_safe(child, it, &proc_list, list) {
 
 		if (child->pid == -1) {
-			/* Close the socketpair master side.  We don't need to
-			 * close the worker side, because it's stored in the
-			 * GLOBAL cli listener which was supposed to be in the
-			 * worker and which will be closed in
-			 * mworker_cleanlisteners()
-			 */
+			/* Close the socketpairs. */
 			if (child->ipc_fd[0] > -1)
 				close(child->ipc_fd[0]);
+			if (child->ipc_fd[1] > -1)
+				close(child->ipc_fd[1]);
 			if (child->srv) {
 				/* only exists if we created a master CLI listener */
 				srv_drop(child->srv);