From 3523286217e45d9731acb15932db7fe93ae05798 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 18 Sep 2026 09:34:56 -0700 Subject: [PATCH 1/6] wolfsshd: keep the shell loop past a stderr EOF read() returns 0 at end of file and leaves errno alone, so the stderr arm consults it only for a -1 return. A 0 marks that stream done and drops it from the read set; stdout does the same, so neither spins on a descriptor that stays ready once it has reported EOF. - a stale EINTR from the child's SIGCHLD no longer ends the loop with the peer's output still queued - EINTR and EWOULDBLOCK join EAGAIN as non-fatal on a -1 return - select polls rather than blocks once the child is gone, its output is drained and nothing is held, so the loop still reaches its exit --- apps/wolfsshd/wolfsshd.c | 43 ++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 6c3ba5623..379d3480c 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -2772,6 +2772,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, int wantWrite = 0; int peerConnected = 1; int stdoutEmpty = 0; + int stderrEmpty = 0; int ptyReq = 0; int childInSz = 0; /* Bytes read off the channel into channelBuffer * that the child has yet to take. The read is @@ -3121,18 +3122,23 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, /* Watch the child's output only on a pass that would read it. A held * backlog skips those reads, and an exited child's pipe is always - * ready, so watching it then spins. */ + * ready, so watching it then spins. A pipe that has reported EOF + * stays ready for the same reason: drop it once it has. */ if (!backlog.len) { if (!ptyReq || forcedCmd) { - FD_SET(stdoutPipe[0], &readFds); - if (stdoutPipe[0] > maxFd) - maxFd = stdoutPipe[0]; + if (!stdoutEmpty) { + FD_SET(stdoutPipe[0], &readFds); + if (stdoutPipe[0] > maxFd) + maxFd = stdoutPipe[0]; + } - FD_SET(stderrPipe[0], &readFds); - if (stderrPipe[0] > maxFd) - maxFd = stderrPipe[0]; + if (!stderrEmpty) { + FD_SET(stderrPipe[0], &readFds); + if (stderrPipe[0] > maxFd) + maxFd = stderrPipe[0]; + } } - else { + else if (!stdoutEmpty) { FD_SET(childFd, &readFds); if (childFd > maxFd) maxFd = childFd; @@ -3165,6 +3171,14 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, noWait.tv_usec = 0; timeout = &noWait; } + else if (!ChildRunning && stdoutEmpty && !backlog.len) { + /* The child is gone, its output is drained and nothing is + * held: the foot of the loop ends the session this pass, so + * do not wait on a peer that has nothing left to send. */ + noWait.tv_sec = 0; + noWait.tv_usec = 0; + timeout = &noWait; + } rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, timeout); if (rc == -1) { @@ -3361,13 +3375,20 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, if (FD_ISSET(stderrPipe[0], &readFds)) { cnt_r = (int)read(stderrPipe[0], shellBuffer, sizeof shellBuffer); - /* This read will return 0 on EOF */ - if (cnt_r <= 0) { + /* errno only speaks for a -1 return. A 0 is EOF and leaves + * it alone, so testing it there reads whatever the last + * call left -- an EINTR from the child's SIGCHLD ends the + * loop with the peer's output still queued. */ + if (cnt_r < 0) { int err = errno; - if (err != EAGAIN && err != 0) { + if (err != EINTR && err != EAGAIN + && err != EWOULDBLOCK) { break; } } + else if (cnt_r == 0) { + stderrEmpty = 1; + } else { if (cnt_r > 0) { cnt_w = wolfSSH_extended_data_send(ssh, shellBuffer, From 421fd06f0bb977980226214b6e51eb090ecc3f08 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 18 Sep 2026 10:42:04 -0700 Subject: [PATCH 2/6] wolfsshd: test a stderr EOF with output queued sshd_stderr_eof_test.sh streams 16 MB through a shell session whose reader stalls, so the send window fills and the child exits while the loop still holds a backlog. The transfer repeats twelve times because whether the stale errno at the stderr EOF is fatal is a race. - a short transfer is never correct, so a failure here is always real - the fixed tree passes 48 consecutive transfers; the truncating one fails on the first - runs in the sudo-only block beside sshd_window_full_test.sh, which reaches the same path about 1 run in 12 --- apps/wolfsshd/test/run_all_sshd_tests.sh | 1 + apps/wolfsshd/test/sshd_stderr_eof_test.sh | 87 ++++++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100755 apps/wolfsshd/test/sshd_stderr_eof_test.sh diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index b2efeffcf..eab454bea 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -576,6 +576,7 @@ else run_test "sshd_forcedcmd_test.sh" run_test "sshd_match_overlap_test.sh" run_test "sshd_window_full_test.sh" + run_test "sshd_stderr_eof_test.sh" run_test "sshd_empty_password_test.sh" run_test "sshd_permitroot_test.sh" run_test "sshd_permitroot_prohibit_password.sh" diff --git a/apps/wolfsshd/test/sshd_stderr_eof_test.sh b/apps/wolfsshd/test/sshd_stderr_eof_test.sh new file mode 100755 index 000000000..502a8a169 --- /dev/null +++ b/apps/wolfsshd/test/sshd_stderr_eof_test.sh @@ -0,0 +1,87 @@ +#!/bin/bash + +# The child's stderr reaching end of file must not end the shell loop while +# its stdout still has bytes queued for the peer. +# +# read() returns 0 at EOF and leaves errno alone, so an errno tested there +# reports whatever the last call left. A shell loop holding a backlog stops +# reading the child's pipes, so stdout backs up and the stderr EOF arrives +# behind it; if that EOF is read as an error the loop ends and everything +# still queued is dropped. The peer sees a short transfer and no error. +# +# Whether the stale errno happens to be fatal is a race, so one transfer +# proves nothing and this repeats. A short transfer is never correct, so a +# failure here is always real; a regression can only hide by passing every +# iteration. + +if [ -z "$1" ] || [ -z "$2" ]; then + echo "expecting host and port as arguments" + echo "./sshd_stderr_eof_test.sh 127.0.0.1 22222" + exit 1 +fi + +PWD=`pwd` + +if [ ! -z "$3" ]; then + USER="$3" +else + USER=`whoami` +fi +TEST_HOST="$1" +TEST_PORT="$2" + +# Enough data to outrun the send window, a reader that stalls long enough for +# the child to finish and exit while the backlog is held, and enough passes to +# make the race show. +TEST_SIZE=16777216 +TEST_STALL=4 +TEST_ITERS=12 + +source ./start_sshd.sh +cat < sshd_config_test_stderr_eof +Port $TEST_PORT +Protocol 2 +LoginGraceTime 600 +PermitRootLogin yes +PasswordAuthentication yes +PermitEmptyPasswords no +UsePrivilegeSeparation no +UseDNS no +HostKey $PWD/../../../keys/server-key.pem +AuthorizedKeysFile $PWD/authorized_keys_test +CONF + +start_wolfsshd "sshd_config_test_stderr_eof" +cd ../../.. + +TEST_CLIENT="./examples/client/client" +PRIVATE_KEY="./keys/hansel-key-ecc.der" +PUBLIC_KEY="./keys/hansel-key-ecc.pub" +PWD=`pwd` + +head -c $TEST_SIZE /dev/urandom > stderr-eof-test.txt +EXPECTED=`wc -c < stderr-eof-test.txt` + +RESULT=0 +for i in `seq 1 $TEST_ITERS`; do + # The inner client cats the file through the outer session, so the shell + # loop is the one relaying it. Stalling the outer client's reader fills + # the window and leaves the loop holding a backlog. + $TEST_CLIENT -q -c "cd $PWD; $TEST_CLIENT -q -c \"cat $PWD/stderr-eof-test.txt\" -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT" \ + -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT 2>/dev/null \ + | { sleep $TEST_STALL; cat; } > stderr-eof-test-result.txt + + GOT=`wc -c < stderr-eof-test-result.txt` + if [ "$GOT" != "$EXPECTED" ]; then + echo "pass $i of $TEST_ITERS truncated the shell output" + echo "expected $EXPECTED bytes, got $GOT, short by $((EXPECTED-GOT))" + RESULT=1 + break + fi +done + +rm -f stderr-eof-test.txt stderr-eof-test-result.txt +cd apps/wolfsshd/test +stop_wolfsshd + +exit $RESULT From cbad0b74b3fc1cc1fc2ef53ad253d3bac3e64edc Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 18 Sep 2026 14:02:42 -0700 Subject: [PATCH 3/6] wolfsshd: bound the wait on a drained child A stream leaves the read set once it has reported EOF, so a child whose output is drained while it still runs leaves the SSH socket as the only descriptor watched, and its SIGCHLD is the only wake left. One handled between the ChildRunning test and select() would not interrupt the call, so wait a second at a time there. An exited child still polls. --- apps/wolfsshd/wolfsshd.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 379d3480c..1686df8e5 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -3171,11 +3171,14 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, noWait.tv_usec = 0; timeout = &noWait; } - else if (!ChildRunning && stdoutEmpty && !backlog.len) { - /* The child is gone, its output is drained and nothing is - * held: the foot of the loop ends the session this pass, so - * do not wait on a peer that has nothing left to send. */ - noWait.tv_sec = 0; + else if (stdoutEmpty && !backlog.len) { + /* The child's output is drained and nothing is held. With the + * child gone the foot of the loop ends the session this pass, + * so do not wait on a peer that has nothing left to send. + * While it is still running its SIGCHLD is the only wake left, + * and one handled between the test here and the call below + * would not interrupt it, so bound that wait. */ + noWait.tv_sec = ChildRunning ? 1 : 0; noWait.tv_usec = 0; timeout = &noWait; } From 97cad3bf6dc25f18db02b6f9e29908fcc8ad7b51 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 18 Sep 2026 14:02:42 -0700 Subject: [PATCH 4/6] wolfsshd: match the stdout and pty read arms The stdout and pty childFd reads treat EINTR and EWOULDBLOCK as non-fatal, the set the stderr read already uses, so an interrupted read cannot end the session with the peer's output still queued. The "err != 0" term goes with it; read() sets errno on the -1 return that is the only way into the test. --- apps/wolfsshd/wolfsshd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 1686df8e5..9d65c570e 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -3434,7 +3434,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, /* This read will return 0 on EOF */ if (cnt_r < 0) { int err = errno; - if (err != EAGAIN && err != 0) { + if (err != EINTR && err != EAGAIN + && err != EWOULDBLOCK) { break; } } @@ -3481,7 +3482,8 @@ 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 != EAGAIN && err != 0) { + if (err != EINTR && err != EAGAIN + && err != EWOULDBLOCK) { break; } } From ce72017d3ad8395fad4b43bf015623a27af102ee Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 18 Sep 2026 14:02:42 -0700 Subject: [PATCH 5/6] wolfsshd: bound and guard the stderr EOF test A byte count cannot tell a short transfer from one that never finished and the runner sets no deadline of its own, so the client runs under "timeout" and a 124 is reported as a hang. An empty PID means the daemon never came up, which the count would otherwise report as the truncation this test exists to catch. - scratch paths held in variables, since bash rewrites PWD on the cd back into the test directory - an EXIT trap removes the 16 MB scratch file and stops the daemon --- apps/wolfsshd/test/sshd_stderr_eof_test.sh | 48 +++++++++++++++++++--- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/apps/wolfsshd/test/sshd_stderr_eof_test.sh b/apps/wolfsshd/test/sshd_stderr_eof_test.sh index 502a8a169..c9cffdd96 100755 --- a/apps/wolfsshd/test/sshd_stderr_eof_test.sh +++ b/apps/wolfsshd/test/sshd_stderr_eof_test.sh @@ -37,6 +37,17 @@ TEST_SIZE=16777216 TEST_STALL=4 TEST_ITERS=12 +# A byte count cannot tell a short transfer from one that never finished, and +# the runner invokes this test synchronously: a regression that leaves the +# session open would stall the whole suite here. One pass takes about four +# seconds, so this is only a deadline, not a budget. Degraded rather than +# skipped where "timeout" is missing, matching run_all_sshd_tests.sh. +TEST_TIMEOUT=120 +TIMEOUT="" +if command -v timeout >/dev/null 2>&1; then + TIMEOUT="timeout $TEST_TIMEOUT" +fi + source ./start_sshd.sh cat < sshd_config_test_stderr_eof Port $TEST_PORT @@ -52,6 +63,10 @@ AuthorizedKeysFile $PWD/authorized_keys_test CONF start_wolfsshd "sshd_config_test_stderr_eof" +if [ -z "$PID" ]; then + echo "Failed to start wolfsshd" + exit 1 +fi cd ../../.. TEST_CLIENT="./examples/client/client" @@ -59,19 +74,40 @@ PRIVATE_KEY="./keys/hansel-key-ecc.der" PUBLIC_KEY="./keys/hansel-key-ecc.pub" PWD=`pwd` -head -c $TEST_SIZE /dev/urandom > stderr-eof-test.txt -EXPECTED=`wc -c < stderr-eof-test.txt` +# Named in full because the trap below outlives the cd back into the test +# directory, and bash keeps PWD in step with that cd whatever this script +# assigned to it. +TEST_FILE="$PWD/stderr-eof-test.txt" +TEST_RESULT_FILE="$PWD/stderr-eof-test-result.txt" + +# The scratch file is 16 MB and the daemon is shared with the rest of the run, +# so neither may be left behind by an interrupted pass. stop_wolfsshd is +# idempotent, so the explicit call below still stands. +trap 'rm -f "$TEST_FILE" "$TEST_RESULT_FILE"; stop_wolfsshd' EXIT + +head -c $TEST_SIZE /dev/urandom > "$TEST_FILE" +EXPECTED=`wc -c < "$TEST_FILE"` RESULT=0 for i in `seq 1 $TEST_ITERS`; do # The inner client cats the file through the outer session, so the shell # loop is the one relaying it. Stalling the outer client's reader fills # the window and leaves the loop holding a backlog. - $TEST_CLIENT -q -c "cd $PWD; $TEST_CLIENT -q -c \"cat $PWD/stderr-eof-test.txt\" -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT" \ + $TIMEOUT $TEST_CLIENT -q -c "cd $PWD; $TEST_CLIENT -q -c \"cat $TEST_FILE\" -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT" \ -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT 2>/dev/null \ - | { sleep $TEST_STALL; cat; } > stderr-eof-test-result.txt + | { sleep $TEST_STALL; cat; } > "$TEST_RESULT_FILE" + # The client's own status, not the reader's. 124 is the deadline above, + # which a byte count would go on to report as a short transfer. + CLIENT_RESULT=${PIPESTATUS[0]} + + if [ "$CLIENT_RESULT" = 124 ]; then + echo "pass $i of $TEST_ITERS never finished" + echo "the client was still running after $TEST_TIMEOUT seconds" + RESULT=1 + break + fi - GOT=`wc -c < stderr-eof-test-result.txt` + GOT=`wc -c < "$TEST_RESULT_FILE"` if [ "$GOT" != "$EXPECTED" ]; then echo "pass $i of $TEST_ITERS truncated the shell output" echo "expected $EXPECTED bytes, got $GOT, short by $((EXPECTED-GOT))" @@ -80,7 +116,7 @@ for i in `seq 1 $TEST_ITERS`; do fi done -rm -f stderr-eof-test.txt stderr-eof-test-result.txt +rm -f "$TEST_FILE" "$TEST_RESULT_FILE" cd apps/wolfsshd/test stop_wolfsshd From c97a768e358be5a29118d93b8691908010d79979 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 18 Sep 2026 15:13:27 -0700 Subject: [PATCH 6/6] wolfsshd: give the stderr EOF test a temp dir The runner leases a port block per run so two runs can share a host, and this test's config and 16 MB payload are the other half of that: a second run overwrites them, and whichever finishes first removes them from under the other, which then reports a transfer that never ran short. Both live in a directory of this run's own now. - the external-host tally counts 12 local-only tests, not 10 --- apps/wolfsshd/test/run_all_sshd_tests.sh | 2 +- apps/wolfsshd/test/sshd_stderr_eof_test.sh | 38 ++++++++++++++-------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index eab454bea..e7279144c 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -587,7 +587,7 @@ else run_test "sshd_privdrop_fail_test.sh" else printf "Skipping tests that need to setup local SSHD\n" - SKIPPED=$((SKIPPED+10)) + SKIPPED=$((SKIPPED+12)) fi # these tests run with X509 sshd-config loaded diff --git a/apps/wolfsshd/test/sshd_stderr_eof_test.sh b/apps/wolfsshd/test/sshd_stderr_eof_test.sh index c9cffdd96..00c0ca7b6 100755 --- a/apps/wolfsshd/test/sshd_stderr_eof_test.sh +++ b/apps/wolfsshd/test/sshd_stderr_eof_test.sh @@ -49,7 +49,28 @@ if command -v timeout >/dev/null 2>&1; then fi source ./start_sshd.sh -cat < sshd_config_test_stderr_eof + +# The runner leases a port block per run so two runs can share a host, and +# fixed names in the checkout are the other half of that: a second run +# overwrites this one's config and payload, and whichever finishes first +# removes them from under the other, which then reports a short transfer that +# never happened. Everything this test writes goes in a directory of its own. +TEST_TMP=`mktemp -d 2>/dev/null` || TEST_TMP=`mktemp -d -t stderreof` +if [ -z "$TEST_TMP" ] || [ ! -d "$TEST_TMP" ]; then + echo "Failed to create a temp dir" + exit 1 +fi +TEST_CONFIG="$TEST_TMP/sshd_config_test_stderr_eof" +TEST_FILE="$TEST_TMP/stderr-eof-test.txt" +TEST_RESULT_FILE="$TEST_TMP/stderr-eof-test-result.txt" + +# The payload is 16 MB and the daemon is shared with the rest of the run, so +# neither may be left behind by an interrupted pass. Installed before the +# daemon starts so a failure in between is covered too; stop_wolfsshd is +# idempotent, so the explicit call at the end still stands. +trap 'rm -rf "$TEST_TMP"; stop_wolfsshd' EXIT + +cat < "$TEST_CONFIG" Port $TEST_PORT Protocol 2 LoginGraceTime 600 @@ -62,7 +83,7 @@ HostKey $PWD/../../../keys/server-key.pem AuthorizedKeysFile $PWD/authorized_keys_test CONF -start_wolfsshd "sshd_config_test_stderr_eof" +start_wolfsshd "$TEST_CONFIG" if [ -z "$PID" ]; then echo "Failed to start wolfsshd" exit 1 @@ -74,17 +95,6 @@ PRIVATE_KEY="./keys/hansel-key-ecc.der" PUBLIC_KEY="./keys/hansel-key-ecc.pub" PWD=`pwd` -# Named in full because the trap below outlives the cd back into the test -# directory, and bash keeps PWD in step with that cd whatever this script -# assigned to it. -TEST_FILE="$PWD/stderr-eof-test.txt" -TEST_RESULT_FILE="$PWD/stderr-eof-test-result.txt" - -# The scratch file is 16 MB and the daemon is shared with the rest of the run, -# so neither may be left behind by an interrupted pass. stop_wolfsshd is -# idempotent, so the explicit call below still stands. -trap 'rm -f "$TEST_FILE" "$TEST_RESULT_FILE"; stop_wolfsshd' EXIT - head -c $TEST_SIZE /dev/urandom > "$TEST_FILE" EXPECTED=`wc -c < "$TEST_FILE"` @@ -116,7 +126,7 @@ for i in `seq 1 $TEST_ITERS`; do fi done -rm -f "$TEST_FILE" "$TEST_RESULT_FILE" +rm -rf "$TEST_TMP" cd apps/wolfsshd/test stop_wolfsshd