Skip to content

Commit 0ab1756

Browse files
ejohnstownejohnstown
committed
Handle the EOF status in apps and examples
Every in-tree caller of wolfSSH_worker() now recognises a peer half-close. wolfsshd's shell loop and both echoservers need it: all three ladders end in "else if (rc != WS_WANT_READ) break", and wolfsshd's reaches kill(childPid, SIGKILL), so without it a client half-close kills the command it just finished feeding. - wolfsshd closes the child's stdin off the channel's own EOF state instead of off a worker return of zero, which no longer happens on a half-close. - The echoservers answer the half-close off wolfSSH_ChannelGetEof() rather than the WS_EOF status: the flush inside wolfSSH_worker() can supersede that status, and it is raised once. They hand back the backlog first, finish a short send, and only send the EOF once the channel is empty. Answering is not conditional on the shell build, where an echo session is the default. - The SFTP loops peek before leaving, so a half-close with requests still buffered is served rather than dropped, and they report an ordinary session end as success. - The clients -- examples/client, scpclient, sftpclient, apps/wolfssh -- treat it as the graceful case instead of an error. apps/wolfssh counts it as a finished flush as well, since one worker pass can drain the queue and consume the peer's EOF together. - portfwd relays it to the local socket with shutdown(SHUT_WR) so a local reader waiting on end-of-input returns, once the backlog has genuinely been handed over: a read cut short by a rekey leaves the half-close for a later pass. - The Windows half of wolfsshd does not answer with an EOF of its own. That latches eofTxd and the child's remaining output would be refused, which is the defect this series removes from the library. - The mplabx port drains before tearing down, the way its SFTP read path already did; its worker arm was unreachable for a half-close until now.
1 parent 27e36cf commit 0ab1756

11 files changed

Lines changed: 432 additions & 39 deletions

File tree

‎apps/wolfssh/wolfssh.c‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ static int FlushQueuedSend(WOLFSSH* ssh, wolfSSL_Mutex* lock)
338338
* conversation is for the reader to sort out. A rekey started on the way
339339
* through is the reader's as well, the send itself went out. */
340340
if (ret == WS_WANT_READ || ret == WS_CHAN_RXD || ret == WS_EXTDATA
341-
|| ret == WS_REKEYING) {
341+
|| ret == WS_REKEYING || ret == WS_EOF) {
342342
ret = WS_SUCCESS;
343343
}
344344

@@ -1362,14 +1362,17 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args)
13621362
ret = WS_SUCCESS;
13631363
}
13641364
}
1365-
else if (ret != WS_CHANNEL_CLOSED && ret != WS_WANT_READ) {
1365+
else if (ret != WS_CHANNEL_CLOSED && ret != WS_WANT_READ
1366+
&& ret != WS_EOF) {
13661367
WLOG(WS_LOG_DEBUG, "Sending the shutdown messages failed.");
13671368
}
13681369

1369-
if (ret == WS_CHANNEL_CLOSED || ret == WS_WANT_READ) {
1370-
/* Shutting down. The channel closing isn't a fail, and neither
1371-
* is the peer having nothing ready on this non-blocking socket;
1372-
* either way there is nothing left to wait for. */
1370+
if (ret == WS_CHANNEL_CLOSED || ret == WS_WANT_READ
1371+
|| ret == WS_EOF) {
1372+
/* Shutting down. The channel closing or the peer's EOF isn't a
1373+
* fail, and neither is the peer having nothing ready on this
1374+
* non-blocking socket; either way there is nothing left to wait
1375+
* for. */
13731376
ret = WS_SUCCESS;
13741377
}
13751378
else if (ret != WS_SUCCESS) {

‎apps/wolfsshd/test/run_all_sshd_tests.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ test_cases=(
1010
"sshd_bad_sftp_test.sh"
1111
"sshd_scp_fail.sh"
1212
"sshd_term_close_test.sh"
13+
"sshd_stdin_eof_test.sh"
1314
"ssh_kex_algos.sh"
1415
)
1516

‎apps/wolfsshd/test/sshd_stdin_eof_test.sh‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
#!/bin/bash
2+
# bash, unlike the rest of this directory: the option array and PIPESTATUS
3+
# below both need it.
4+
5+
# A client that half-closes its stdin sends SSH_MSG_CHANNEL_EOF and waits for
6+
# the command to finish. wolfSSHd must close the write end of the child's stdin
7+
# pipe so a command reading to end-of-input returns, and it must hand over
8+
# everything the peer sent before it does.
9+
#
10+
# Needs the OpenSSH client; the wolfSSH example client does not half-close.
11+
12+
if [ -z "$1" ] || [ -z "$2" ]; then
13+
echo "expecting host and port as arguments"
14+
echo "./sshd_stdin_eof_test.sh 127.0.0.1 22222"
15+
exit 1
16+
fi
17+
18+
HOST="$1"
19+
PORT="$2"
20+
USER_NAME="${3:-`whoami`}"
21+
22+
command -v ssh >/dev/null 2>&1 || {
23+
echo "ssh not found, skipping"
24+
exit 77
25+
}
26+
27+
# The RESULT==124 assertions below are the point of the test.
28+
command -v timeout >/dev/null 2>&1 || {
29+
echo "timeout not found, skipping"
30+
exit 77
31+
}
32+
33+
# ssh refuses a group/world readable identity file.
34+
KEYDIR=`mktemp -d 2>/dev/null` || KEYDIR=`mktemp -d -t sshdeof`
35+
if [ -z "$KEYDIR" ] || [ ! -d "$KEYDIR" ]; then
36+
echo "could not create temp dir"
37+
exit 1
38+
fi
39+
trap 'rm -rf "$KEYDIR"' EXIT
40+
41+
cp ../../../keys/hansel-key-ecc.pem "$KEYDIR/id_ecdsa" || exit 1
42+
chmod 600 "$KEYDIR/id_ecdsa"
43+
44+
SSH_OPTS=(-i "$KEYDIR/id_ecdsa" -p "$PORT"
45+
-o IdentitiesOnly=yes
46+
-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null
47+
-o PreferredAuthentications=publickey -o PasswordAuthentication=no
48+
-o BatchMode=yes -o ConnectTimeout=5 -o LogLevel=ERROR)
49+
50+
# An identity the client cannot load, or a host it cannot reach, is not this
51+
# test's subject. sshd_exec_test.sh runs ahead of this one and owns a server
52+
# that cannot run commands at all.
53+
if ! timeout 20 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" true >/dev/null 2>&1
54+
then
55+
echo "no session with these options, skipping"
56+
exit 77
57+
fi
58+
59+
# Case 1: a small input through 'sort'. 'sort' emits nothing until
60+
# end-of-input, so a missed EOF is the timeout and a killed or hung child is
61+
# empty output -- neither can pass by winning a race the way a streaming 'cat'
62+
# can. The input is unsorted so the comparison also proves the remote command
63+
# ran rather than the input being echoed back.
64+
printf 'charlie\nalpha\nbravo\n' > "$KEYDIR/in.txt"
65+
66+
# wolfsshd runs the user's login shell, so its startup files can print on
67+
# either stream: stderr goes to a file, and stdout is filtered to the lines
68+
# the command itself produced.
69+
OUT=`timeout 20 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" 'sort' \
70+
< "$KEYDIR/in.txt" 2> "$KEYDIR/in.err" \
71+
| grep -E '^(alpha|bravo|charlie)$'`
72+
RESULT=${PIPESTATUS[0]}
73+
74+
if [ "$RESULT" == 124 ]; then
75+
echo "session did not end after the client half-closed its stdin"
76+
cat "$KEYDIR/in.err"
77+
exit 1
78+
fi
79+
80+
if [ "$RESULT" != 0 ]; then
81+
echo "ssh failed with $RESULT"
82+
cat "$KEYDIR/in.err"
83+
exit 1
84+
fi
85+
86+
if [ "$OUT" != "`sort "$KEYDIR/in.txt"`" ]; then
87+
echo "unexpected output from the remote command"
88+
echo "$OUT"
89+
cat "$KEYDIR/in.err"
90+
exit 1
91+
fi
92+
93+
# Case 2: the same half-close with the send window full. The reader below
94+
# stalls, so the client stops draining, the server's window to the peer fills
95+
# while the client is still sending, and the EOF arrives with channel data
96+
# still buffered on the server. Data held back that way has to be handed to
97+
# the child before its stdin closes: dropping it truncates the output, and
98+
# never handing it over leaves 'cat' waiting on a stdin that never closes.
99+
awk 'BEGIN { for (i = 0; i < 200000; i++)
100+
printf "%08d one two three four five six seven\n", (i * 48271) % 99991
101+
}' > "$KEYDIR/big.txt"
102+
103+
timeout 90 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" 'cat' \
104+
< "$KEYDIR/big.txt" 2> "$KEYDIR/big.err" \
105+
| { sleep 5; cat; } > "$KEYDIR/big.out"
106+
RESULT=${PIPESTATUS[0]}
107+
108+
if [ "$RESULT" == 124 ]; then
109+
echo "session did not end after a half-close with the window full"
110+
cat "$KEYDIR/big.err"
111+
exit 1
112+
fi
113+
114+
if [ "$RESULT" != 0 ]; then
115+
echo "ssh failed with $RESULT"
116+
cat "$KEYDIR/big.err"
117+
exit 1
118+
fi
119+
120+
SENT=`wc -c < "$KEYDIR/big.txt"`
121+
GOT=`wc -c < "$KEYDIR/big.out"`
122+
123+
if [ "$GOT" -lt "$SENT" ] \
124+
|| ! tail -c "$SENT" "$KEYDIR/big.out" | cmp -s - "$KEYDIR/big.txt"
125+
then
126+
echo "the remote command did not see all of the input"
127+
echo "sent $SENT bytes, got $GOT bytes"
128+
cat "$KEYDIR/big.err"
129+
exit 1
130+
fi
131+
132+
exit 0

‎apps/wolfsshd/wolfsshd.c‎

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -908,6 +908,8 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
908908
* if there is still pending sends */
909909
}
910910
if (error == WS_EOF) {
911+
/* An ordinary session end, not a failure. */
912+
ret = 0;
911913
break;
912914
}
913915
}
@@ -934,10 +936,19 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
934936
continue;
935937
}
936938

939+
/* Drain what is buffered first. A rekey is not a drained
940+
* channel: peek reports it without looking. */
937941
if (error == WS_EOF) {
938-
break;
942+
int peekRet = wolfSSH_stream_peek(ssh, NULL, 1);
943+
944+
if (peekRet != WS_REKEYING && peekRet <= 0) {
945+
/* An ordinary session end, not a failure. */
946+
ret = 0;
947+
break;
948+
}
939949
}
940-
if (ret != WS_SUCCESS && ret != WS_CHAN_RXD) {
950+
if (ret != WS_SUCCESS && ret != WS_CHAN_RXD
951+
&& ret != WS_EOF) {
941952
/* If not successful and no channel data, leave. */
942953
break;
943954
}
@@ -954,8 +965,11 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
954965
error == WS_CHAN_RXD || error == WS_REKEYING ||
955966
error == WS_WINDOW_FULL)
956967
ret = error;
957-
if (error == WS_EOF)
968+
if (error == WS_EOF) {
969+
/* An ordinary session end, not a failure. */
970+
ret = 0;
958971
break;
972+
}
959973
continue;
960974
}
961975
else if (ret == WS_REKEYING) {
@@ -964,8 +978,11 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
964978
}
965979
else if (ret < 0) {
966980
error = wolfSSH_get_error(ssh);
967-
if (error == WS_EOF)
981+
if (error == WS_EOF) {
982+
/* An ordinary session end, not a failure. */
983+
ret = 0;
968984
break;
985+
}
969986
}
970987

971988
if (ret == WS_FATAL_ERROR && error == 0) {
@@ -1325,6 +1342,19 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
13251342
else if (rc == WS_CHANNEL_CLOSED) {
13261343
continue;
13271344
}
1345+
else if (rc == WS_EOF) {
1346+
/* The peer is done sending. No EOF of ours here: it
1347+
* latches eofTxd and the child's remaining console
1348+
* output would then be refused, which both send sites
1349+
* below treat as fatal. wolfSSH_shutdown() sends it at
1350+
* teardown, as the POSIX copy relies on. Closing the
1351+
* write end of the child's stdin is still owed on this
1352+
* platform, and so is the per-pass drain: ptyIn is the
1353+
* terminal-resize context, and this copy reads into
1354+
* shellBuffer, which the windowFull resend owes the
1355+
* peer. Both want fixing where they can be tested. */
1356+
continue;
1357+
}
13281358
else if (rc != WS_WANT_READ) {
13291359
break;
13301360
}
@@ -1518,8 +1548,10 @@ static int SHELL_FlushOut(WOLFSSH* ssh, WS_SOCKET_T sshFd, word32 channelId,
15181548
if (wolfSSH_worker(ssh, NULL) < 0) {
15191549
int err = wolfSSH_get_error(ssh);
15201550

1551+
/* A peer EOF during the final flush is expected. */
15211552
if (err != WS_WANT_READ && err != WS_WANT_WRITE &&
1522-
err != WS_CHAN_RXD && err != WS_REKEYING) {
1553+
err != WS_CHAN_RXD && err != WS_REKEYING &&
1554+
err != WS_EOF) {
15231555
wolfSSH_Log(WS_LOG_ERROR,
15241556
"[SSHD] Issue draining connection on final flush");
15251557
return -1;
@@ -1824,11 +1856,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
18241856
word32 shellChannelId = 0;
18251857
WOLFSSH_CHANNEL* shellChannel;
18261858

1827-
/* Name the session channel off the channel list rather than trusting
1828-
* DEFAULT_NEXT_CHANNEL to be 0, which a build can override. It is the
1829-
* only channel open at this point; the agent channel comes later. The
1830-
* loop below closes the child's stdin off this channel, so a wrong id
1831-
* there drops the peer's input instead of handing it over. */
1859+
/* Off the channel list, not DEFAULT_NEXT_CHANNEL, which a build can
1860+
* override. The session channel is the only one open here. lastRxId is
1861+
* no use: no request path sets it. A wrong id drops the peer's input. */
18321862
shellChannel = wolfSSH_ChannelNext(ssh, NULL);
18331863
if (shellChannel == NULL || wolfSSH_ChannelGetId(shellChannel,
18341864
&shellChannelId, WS_CHANNEL_ID_SELF) != WS_SUCCESS) {
@@ -1960,6 +1990,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
19601990
peerConnected = 0;
19611991
continue;
19621992
}
1993+
else if (rc == WS_EOF) {
1994+
/* Half-close, handled below. */
1995+
}
19631996
else if (rc == WS_WANT_WRITE) {
19641997
wantWrite = 1;
19651998
continue;
@@ -2716,7 +2749,7 @@ static void* HandleConnection(void* arg)
27162749
error = wolfSSH_get_error(ssh);
27172750

27182751
/* peer successfully closed down gracefully */
2719-
if (ret == WS_CHANNEL_CLOSED) {
2752+
if (ret == WS_CHANNEL_CLOSED || ret == WS_EOF) {
27202753
ret = 0;
27212754
break;
27222755
}

‎examples/client/client.c‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1179,7 +1179,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args)
11791179
if (ret <= 0) {
11801180
ret = wolfSSH_get_error(ssh);
11811181
if (ret != WS_WANT_READ && ret != WS_WANT_WRITE &&
1182-
ret != WS_CHAN_RXD) {
1182+
ret != WS_CHAN_RXD && ret != WS_EOF) {
11831183
ClientFreeBuffers(pubKeyName, privKeyName, NULL);
11841184
wolfSSH_free(ssh);
11851185
wolfSSH_CTX_free(ctx);
@@ -1198,7 +1198,9 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args)
11981198
#endif
11991199
}
12001200
ret = wolfSSH_shutdown(ssh);
1201-
/* do not continue on with shutdown process if peer already disconnected */
1201+
/* do not continue on with shutdown process if peer already disconnected.
1202+
* A peer EOF is not a disconnect: the channel is still open and its close
1203+
* is still owed, so the drain below is exactly what is wanted. */
12021204
if (ret != WS_SOCKET_ERROR_E && wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E
12031205
&& wolfSSH_get_error(ssh) != WS_CHANNEL_CLOSED) {
12041206
if (ret != WS_SUCCESS) {
@@ -1209,7 +1211,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args)
12091211
}
12101212
ret = wolfSSH_worker(ssh, NULL);
12111213
if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E &&
1212-
ret != WS_CHANNEL_CLOSED) {
1214+
ret != WS_CHANNEL_CLOSED && ret != WS_EOF) {
12131215
ClientFreeBuffers(pubKeyName, privKeyName, NULL);
12141216
wolfSSH_free(ssh);
12151217
wolfSSH_CTX_free(ctx);
@@ -1226,7 +1228,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args)
12261228
wolfSSH_free(ssh);
12271229
wolfSSH_CTX_free(ctx);
12281230
if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E &&
1229-
ret != WS_CHANNEL_CLOSED) {
1231+
ret != WS_CHANNEL_CLOSED && ret != WS_EOF) {
12301232
err_sys("Closing client stream failed");
12311233
}
12321234

0 commit comments

Comments
 (0)