From a14e6d0f3cdc9f6a2226fab223ab4a69887891c5 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Mon, 14 Sep 2026 15:58:28 +0900 Subject: [PATCH] ssh, apps, examples: take the worker's status from its return - wolfSSH_OutputPending() moves from wolfssh/internal.h to wolfssh/ssh.h as a WOLFSSH_API taking a const WOLFSSH*, defined in src/ssh.c beside the other public calls. - wolfSSH_worker() puts the send's code in the return in place of an event when the flush fails outright, and gates its flush on wolfSSH_OutputPending(). wolfssh/ssh.h states both. - Both wolfsshd shell loops, both echoservers and ReceiveScpMessage() dispatch on wolfSSH_worker()'s return, and call wolfSSH_get_error() only to tell a transient failure from a terminal one. The POSIX wolfsshd loop sets wantWrite from wolfSSH_OutputPending(). - Five worker tests expect the send's code where they expected the event, and tests/testsuite.c calls wolfSSH_OutputPending(). --- apps/wolfsshd/wolfsshd.c | 51 +++++++++---------- examples/echoserver/echoserver.c | 15 +++--- .../wolfssh_echoserver/main/echoserver.c | 15 +++--- src/internal.c | 4 -- src/ssh.c | 13 ++++- src/wolfscp.c | 7 +-- tests/testsuite.c | 7 +++ tests/unit.c | 37 +++++++------- wolfssh/internal.h | 1 - wolfssh/ssh.h | 15 ++++-- 10 files changed, 90 insertions(+), 75 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index eb857e18..a3d0389d 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -2277,12 +2277,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, agent. */ cnt_r = wolfSSH_worker(ssh, &lastChannel); if (cnt_r < 0) { - rc = wolfSSH_get_error(ssh); - if (cnt_r == WS_CHAN_RXD || cnt_r == WS_CHANNEL_CLOSED - || cnt_r == WS_EOF) { - rc = cnt_r; - } - if (rc == WS_CHAN_RXD) { + if (cnt_r == WS_CHAN_RXD) { if (lastChannel == shellChannelId) { cnt_r = wolfSSH_ChannelIdRead(ssh, shellChannelId, shellBuffer, @@ -2299,10 +2294,10 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } } } - else if (rc == WS_CHANNEL_CLOSED) { + else if (cnt_r == WS_CHANNEL_CLOSED) { continue; } - else if (rc == WS_EOF) { + else if (cnt_r == WS_EOF) { /* The peer is done sending. No EOF of ours here: it * latches eofTxd and the child's remaining console * output would then be refused, which both send sites @@ -2315,7 +2310,12 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, * peer. Both want fixing where they can be tested. */ continue; } - else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) { + else if (cnt_r == WS_WANT_WRITE) { + /* Transient; the queue drives the write side. */ + } + else if (cnt_r != WS_FATAL_ERROR + || (wolfSSH_get_error(ssh) != WS_WANT_READ + && wolfSSH_get_error(ssh) != WS_WANT_WRITE)) { break; } } @@ -2979,20 +2979,14 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, the channel itself, so the id the worker would report here is not needed. */ cnt_r = wolfSSH_worker(ssh, NULL); + if (wolfSSH_OutputPending(ssh)) { + wantWrite = 1; + } if (cnt_r < 0) { - rc = wolfSSH_get_error(ssh); - /* Take the owed write before the event overwrites rc. */ - if (rc == WS_WANT_WRITE) { - wantWrite = 1; - } - if (cnt_r == WS_CHAN_RXD || cnt_r == WS_CHANNEL_CLOSED - || cnt_r == WS_EOF) { - rc = cnt_r; - } - if (rc == WS_CHAN_RXD) { + if (cnt_r == WS_CHAN_RXD) { /* Arrival only; the drain below owns the read. */ } - else if (rc == WS_CHANNEL_CLOSED) { + else if (cnt_r == WS_CHANNEL_CLOSED) { /* The channel is retired, so nothing more can reach the * child and the drain below is skipped on this pass. * Close its stdin here or it blocks forever on input @@ -3007,17 +3001,18 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, peerConnected = 0; continue; } - else if (rc == WS_EOF) { + else if (cnt_r == WS_EOF) { /* Half-close, handled below. */ } - else if (rc == WS_WANT_WRITE) { - /* Recorded above; the channel drain below still runs. */ - } - else if (rc == WS_REKEYING) { - wantWrite = 1; + else if (cnt_r == WS_REKEYING) { continue; } - else if (rc != WS_WANT_READ) { + else if (cnt_r == WS_WANT_WRITE) { + /* Transient; the queue drives the write side. */ + } + else if (cnt_r != WS_FATAL_ERROR + || (wolfSSH_get_error(ssh) != WS_WANT_READ + && wolfSSH_get_error(ssh) != WS_WANT_WRITE)) { /* unexpected error, kill off child process */ kill(childPid, SIGKILL); break; @@ -3046,7 +3041,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, break; /* The credit that read issued may still be queued. */ - if (wolfSSH_get_error(ssh) == WS_WANT_WRITE) + if (wolfSSH_OutputPending(ssh)) wantWrite = 1; childInIdx = 0; diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index e45ee806..2212b1e7 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -1035,11 +1035,9 @@ static int ssh_worker(thread_ctx_t* threadCtx) channel. The additional channel is only used with the agent. */ cnt_r = wolfSSH_worker(ssh, &lastChannel); - rc = wolfSSH_get_error(ssh); - if (cnt_r == WS_CHAN_RXD || cnt_r == WS_REKEYING - || cnt_r == WS_CHANNEL_CLOSED || cnt_r == WS_EOF) { - rc = cnt_r; - } + /* The channel reads below overwrite cnt_r with a byte + * count, so keep the worker's status. */ + rc = cnt_r; /* The peer is done sending: hand back the backlog and answer * its EOF, since the library no longer answers for us. Off @@ -1242,7 +1240,12 @@ static int ssh_worker(thread_ctx_t* threadCtx) * above, which has already run this pass. */ continue; } - else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) { + else if (rc == WS_WANT_WRITE) { + /* Transient; the queue drives the write side. */ + } + else if (rc != WS_FATAL_ERROR + || (wolfSSH_get_error(ssh) != WS_WANT_READ + && wolfSSH_get_error(ssh) != WS_WANT_WRITE)) { #ifdef SHELL_DEBUG printf("Break:read sshFd returns %d: errno =%x\n", cnt_r, errno); diff --git a/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c b/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c index 0e8db096..c8c1fdb7 100644 --- a/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c +++ b/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c @@ -995,11 +995,9 @@ static int ssh_worker(thread_ctx_t* threadCtx) channel. The additional channel is only used with the agent. */ cnt_r = wolfSSH_worker(ssh, &lastChannel); - rc = wolfSSH_get_error(ssh); - if (cnt_r == WS_CHAN_RXD || cnt_r == WS_REKEYING - || cnt_r == WS_CHANNEL_CLOSED || cnt_r == WS_EOF) { - rc = cnt_r; - } + /* The channel reads below overwrite cnt_r with a byte + * count, so keep the worker's status. */ + rc = cnt_r; /* The peer is done sending: hand back the backlog and answer * its EOF, since the library no longer answers for us. Off @@ -1169,7 +1167,12 @@ static int ssh_worker(thread_ctx_t* threadCtx) * above, which has already run this pass. */ continue; } - else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) { + else if (rc == WS_WANT_WRITE) { + /* Transient; the queue drives the write side. */ + } + else if (rc != WS_FATAL_ERROR + || (wolfSSH_get_error(ssh) != WS_WANT_READ + && wolfSSH_get_error(ssh) != WS_WANT_WRITE)) { #ifdef SHELL_DEBUG printf("Break:read sshFd returns %d: errno =%x\n", cnt_r, errno); diff --git a/src/internal.c b/src/internal.c index 4831dc59..9b400ad4 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5578,10 +5578,6 @@ int wolfSSH_SendPacket(WOLFSSH* ssh) } -int wolfSSH_OutputPending(WOLFSSH* ssh) -{ - return (ssh != NULL && ssh->outputBuffer.length > ssh->outputBuffer.idx); -} static int GetInputData(WOLFSSH* ssh, word32 size) diff --git a/src/ssh.c b/src/ssh.c index 4ee9c6ae..b1915fe9 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -4631,7 +4631,7 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) /* Flush queued output whatever DoReceive() made of the socket, since an * idle receive reports WS_FATAL_ERROR. !ssh->disconnected gates it. */ - if (ssh != NULL && !ssh->disconnected && ssh->outputBuffer.length != 0) { + if (wolfSSH_OutputPending(ssh) && !ssh->disconnected) { int rxErr = ssh->error; sendRet = wolfSSH_SendPacket(ssh); @@ -4639,6 +4639,11 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) if (ret == WS_SUCCESS) { ret = sendRet; } + else if (sendRet != WS_WANT_WRITE && ret != WS_CHANNEL_CLOSED + && ret != WS_FATAL_ERROR) { + /* The transport is gone, so the event it came with is moot. */ + ret = sendRet; + } else if ((ret == WS_CHANNEL_CLOSED && sendRet != WS_WANT_WRITE) || (ret == WS_FATAL_ERROR && rxErr != WS_WANT_READ)) { /* A failed receive outranks the flush, and so does a close @@ -4691,6 +4696,12 @@ int wolfSSH_GetLastRxId(WOLFSSH* ssh, word32* channelId) } +int wolfSSH_OutputPending(const WOLFSSH* ssh) +{ + return (ssh != NULL && ssh->outputBuffer.length > ssh->outputBuffer.idx); +} + + #ifdef WOLFSSH_FWD int wolfSSH_CTX_SetFwdCb(WOLFSSH_CTX* ctx, diff --git a/src/wolfscp.c b/src/wolfscp.c index 4a70816a..cbbeb704 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -1793,12 +1793,7 @@ int ReceiveScpMessage(WOLFSSH* ssh) if (err < 0) { int rc; - rc = wolfSSH_get_error(ssh); - if (err == WS_CHAN_RXD || err == WS_EXTDATA - || err == WS_CHANNEL_CLOSED) { - rc = err; - } - switch (rc) { + switch (err) { case WS_CHAN_RXD: sz = wolfSSH_ChannelIdRead(ssh, lastChannel, buf + ssh->scpRecvMsgSz, diff --git a/tests/testsuite.c b/tests/testsuite.c index 77764bdb..48ed5953 100644 --- a/tests/testsuite.c +++ b/tests/testsuite.c @@ -241,6 +241,13 @@ int wolfSSH_TestsuiteTest(int argc, char** argv) wolfSSH_Init(); + /* Linked against the installed library, so this also proves + * wolfSSH_OutputPending() is exported and not hidden. */ + if (wolfSSH_OutputPending(NULL) != 0) { + fprintf(stderr, "wolfSSH_OutputPending(NULL) was not zero\n"); + return EXIT_FAILURE; + } + #if defined(FIPS_VERSION_GE) && FIPS_VERSION_GE(5,2) { int i; diff --git a/tests/unit.c b/tests/unit.c index 4a0d5221..0d9c8995 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -6769,9 +6769,9 @@ done: return result; } -/* Channel data arrives and the flush fails on the same call. ret carries - * WS_CHAN_RXD so the caller reads the data, and ssh->error carries the send - * failure, which is the rule wolfssh/ssh.h states for wolfSSH_worker(). */ +/* Channel data arrives and the flush fails on the same call. The send's + * code takes ret, since the data is still buffered but the transport is + * gone. */ static int test_WorkerChanRxdSurfacesSendError(void) { WOLFSSH_CTX* ctx = NULL; @@ -6811,7 +6811,7 @@ static int test_WorkerChanRxdSurfacesSendError(void) wolfSSH_SetIOSend(ctx, ConnResetIoSend); ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_CHAN_RXD) { result = -1755; goto done; } + if (ret != WS_SOCKET_ERROR_E) { result = -1755; goto done; } if (wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E) { result = -1756; goto done; @@ -6895,9 +6895,8 @@ done: } -/* Extended data arrives and the flush fails on the same call. Neither the - * close nor the receive-failure rule applies, so ret keeps WS_EXTDATA and - * ssh->error carries the send failure. */ +/* Extended data arrives and the flush fails on the same call. The send's + * code takes ret, and channelId is left alone with it. */ static int test_WorkerExtDataSurfacesSendError(void) { WOLFSSH_CTX* ctx = NULL; @@ -6938,8 +6937,8 @@ static int test_WorkerExtDataSurfacesSendError(void) wolfSSH_SetIOSend(ctx, ConnResetIoSend); ret = wolfSSH_worker(ssh, &reportedId); - if (ret != WS_EXTDATA) { result = -1835; goto done; } - if (reportedId != ch->channel) { result = -1836; goto done; } + if (ret != WS_SOCKET_ERROR_E) { result = -1835; goto done; } + if (reportedId != 0xFFFFFFFF) { result = -1836; goto done; } if (wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E) { result = -1837; goto done; @@ -6947,7 +6946,7 @@ static int test_WorkerExtDataSurfacesSendError(void) /* A reset does not discard, so the bytes stay owed. */ if (ssh->outputBuffer.length == 0) { result = -1838; goto done; } - /* The stderr is still there to drain, which is why ret kept it. */ + /* The stderr is still there to drain, off the channel rather than ret. */ if (ch->extDataBuffer.length - ch->extDataBuffer.idx != 10) { result = -1839; goto done; @@ -6963,8 +6962,8 @@ done: } -/* The peer half-closes and the flush fails on the same call. ret keeps - * WS_EOF and ssh->error carries the send failure. */ +/* The peer half-closes and the flush fails on the same call. The send's + * code takes ret; the half-close is read off the channel instead. */ static int test_WorkerEofSurfacesSendError(void) { WOLFSSH_CTX* ctx = NULL; @@ -7005,8 +7004,8 @@ static int test_WorkerEofSurfacesSendError(void) wolfSSH_SetIOSend(ctx, ConnResetIoSend); ret = wolfSSH_worker(ssh, &reportedId); - if (ret != WS_EOF) { result = -1845; goto done; } - if (reportedId != ch->channel) { result = -1846; goto done; } + if (ret != WS_SOCKET_ERROR_E) { result = -1845; goto done; } + if (reportedId != 0xFFFFFFFF) { result = -1846; goto done; } if (wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E) { result = -1847; goto done; @@ -7083,8 +7082,8 @@ done: /* The same pass with a rekey in flight. WS_REKEYING would tell the caller to - * keep turning the crank, so a flush that hard-failed keeps ssh->error and - * the rekey mask stands down. */ + * keep turning the crank, so a flush that hard-failed takes ret and the rekey + * mask stands down. */ static int test_WorkerKeyingSurfacesSendError(void) { WOLFSSH_CTX* ctx = NULL; @@ -7124,7 +7123,7 @@ static int test_WorkerKeyingSurfacesSendError(void) ssh->isKeying = WOLFSSH_SELF_IS_KEYING; ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_CHAN_RXD) { result = -1805; goto done; } + if (ret != WS_SOCKET_ERROR_E) { result = -1805; goto done; } if (wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E) { result = -1806; goto done; @@ -7201,7 +7200,7 @@ done: } /* A send that fails with WS_CBIO_ERR_GENERAL discards the output buffer, so - * no later call retries the flush. ssh->error has to keep the send failure + * no later call retries the flush. The send failure has to reach the caller * even though the receive reported channel data. */ static int test_WorkerDiscardedFlushKeepsError(void) { @@ -7242,7 +7241,7 @@ static int test_WorkerDiscardedFlushKeepsError(void) wolfSSH_SetIOSend(ctx, FailIoSend); ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_CHAN_RXD) { result = -1765; goto done; } + if (ret != WS_SOCKET_ERROR_E) { result = -1765; goto done; } /* Nothing is left to flush, so this is the only report there will be. */ if (ssh->outputBuffer.length != 0) { result = -1766; goto done; } if (wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E) { diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 16c65325..18d753db 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1701,7 +1701,6 @@ WOLFSSH_LOCAL int SendAfterDisconnect(WOLFSSH* ssh); WOLFSSH_LOCAL int DoReceive(WOLFSSH* ssh); WOLFSSH_LOCAL int DoProtoId(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_SendPacket(WOLFSSH* ssh); -WOLFSSH_LOCAL int wolfSSH_OutputPending(WOLFSSH* ssh); /* Will the packet just framed reach the peer? A completed flush says so; the * return does not, since the highwater callback runs after the last byte is * out and fails with the same codes a lost send does. Take flushes from diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index ffef6e6c..e2e4b4c7 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -96,10 +96,14 @@ WOLFSSH_API void wolfSSH_free(WOLFSSH* ssh); * neither as a substitute for the other. * A want is transient either way: call again. A caller that tolerates only * WS_WANT_READ drops live sessions, since a queued write reports - * WS_WANT_WRITE. - * Any other code is an error: WS_BAD_ARGUMENT, or WS_FATAL_ERROR with the - * cause in wolfSSH_get_error() -- WS_DISCONNECT for the peer's disconnect, - * which is how most sessions end. + * WS_WANT_WRITE. A send that fails outright takes the return instead, since + * the event it arrived with is moot once the transport is gone; only a + * WS_CHANNEL_CLOSED keeps the return there. + * Any other code is an error, either in the return itself or as + * WS_FATAL_ERROR with the cause in wolfSSH_get_error() -- WS_DISCONNECT for + * the peer's disconnect, which is how most sessions end. + * To ask whether a write is still owed, call wolfSSH_OutputPending() rather + * than reading a status: it answers after any return, including a success. * * For WS_CHAN_RXD, WS_EXTDATA, WS_EOF, WS_SUCCESS and a WS_REKEYING that * displaced one of those, channelId (when not NULL) names the channel the @@ -111,6 +115,9 @@ WOLFSSH_API void wolfSSH_free(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_worker(WOLFSSH* ssh, word32* channelId); WOLFSSH_API int wolfSSH_GetLastRxId(WOLFSSH* ssh, word32* channelId); +/* Returns nonzero if a write is still owed. Session state */ +WOLFSSH_API int wolfSSH_OutputPending(const WOLFSSH* ssh); + WOLFSSH_API int wolfSSH_set_fd(WOLFSSH* ssh, WS_SOCKET_T fd); WOLFSSH_API WS_SOCKET_T wolfSSH_get_fd(const WOLFSSH* ssh);