wolfsshd: bound the shell child reap and flush the held backlog

.github/workflows/code-coverage.yml

@@ -55,10 +55,10 @@ jobs:
         uses: actions/checkout@v6
 
       # clang 18 is the min: -fcoverage-mcdc does not exist before it.
-      - name: Install clang and LLVM coverage tools
+      - name: Install clang, LLVM coverage tools and paramiko
         run: |
           sudo apt-get update
-          sudo apt-get install -y clang-18 llvm-18 libclang-rt-18-dev
+          sudo apt-get install -y clang-18 llvm-18 libclang-rt-18-dev python3-paramiko
 
       - name: Download wolfSSL
         uses: actions/download-artifact@v8

.github/workflows/sshd-test.yml

@@ -118,6 +118,13 @@ jobs:
         working-directory: ./wolfssh/
         run: make check
 
+      # For sshd_channel_close_test.sh. The suite runs as root, which does not
+      # see a per-user pip install, so use the distro package.
+      - name: Install paramiko
+        run: |
+          sudo apt-get -y update
+          sudo apt-get -y install python3-paramiko
+
       - name: Run wolfSSHd tests
         working-directory: ./wolfssh/apps/wolfsshd/test
         run: |

apps/wolfsshd/test/run_all_sshd_tests.sh

@@ -15,6 +15,7 @@ test_cases=(
  "sshd_term_close_test.sh"
  "sshd_stdin_eof_test.sh"
  "sshd_stdin_stall_test.sh"
+ "sshd_channel_close_test.sh"
  "ssh_kex_algos.sh"
 )
 

apps/wolfsshd/test/sshd_channel_close_test.sh

@@ -0,0 +1,163 @@
+#!/bin/bash
+# Closing a session channel while wolfsshd holds its output must not leave the
+# connection process waiting on the command. The exec command traps INT and
+# HUP, so only the reap's SIGKILL ends it; a pty shell must exit on the hangup.
+# The exit status must still reach the client, from an exec and from a shell.
+#
+# Needs paramiko: the OpenSSH client never closes the channel early; it keeps
+# reading and discards what it cannot write.
+HOST="$1"
+PORT="$2"
+USER_NAME="${3:-`whoami`}"
+
+command -v python3 >/dev/null 2>&1 || exit 77
+python3 -c "import paramiko" >/dev/null 2>&1 || exit 77
+command -v timeout >/dev/null 2>&1 || exit 77
+
+# The command's pid is checked on this machine.
+case "$HOST" in
+    127.0.0.1|localhost|::1) ;;
+    *) exit 77 ;;
+esac
+
+KEYDIR=`mktemp -d` || exit 1
+MARKDIR=`mktemp -d` || exit 1
+trap 'rm -rf "$KEYDIR" "$MARKDIR"' EXIT
+# Private to the login user, who writes the marker.
+chown "$USER_NAME" "$MARKDIR" || exit 77
+cp ../../../keys/hansel-key-ecc.pem "$KEYDIR/id_ecdsa" || exit 1
+chmod 600 "$KEYDIR/id_ecdsa"
+
+timeout 60 python3 -u - "$HOST" "$PORT" "$USER_NAME" "$KEYDIR/id_ecdsa" \
+    "$MARKDIR/exited" <<'EOF'
+import os
+import re
+import sys
+import time
+
+import paramiko
+
+host, port, user, key = sys.argv[1], int(sys.argv[2]), sys.argv[3], sys.argv[4]
+marker = sys.argv[5]
+LIMIT = 5
+WAIT = 20  # for any one read or exit status
+
+
+def alive(pid):
+    try:
+        os.kill(pid, 0)
+    except ProcessLookupError:
+        return False
+    except PermissionError:
+        pass
+    return True
+
+
+def connect():
+    client = paramiko.SSHClient()
+    client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
+    client.connect(host, port=port, username=user,
+                   pkey=paramiko.ECDSAKey.from_private_key_file(key),
+                   look_for_keys=False, allow_agent=False, timeout=10)
+    return client
+
+
+def run_case(name, shell):
+    client = connect()
+    chan = client.get_transport().open_session()
+    chan.settimeout(WAIT)
+    if shell:
+        chan.get_pty()
+        chan.invoke_shell()
+        # The marker is a builtin redirection on the exit path only.
+        chan.send("trap 'exit' HUP; trap ': > %s' EXIT; echo PID=$$\n"
+                  % marker)
+    else:
+        chan.exec_command('echo PID=$$; trap "" INT HUP; '
+                          'seq 1 10000000; sleep 30')
+
+    # The pty echoes the typed line with a literal $$, which does not match,
+    # and the newline keeps a pid split across two reads from matching.
+    out = b""
+    match = None
+    while match is None:
+        data = chan.recv(4096)
+        if not data:
+            break
+        out += data
+        match = re.search(rb"PID=(\d+)\r?\n", out)
+    if match is None:
+        print("%s: no pid from the command" % name)
+        client.close()
+        return False
+    pid = int(match.group(1))
+    if shell:
+        chan.send("seq 1 10000000\n")
+
+    # Stop reading until the window fills and wolfsshd holds output, then
+    # close the channel with the transport still up.
+    time.sleep(1)
+    chan.close()
+
+    start = time.time()
+    while alive(pid) and time.time() - start < LIMIT:
+        time.sleep(0.1)
+    # The connection process ends too, and closes the connection.
+    transport = client.get_transport()
+    while transport.is_active() and time.time() - start < LIMIT:
+        time.sleep(0.1)
+    closed = not transport.is_active()
+    client.close()
+
+    if alive(pid):
+        print("%s: command %d still running %ds after the channel close" %
+              (name, pid, LIMIT))
+        return False
+    print("%s: command %d gone %.1fs after the channel close" %
+          (name, pid, time.time() - start))
+    if not closed:
+        print("%s: connection still open %ds after the channel close" %
+              (name, LIMIT))
+        return False
+    # SIGKILL cannot run the trap, so the marker means the shell exited itself.
+    if shell and not os.path.exists(marker):
+        print("%s: killed instead of exiting on the hangup" % name)
+        return False
+    return True
+
+
+def run_status(name, shell):
+    client = connect()
+    chan = client.get_transport().open_session()
+    if shell:
+        # An exec takes the pipe path even with a pty; a shell reads the pty.
+        chan.get_pty()
+        chan.invoke_shell()
+        chan.send("exit 3\n")
+    else:
+        chan.exec_command("exit 3")
+    start = time.time()
+    while not chan.exit_status_ready() and time.time() - start < WAIT:
+        time.sleep(0.05)
+    if not chan.exit_status_ready():
+        print("%s: no exit status within %ds" % (name, WAIT))
+        client.close()
+        return False
+    status = chan.recv_exit_status()
+    client.close()
+    print("%s: exit status %d, expected 3" % (name, status))
+    return status == 3
+
+
+try:
+    connect().close()
+except (paramiko.SSHException, OSError) as e:
+    print("cannot connect with paramiko: %s" % e)
+    sys.exit(77)
+
+ok = run_case("exec", False)
+ok = run_case("pty shell", True) and ok
+ok = run_status("exec status", False) and ok
+ok = run_status("shell status", True) and ok
+sys.exit(0 if ok else 1)
+EOF

apps/wolfsshd/wolfsshd.c

@@ -2672,6 +2672,12 @@ static int SHELL_SetNonBlocking(int fd)
 #ifndef WOLFSSHD_SHELL_FLUSH_WAIT_US
     #define WOLFSSHD_SHELL_FLUSH_WAIT_US 50000
 #endif
+#ifndef WOLFSSHD_CHILD_REAP_TRIES
+    #define WOLFSSHD_CHILD_REAP_TRIES 10
+#endif
+#ifndef WOLFSSHD_CHILD_REAP_WAIT_US
+    #define WOLFSSHD_CHILD_REAP_WAIT_US 50000
+#endif
 
 /* Send out the last of a shell's output once the child has been reaped. The
  * SSH socket is non blocking, so retry a bounded number of times on a full
@@ -2784,6 +2790,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
                            * destructive, so what a short write leaves is
                            * carried to the next pass.  */
     int   childInIdx = 0; /* How much of those the child has taken.  */
+    int   drainPipes = 1;
 
     backlog.len = 0;
     backlog.state = SHELL_SEND_READY;
@@ -2862,6 +2869,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
         }
     }
 
+    /* Before the fork, so a child that exits at once clears ChildRunning */
+    signal(SIGCHLD, ChildSig);
     ChildRunning = 1;
     childPid = forkpty(&childFd, NULL, NULL, NULL);
     if (childPid < 0) {
@@ -3039,7 +3048,6 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
         return WS_FATAL_ERROR;
     }
 
-    signal(SIGCHLD, ChildSig);
     signal(SIGINT, SIG_DFL);
 
     rc = tcgetattr(childFd, &tios);
@@ -3487,7 +3495,10 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
                 /* Treat a 0 return as EOF so the loop can shut down. */
                 if (cnt_r < 0) {
                     int err = errno;
-                    if (err != EINTR && err != EAGAIN
+                    if (err == EIO) {
+                        stdoutEmpty = 1;  /* no process has the terminal open */
+                    }
+                    else if (err != EINTR && err != EAGAIN
                             && err != EWOULDBLOCK) {
                         break;
                     }
@@ -3535,14 +3546,39 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
         }
     }
 
+    /* Close the pty master so the shell gets SIGHUP and can exit on its own */
+    if (childFd >= 0) {
+        close(childFd);
+        childFd = -1;
+    }
+
     /* get return value of child process */
     {
         int waitStatus;
+        int tries = 0;
 
         do {
-            rc = waitpid(childPid, &waitStatus, 0);
+            rc = waitpid(childPid, &waitStatus, WNOHANG);
+            if (rc == 0 && tries < WOLFSSHD_CHILD_REAP_TRIES) {
+                usleep(WOLFSSHD_CHILD_REAP_WAIT_US);
+                tries++;
+            }
+            else if (rc == 0) {
+                /* Child did not exit in time; force it with SIGKILL. */
+                if (kill(childPid, SIGKILL) == 0) {
+                    rc = waitpid(childPid, &waitStatus, 0);
+                }
+                else {
+                    int err = errno;
+
+                    wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Unable to kill child "
+                        "process %d, errno %d", (int)childPid, err);
+                    errno = err;
+                    rc = -1;
+                }
+            }
             /* if the waitpid experienced an interrupt then try again */
-        } while (rc < 0 && errno == EINTR);
+        } while (rc == 0 || (rc < 0 && errno == EINTR));
 
         if (rc < 0) {
             wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Issue waiting for child's exit "
@@ -3557,21 +3593,33 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
         }
     }
 
+    /* A break can leave unsent shell output in shellBuffer; send it before
+     * the pipe drain reads into shellBuffer. */
+    if (backlog.len > 0) {
+        if (backlog.state == SHELL_SEND_NEVER
+                || SHELL_FlushOut(ssh, sshFd, shellChannelId, shellBuffer,
+                    backlog.len, backlog.ext) != 0) {
+            /* later pipe output would leave a gap in the stream */
+            drainPipes = 0;
+        }
+        backlog.len = 0;
+    }
+
     /* check for any left over data in pipes then close them */
     if (!ptyReq || forcedCmd) {
         int readSz;
 
         /* when the pipe can not be made non blocking skip the drain, a
          * blocking read here could hang the connection process */
-        if (SHELL_SetNonBlocking(stdoutPipe[0]) == 0) {
+        if (drainPipes && SHELL_SetNonBlocking(stdoutPipe[0]) == 0) {
             readSz = (int)read(stdoutPipe[0], shellBuffer, sizeof shellBuffer);
             if (readSz > 0) {
                 SHELL_FlushOut(ssh, sshFd, shellChannelId, shellBuffer, readSz,
                     0);
             }
         }
 
-        if (SHELL_SetNonBlocking(stderrPipe[0]) == 0) {
+        if (drainPipes && SHELL_SetNonBlocking(stderrPipe[0]) == 0) {
             readSz = (int)read(stderrPipe[0], shellBuffer, sizeof shellBuffer);
             if (readSz > 0) {
                 SHELL_FlushOut(ssh, sshFd, shellChannelId, shellBuffer, readSz,