BUG/MEDIUM: cli: fix master CLI connection slot leak on client disconnect

In master-worker mode the master CLI proxy (mworker_proxy) has a
hardcoded maxconn of 10. When a client connects to the master CLI
socket and issues a command that gets forwarded to an unresponsive
worker (e.g. one that is stuck or very slow), the connection hangs
waiting for the worker's response. If the client then disconnects
(timeout, Ctrl-C, etc.), the connection slot is never released because
the client-side FIN is never propagated to tear down the backend.

After 10 such leaked slots the master CLI socket becomes completely
unreachable, returning "Resource temporarily unavailable" to any new
connection attempt.

The fix has three parts:

1) Remove sc_schedule_shutdown(s->scb) after command forwarding.
   The worker doesn't need the TCP FIN to know when a command ends -
   CLI commands are newline-delimited. Without this shutdown, the
   backend never enters half-close during normal command processing,
   so timeout server-fin is never implicitly armed by process_stream().

2) In the AN_RES_WAIT_CLI early-return path, call sc_set_hcto(s->scb)
   only when the client has disconnected (SC_FL_EOS on scf) and there
   is no more pending request data. This arms the 1s server-fin timer
   exclusively in the stuck-worker scenario.

3) Call channel_dont_close(req) to prevent process_stream() from
   auto-forwarding the client's FIN to the backend via CF_AUTO_CLOSE.

A 1s timeout server-fin is configured on mworker_proxy. It is only
armed after the client disconnects cleanly, so it never fires during
normal command processing. It ensures a stuck backend releases its
connection slot promptly once the client is gone.

Locally-handled commands (master applet) are unaffected because they
complete synchronously before the client has reason to disconnect.
The scb->ioto reset (TICK_ETERNITY) at end-of-transaction in
pcli_wait_for_response() prevents any timer leakage between commands.

This fixes GH issue haproxy#3351.
This should be backported to all stable branches.

Author: alexanderstephan

reg-tests/mcli/mcli_master_socket_leak.vtc

@@ -0,0 +1,66 @@
+varnishtest "Bug fix: master CLI connection slots freed on client disconnect"
+
+# Regression test for a master CLI socket connection leak in master-worker mode.
+#
+# mworker_proxy has a fixed maxconn of 10.  When clients connect to the master
+# socket, send a command that is forwarded to a busy worker, and then close
+# the connection due to a client-side receive timeout, haproxy must free each
+# slot as the client disconnects.
+#
+# Without the fix: slots remain occupied after the client disconnects, so the
+# master CLI becomes unreachable once all 10 slots are filled.
+# With the fix: slots are freed on client disconnect and the master CLI keeps
+# accepting new connections.
+#
+# The worker is made unresponsive using "debug dev delay" (expert-mode) with
+# nbthread 1 so a single delay blocks the entire worker CLI.  This avoids
+# using SIGSTOP/SIGCONT which can be unreliable in CI environments.
+
+#REGTEST_TYPE=bug
+
+feature cmd "command -v socat"
+feature cmd "command -v timeout"
+feature ignore_unknown_macro
+
+server s1 {
+} -start
+
+haproxy h1 -W -S -conf {
+    global
+        nbthread 1
+
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe
+        bind "fd@${fe}"
+        default_backend be
+
+    backend be
+        server s1 ${s1_addr}:${s1_port}
+} -start
+
+# Fill all 10 master CLI slots (mworker_proxy->maxconn is hardcoded to 10).
+# Each socat sends "expert-mode on" followed by "@1 debug dev delay 10000"
+# which blocks the single worker thread for 10 s.  After 2 s the timeout(1)
+# wrapper kills socat, simulating a client-side receive timeout.  "wait"
+# ensures all background processes have exited before proceeding.
+shell {
+    for i in $(seq 1 10); do
+        (printf "expert-mode on\n@1 debug dev delay 10000\n" \
+         | timeout 2 socat TCP:${h1_mcli_addr}:${h1_mcli_port} - 2>/dev/null) &
+    done
+    wait
+}
+
+# This is the key assertion: after all 10 clients have disconnected, a new
+# connection to the master CLI must succeed.  With the bug all 10 slots are
+# still marked occupied and this connect is refused or times out.
+haproxy h1 -mcli {
+    send "show version"
+    expect ~ "3."
+}
+

reg-tests/mcli/mcli_reload_no_timeout.lua

@@ -0,0 +1,5 @@
+-- Delay worker readiness by 2s to test that serverfin does not
+-- kill the reload command while the new worker is starting.
+core.register_init(function()
+    os.execute("sleep 2")
+end)

reg-tests/mcli/mcli_reload_no_timeout.vtc

@@ -0,0 +1,65 @@
+varnishtest "Verify reload via master CLI is not affected by serverfin timeout"
+
+# Regression test: the timeout server-fin on the MASTER proxy must not
+# apply to locally-handled commands (applets).  A reload command is
+# forwarded to the master applet and may take longer than 1s while the
+# new worker is starting.  Without the fix, the pcli response analyser
+# would fire a read timeout and return "Can't connect to the target CLI!"
+# instead of the reload status.
+
+#REQUIRE_OPTIONS=LUA
+#REGTEST_TYPE=bug
+
+feature cmd "command -v socat"
+feature cmd "$HAPROXY_PROGRAM -vv | grep -q '+LUA'"
+feature ignore_unknown_macro
+
+server s1 {
+    rxreq
+    txresp
+} -start
+
+haproxy h1 -W -S -conf {
+    global
+        tune.lua.bool-sample-conversion normal
+        # Lua init hook that sleeps 2s, delaying worker readiness on reload
+        lua-load ${testdir}/mcli_reload_no_timeout.lua
+
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe
+        bind "fd@${fe}"
+        default_backend be
+
+    backend be
+        server s1 ${s1_addr}:${s1_port}
+} -start
+
+# Issue a reload via socat as a second command (after "show version").
+# The first command causes CF_AUTO_CLOSE to be set on the request channel.
+# When socat half-closes its write side after sending, process_stream()
+# triggers sc_shutdown(scb) -> sc_set_hcto() which applies serverfin=1s
+# to the backend applet.  Without the fix, the reload response (which comes
+# from the master applet) would be killed by this 1s timeout if the new
+# worker takes time to start, returning "Can't connect to the target CLI!".
+shell {
+    RESULT=$(printf "show version\nreload\n" | socat -t10 TCP:${h1_mcli_addr}:${h1_mcli_port} - 2>/dev/null)
+    echo "Got: $RESULT"
+    echo "$RESULT" | grep -q "Success=1" || {
+        echo "FAIL: reload did not succeed. Got: $RESULT"
+        exit 1
+    }
+}
+
+# Verify the master CLI is still functional after reload
+shell {
+    RESULT=$(printf "show version\n" | socat -t5 TCP:${h1_mcli_addr}:${h1_mcli_port} - 2>/dev/null)
+    echo "$RESULT" | grep -q "3\." || {
+        echo "FAIL: show version failed. Got: $RESULT"
+        exit 1
+    }
+}

src/cli.c

@@ -3484,8 +3484,25 @@ int pcli_wait_for_request(struct stream *s, struct channel *req, int an_bit)
 	 * current one. Just wait. At this stage, errors should be handled by
 	 * the response analyzer.
 	 */
-	if (s->res.analysers & AN_RES_WAIT_CLI)
+	if (s->res.analysers & AN_RES_WAIT_CLI) {
+		/* Prevent process_stream from auto-forwarding the client close to
+		 * the backend via the CF_AUTO_CLOSE check while we're waiting for
+		 * a response - we manage the connection lifetime ourselves.
+		 */
+		channel_dont_close(req);
+
+		/* If the client disconnected cleanly and we have no more commands
+		 * to send, arm the server-fin timer on the backend.  A stuck
+		 * worker that never responds will then be aborted after
+		 * timeout server-fin, freeing the connection slot (GH #3351).
+		 * When lra is TICK_ETERNITY (no data received yet) the timer never
+		 * fires, so this is safe to call on every wakeup.
+		 */
+		if ((s->scf->flags & SC_FL_EOS) && !ci_data(req))
+			sc_set_hcto(s->scb);
+
 		return 0;
+	}
 
 	pcli->flags &= ~PCLI_F_BIDIR; // only for one connection
 	if ((pcli->flags & ACCESS_LVL_MASK) == ACCESS_LVL_NONE)
@@ -3528,13 +3545,7 @@ int pcli_wait_for_request(struct stream *s, struct channel *req, int an_bit)
 		else
 			channel_forward_forever(req);
 
-		if (!(pcli->flags & PCLI_F_PAYLOAD)) {
-			/* we send only 1 command per request, and we write
-			 * close after it when not in full-duplex mode.
-			 */
-			if (!(pcli->flags & PCLI_F_BIDIR))
-				sc_schedule_shutdown(s->scb);
-		} else {
+		if (pcli->flags & PCLI_F_PAYLOAD) {
 			pcli_write_prompt(s);
 		}
 
@@ -3595,6 +3606,13 @@ int pcli_wait_for_request(struct stream *s, struct channel *req, int an_bit)
         if (s->scf->flags & (SC_FL_ABRT_DONE|SC_FL_EOS)) {
                 /* There is no more request or a only a partial one and we
                  * receive a close from the client, we can leave */
+		if (s->scf->flags & SC_FL_ABRT_DONE) {
+			/* Client sent RST: abort the backend immediately.
+			 * Note: when AN_RES_WAIT_CLI is set we never reach here
+			 * (early return above), so this handles the pre-connect case.
+			 */
+			sc_schedule_abort(s->scb);
+		}
 		sc_schedule_shutdown(s->scf);
                 s->req.analysers &= ~AN_REQ_WAIT_CLI;
                 return 1;
@@ -3834,8 +3852,13 @@ int mworker_cli_create_master_proxy(char **errmsg)
 	mworker_proxy->mode = PR_MODE_CLI;
 	/* default to 10 concurrent connections */
 	mworker_proxy->maxconn = 10;
-	/* no timeout */
-	mworker_proxy->timeout.client = 0;
+	mworker_proxy->timeout.client = 0; /* no timeout */
+	/* 1s server-fin timeout: armed only after the client disconnects cleanly
+	 * (scf EOS + no pending commands), so it never fires during normal command
+	 * processing.  It ensures a stuck backend releases its slot promptly once
+	 * the client is gone.
+	 */
+	mworker_proxy->timeout.serverfin = MS_TO_TICKS(1000);
 	mworker_proxy->conf.file = strdup("MASTER");
 	mworker_proxy->conf.line = 0;
 	mworker_proxy->accept = frontend_accept;