diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index b2efeffcf..e7279144c 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" @@ -586,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 new file mode 100755 index 000000000..00c0ca7b6 --- /dev/null +++ b/apps/wolfsshd/test/sshd_stderr_eof_test.sh @@ -0,0 +1,133 @@ +#!/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 + +# 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 + +# 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 +PermitRootLogin yes +PasswordAuthentication yes +PermitEmptyPasswords no +UsePrivilegeSeparation no +UseDNS no +HostKey $PWD/../../../keys/server-key.pem +AuthorizedKeysFile $PWD/authorized_keys_test +CONF + +start_wolfsshd "$TEST_CONFIG" +if [ -z "$PID" ]; then + echo "Failed to start wolfsshd" + exit 1 +fi +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 > "$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. + $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; } > "$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 < "$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))" + RESULT=1 + break + fi +done + +rm -rf "$TEST_TMP" +cd apps/wolfsshd/test +stop_wolfsshd + +exit $RESULT diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 6c3ba5623..9d65c570e 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,17 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, noWait.tv_usec = 0; timeout = &noWait; } + 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; + } rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, timeout); if (rc == -1) { @@ -3361,13 +3378,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, @@ -3410,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; } } @@ -3457,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; } }