From 519ba208deb07ae817ded0b4bde43de7bfa62280 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 21 Aug 2026 15:06:46 -0700 Subject: [PATCH 01/11] Fix the shutdown channel teardown wolfSSH_shutdown() searched for the session channel by the peer's channel ID while telling ChannelFind() to match the local ID field. Each side numbers its channels independently, so the search usually found nothing. - The session channel is the head of the list; take it directly instead of searching for what is already in hand. - Restores the EOF, exit-status and close sends, and the drain that waits on the peer's close, all skipped on the NULL result. - Only bit when the two IDs differ, so the single-channel tests, where both sides pick 0, never saw it. - unit.c: shut down a channel whose peer ID is not its local ID, then check that EOF and close went out. Issue: F-8817 --- src/ssh.c | 4 +-- tests/unit.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index 1f3f7cc49..77d753694 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1113,9 +1113,9 @@ int wolfSSH_shutdown(WOLFSSH* ssh) if (ssh == NULL || ssh->channelList == NULL) ret = WS_BAD_ARGUMENT; - /* look up the channel if it still exists */ + /* The session channel is the head of the list. */ if (ret == WS_SUCCESS) { - channel = ChannelFind(ssh, ssh->channelList->peerChannel, WS_CHANNEL_ID_SELF); + channel = ssh->channelList; } /* if channel close was not already sent then send it */ diff --git a/tests/unit.c b/tests/unit.c index e1eff261c..feadbc754 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -16100,6 +16100,75 @@ static int test_ResolveOffset(void) #endif /* WOLFSSH_TEST_RESOLVE_OFFSET */ +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) + +/* IORecv mock reporting nothing to read yet, so the shutdown drain below + * completes without a live socket. */ +static int ShutdownIoRecv(WOLFSSH* ssh, void* data, word32 sz, void* ctx) +{ + WOLFSSH_UNUSED(ssh); + WOLFSSH_UNUSED(data); + WOLFSSH_UNUSED(sz); + WOLFSSH_UNUSED(ctx); + return WS_CBIO_ERR_WANT_READ; +} + +/* wolfSSH_shutdown() has to reach the session channel when the peer numbered + * it differently than this side did, which is the normal case: each side + * picks its own channel IDs. Looking the channel up by the peer's ID while + * matching against the local ID field found nothing, and the whole teardown + * was skipped. Only the EOF and close sends leave a flag behind to check; + * the exit-status request in between does not. */ +static int test_ShutdownPeerChannelId(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + word32 peerChannel; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1080; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, ShutdownIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1081; goto done; } + + /* Let the channel messages past the message filter. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 64); + if (ch == NULL) { result = -1082; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1083; + goto done; + } + + peerChannel = ch->channel + 7; + ch->peerChannel = peerChannel; + ch->openConfirmed = 1; + + /* The drain at the end of shutdown only sees a want-read, so the return + * is not the interesting part here; what got sent is. */ + (void)wolfSSH_shutdown(ssh); + + ch = ChannelFind(ssh, peerChannel, WS_CHANNEL_ID_PEER); + if (ch == NULL) { result = -1084; goto done; } + if (!ch->eofTxd) { result = -1085; goto done; } + if (!ch->closeTxd) { result = -1086; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +#endif /* WOLFSSH_TEST_INTERNAL && !NO_WOLFSSH_SERVER */ + + int wolfSSH_UnitTest(int argc, char** argv) { int testResult = 0, unitResult = 0; @@ -16770,6 +16839,13 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) + unitResult = test_ShutdownPeerChannelId(); + printf("ShutdownPeerChannelId: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + wolfSSH_Cleanup(); return (testResult ? 1 : 0); From 284e9e461cfaa4d9128f0623d7c41ad56ef2263f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 21 Aug 2026 15:06:46 -0700 Subject: [PATCH 02/11] Make a disconnect end the session SSH_MSG_DISCONNECT left nothing behind but ssh->error, which wolfSSH_stream_read() clears on entry. An application looping on the stream calls lost the code and went back to a connection already over. - Add WOLFSSH.disconnected, set by DoDisconnect() and SendDisconnect(). - DoDisconnect() sets it before decoding the payload, so a malformed message still ends the session. RFC 4253 section 11.1. - wolfSSH_stream_read() and wolfSSH_stream_send() report WS_DISCONNECT from the flag instead of reaching for the transport again. - Both guards run ahead of the channelList NULL test, so a torn-down session reports the disconnect rather than WS_BAD_ARGUMENT. - ssh.h states that undrained channel data goes with the session; internal.h states which calls the flag gates and which it does not. - regress.c: the receive side, the send side, and both of those again on a session with an open channel. Issue: F-8837 The test channel credits the peer's window too. Left at 0, SendChannelData() bails with WS_WINDOW_FULL before the wire, and the "nothing went out" checks would hold with the gate removed. --- src/internal.c | 9 +++ src/ssh.c | 20 ++++++- tests/regress.c | 136 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 4 ++ wolfssh/ssh.h | 3 + 5 files changed, 170 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index 316397e69..50029eb2c 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8166,6 +8166,10 @@ static int DoDisconnect(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) WOLFSSH_UNUSED(reasonStr); + /* RFC 4253 section 11.1, the peer is gone whether or not the rest of + * the message decodes. */ + ssh->disconnected = 1; + ret = GetUint32(&reason, buf, len, &begin); if (ret == WS_SUCCESS) { /* Skip the description text. */ @@ -16731,6 +16735,11 @@ int SendDisconnect(WOLFSSH* ssh, word32 reason) if (ssh == NULL) ret = WS_BAD_ARGUMENT; + /* Mark the session over before the send. A partial or failed send + * still ends it. */ + if (ret == WS_SUCCESS) + ssh->disconnected = 1; + if (ret == WS_SUCCESS) ret = PreparePacket(ssh, MSG_ID_SZ + UINT32_SZ + (LENGTH_SZ * 2)); diff --git a/src/ssh.c b/src/ssh.c index 77d753694..24156d19a 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1228,7 +1228,15 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_read()"); - if (ssh == NULL || buf == NULL || bufSz == 0 || ssh->channelList == NULL) + if (ssh == NULL || buf == NULL || bufSz == 0) + return WS_BAD_ARGUMENT; + + if (ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } + + if (ssh->channelList == NULL) return WS_BAD_ARGUMENT; if (ssh->channelList->eofRxd) { @@ -1307,7 +1315,15 @@ int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_send()"); - if (ssh == NULL || buf == NULL || ssh->channelList == NULL) + if (ssh == NULL || buf == NULL) + return WS_BAD_ARGUMENT; + + if (ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } + + if (ssh->channelList == NULL) return WS_BAD_ARGUMENT; if (ssh->isKeying) { diff --git a/tests/regress.c b/tests/regress.c index 826ed1d12..e5aa8bd6a 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -88,6 +88,7 @@ static void ResetSession(WOLFSSH* ssh) ssh->connectState = CONNECT_BEGIN; ssh->acceptState = ACCEPT_BEGIN; ssh->error = 0; + ssh->disconnected = 0; } @@ -2571,6 +2572,7 @@ static void TestDisconnectSetsDisconnectError(void) MemIo io; byte in[128]; byte out[32]; + byte data[8]; word32 inSz; int ret; @@ -2594,6 +2596,138 @@ static void TestDisconnectSetsDisconnectError(void) AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); AssertIntEQ(io.inOff, io.inSz); + /* The disconnect is terminal, not just this call's error. Later stream + * calls must report it rather than clearing the error and reading or + * writing more. */ + AssertTrue(ssh->disconnected); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Append a bare session channel so the stream calls have a channel to work + * on, the state a disconnect actually arrives in. */ +static void AddSessionChannel(WOLFSSH* ssh) +{ + WOLFSSH_CHANNEL* ch; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + AssertNotNull(ch); + AssertIntEQ(ChannelAppend(ssh, ch), WS_SUCCESS); + ch->openConfirmed = 1; + /* Credit the peer's window too. Left at 0, SendChannelData() bails with + * WS_WINDOW_FULL before the wire, and the "nothing went out" checks + * would hold with the gates removed. */ + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; +} + + +/* The same received disconnect on an established session. Without a channel + * the stream calls bail out on the NULL channel list before they reach + * anything, so this is the case that shows the gate doing work. */ +static void TestDisconnectTerminalWithChannel(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[128]; + byte out[128]; + byte data[8]; + word32 inSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + /* Past userauth, or the message filter blocks the sends on its own. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = DoReceive(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertTrue(ssh->disconnected); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + /* Nothing may go out on the channel either. */ + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Sending SSH_MSG_DISCONNECT ends the session the same way receiving one + * does: RFC 4253 section 11.1 says the connection is over once the message + * goes out, so the stream calls must refuse afterwards. */ +static void TestSendDisconnectIsTerminal(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[128]; + byte data[8]; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + /* Past userauth, or the message filter blocks the sends on its own. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_SUCCESS); + AssertTrue(ssh->disconnected); + AssertTrue(io.outSz > 0); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); } @@ -6328,6 +6462,8 @@ int main(int argc, char** argv) TestDoNewKeys(); #endif TestDisconnectSetsDisconnectError(); + TestDisconnectTerminalWithChannel(); + TestSendDisconnectIsTerminal(); #if !(defined(WOLFSSH_NO_RSA) && defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256)) TestClientBuffersIdempotent(); #endif diff --git a/wolfssh/internal.h b/wolfssh/internal.h index c40d78edc..ee728489b 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1073,6 +1073,10 @@ struct WOLFSSH { #endif byte connReset; byte isClosed; + /* Set when a DISCONNECT is sent or received. Only wolfSSH_stream_read() + * and wolfSSH_stream_send() are gated on it; the channel-id calls and + * wolfSSH_worker() are not, since the shutdown paths still pump them. */ + byte disconnected; byte clientOpenSSH; byte kexId; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index dfb45ab60..842de3049 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -558,6 +558,9 @@ WOLFSSH_API int wolfSSH_CTX_SetWindowPacketSize(WOLFSSH_CTX* ctx, WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); +/* A disconnect, sent or received, ends the session: wolfSSH_stream_read() + * and wolfSSH_stream_send() report WS_DISCONNECT from then on, and channel + * data that arrived before it but was never drained is dropped. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); From 6bc538c7a018b96b3050d81e907c1b0a20f6721f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 21 Aug 2026 15:14:53 -0700 Subject: [PATCH 03/11] Block every send after a disconnect The disconnect flag gated wolfSSH_stream_read() and wolfSSH_stream_send(), which is the client-side API. wolfsshd and echoserver drive their channels through the channel-id calls, so the daemon was never gated at all. - New SendAfterDisconnect() helper, used by the six send entry points: stream_send, stream_exit, ChannelIdSend, ChannelIdSendExt, extended_data_send and global_request. - Reads stay open, since data that arrived before the disconnect is still the caller's. wolfSSH_stream_read() drains its buffer and reports WS_DISCONNECT only once it runs dry. - wolfSSH_worker() stays ungated; the shutdown paths still pump it. - ssh.h and internal.h describe the split. - regress.c: buffered data survives the disconnect, and every send call refuses without a byte leaving the session. Issue: F-8837 Every public send call means every one: the channel-pointer sends (wolfSSH_ChannelSend, wolfSSH_ChannelSendExt, wolfSSH_ChannelExit), the forwarding requests and both wolfSSH_ChannelFwdNew* opens carry the gate too, and none of them had a message-filter backstop. ChannelCreditWindow() parks its credit rather than sending. The reads that drain what arrived before the disconnect credit the window for the bytes taken, and that credit went straight to the transport: each drain put a CHANNEL_WINDOW_ADJUST on the wire after the session was over, and a failing send replaced the byte count already copied for the caller. --- src/internal.c | 10 +- src/ssh.c | 87 ++++++++++-- tests/regress.c | 327 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 8 +- wolfssh/ssh.h | 12 +- 5 files changed, 423 insertions(+), 21 deletions(-) diff --git a/src/internal.c b/src/internal.c index 50029eb2c..7ff15ad25 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7627,8 +7627,9 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) /* Returns amount bytes of receive-window credit to the peer, folding in credit * already parked on the channel. Credit that cannot reach the transport is * parked, not dropped: no WINDOW_ADJUST may be sent mid-rekey (RFC 4253 section - * 7.1), an unbundled packet queued nothing, and a socket error can discard what - * was bundled. Credit that reached the output buffer counts as delivered. */ + * 7.1) or after a disconnect (section 11.1), an unbundled packet queued + * nothing, and a socket error can discard what was bundled. Credit that reached + * the output buffer counts as delivered. */ int ChannelCreditWindow(WOLFSSH* ssh, WOLFSSH_CHANNEL* channel, word32 amount) { word32 total; @@ -7647,7 +7648,10 @@ int ChannelCreditWindow(WOLFSSH* ssh, WOLFSSH_CHANNEL* channel, word32 amount) if (total == 0) return WS_SUCCESS; - if (ssh->isKeying) { + /* The reads that drain what arrived before a disconnect still credit the + * window locally, but the session is over and nothing more may go out. + * Park the credit so the read reports its bytes, not a send failure. */ + if (ssh->isKeying || ssh->disconnected) { channel->pendingWindowAdjust = total; return WS_SUCCESS; } diff --git a/src/ssh.c b/src/ssh.c index 24156d19a..a1c3915e0 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1103,6 +1103,21 @@ int wolfSSH_connect(WOLFSSH* ssh) #endif /* NO_WOLFSSH_CLIENT */ +/* A disconnect, sent or received, ends the session, so nothing further may + * go out. RFC 4253 section 11.1. Reads are deliberately not gated on this: + * channel data that arrived before the disconnect is still the caller's. + * Call only after ssh has been checked for NULL. */ +static int SendAfterDisconnect(WOLFSSH* ssh) +{ + if (ssh->disconnected) { + WLOG(WS_LOG_DEBUG, "Send attempted after a disconnect"); + ssh->error = WS_DISCONNECT; + return 1; + } + return 0; +} + + int wolfSSH_shutdown(WOLFSSH* ssh) { int ret = WS_SUCCESS; @@ -1231,13 +1246,14 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) if (ssh == NULL || buf == NULL || bufSz == 0) return WS_BAD_ARGUMENT; - if (ssh->disconnected) { - ssh->error = WS_DISCONNECT; - return WS_FATAL_ERROR; - } - - if (ssh->channelList == NULL) + if (ssh->channelList == NULL) { + /* No channel left to drain, so the disconnect is all there is. */ + if (ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } return WS_BAD_ARGUMENT; + } if (ssh->channelList->eofRxd) { ssh->error = WS_EOF; @@ -1252,6 +1268,13 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) inputBuffer = &ssh->channelList->inputBuffer; ssh->error = WS_SUCCESS; + /* Hand back whatever arrived before the disconnect, then report it once + * the buffer runs dry rather than going back to a dead transport. */ + if (ssh->disconnected && inputBuffer->length - inputBuffer->idx == 0) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { WLOG(WS_LOG_DEBUG, " Stream read index of %u", inputBuffer->idx); WLOG(WS_LOG_DEBUG, " Stream read ava data %u", inputBuffer->length); @@ -1318,10 +1341,8 @@ int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz) if (ssh == NULL || buf == NULL) return WS_BAD_ARGUMENT; - if (ssh->disconnected) { - ssh->error = WS_DISCONNECT; + if (SendAfterDisconnect(ssh)) return WS_FATAL_ERROR; - } if (ssh->channelList == NULL) return WS_BAD_ARGUMENT; @@ -1350,6 +1371,9 @@ int wolfSSH_ChannelIdSend(WOLFSSH* ssh, word32 channelId, if (ssh == NULL || buf == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF); if (channel == NULL) { @@ -1386,6 +1410,9 @@ int wolfSSH_ChannelIdSendExt(WOLFSSH* ssh, word32 channelId, if (ssh == NULL || buf == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF); if (channel == NULL) { @@ -1416,7 +1443,15 @@ int wolfSSH_stream_exit(WOLFSSH* ssh, int status) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_exit(), status = %d", status); - if (ssh == NULL || ssh->channelList == NULL) + if (ssh == NULL) + ret = WS_BAD_ARGUMENT; + + /* Ahead of the channel-list test, like the other stream calls, so a + * torn-down session reports the disconnect and not a bad argument. */ + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + + if (ret == WS_SUCCESS && ssh->channelList == NULL) ret = WS_BAD_ARGUMENT; if (ret == WS_SUCCESS) @@ -1442,6 +1477,8 @@ int wolfSSH_global_request(WOLFSSH *ssh, const unsigned char* data, word32 dataS return WS_BAD_ARGUMENT; if (reply != 0 && reply != 1) return WS_BAD_ARGUMENT; + if (SendAfterDisconnect(ssh)) + return WS_FATAL_ERROR; return SendGlobalRequest(ssh, data, dataSz, reply); } @@ -1452,7 +1489,13 @@ int wolfSSH_extended_data_send(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_extended_data_send()"); - if (ssh == NULL || buf == NULL || ssh->channelList == NULL) + if (ssh == NULL || buf == NULL) + return WS_BAD_ARGUMENT; + + if (SendAfterDisconnect(ssh)) + return WS_FATAL_ERROR; + + if (ssh->channelList == NULL) return WS_BAD_ARGUMENT; if (ssh->isKeying) { @@ -3539,6 +3582,9 @@ WOLFSSH_CHANNEL* wolfSSH_ChannelFwdNewLocal(WOLFSSH* ssh, if (ssh == NULL || ssh->ctx == NULL || host == NULL || origin == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { newChannel = ChannelNew(ssh, ID_CHANTYPE_TCPIP_DIRECT, ssh->ctx->windowSz, ssh->ctx->maxPacketSz); @@ -3578,6 +3624,9 @@ WOLFSSH_CHANNEL* wolfSSH_ChannelFwdNewRemote(WOLFSSH* ssh, if (ssh == NULL || ssh->ctx == NULL || host == NULL || origin == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { newChannel = ChannelNew(ssh, ID_CHANTYPE_TCPIP_FORWARD, ssh->ctx->windowSz, ssh->ctx->maxPacketSz); @@ -3646,6 +3695,9 @@ int wolfSSH_FwdRemoteSetup(WOLFSSH* ssh, const char* bindAddr, if (ret == WS_SUCCESS && ssh->ctx->side != WOLFSSH_ENDPOINT_CLIENT) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + /* A global request must not go out mid-rekey; only KEX traffic may. */ if (ret == WS_SUCCESS && ssh->isKeying) ret = WS_REKEYING; @@ -3681,6 +3733,9 @@ int wolfSSH_FwdRemoteCancel(WOLFSSH* ssh, const char* bindAddr, if (ret == WS_SUCCESS && ssh->ctx->side != WOLFSSH_ENDPOINT_CLIENT) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + /* A global request must not go out mid-rekey; only KEX traffic may. */ if (ret == WS_SUCCESS && ssh->isKeying) ret = WS_REKEYING; @@ -3998,6 +4053,9 @@ int wolfSSH_ChannelSend(WOLFSSH_CHANNEL* channel, WLOG(WS_LOG_DEBUG, "Entering wolfSSH_ChannelSend(), ID = %d, peerID = %d", channel->channel, channel->peerChannel); + if (channel->ssh != NULL && SendAfterDisconnect(channel->ssh)) + return WS_FATAL_ERROR; + #ifdef DEBUG_WOLFSSH DumpOctetString(buf, bufSz); #endif @@ -4033,6 +4091,9 @@ int wolfSSH_ChannelSendExt(WOLFSSH_CHANNEL* channel, "Entering wolfSSH_ChannelSendExt(), ID = %d, peerID = %d", channel->channel, channel->peerChannel); + if (channel->ssh != NULL && SendAfterDisconnect(channel->ssh)) + return WS_FATAL_ERROR; + #ifdef DEBUG_WOLFSSH DumpOctetString(buf, bufSz); #endif @@ -4062,6 +4123,10 @@ int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel) if (channel == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && channel->ssh != NULL && + SendAfterDisconnect(channel->ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) ret = SendChannelEof(channel->ssh, channel->peerChannel); diff --git a/tests/regress.c b/tests/regress.c index e5aa8bd6a..0b597e588 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2686,6 +2686,128 @@ static void TestDisconnectTerminalWithChannel(void) } +/* The disconnect stops sends, not reads. Channel data that arrived before + * it is still the caller's, and only once that runs dry does the read + * report the disconnect. */ +static void TestDisconnectDrainsBufferedData(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[128]; + byte out[128]; + byte data[16]; + byte payload[] = { 'h', 'e', 'l', 'l', 'o' }; + word32 inSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + + AssertIntEQ(ChannelPutData(ssh->channelList, payload, sizeof(payload)), + WS_SUCCESS); + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = DoReceive(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertTrue(ssh->disconnected); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, (int)sizeof(payload)); + AssertIntEQ(WMEMCMP(data, payload, sizeof(payload)), 0); + + /* Buffer is dry now, so the disconnect is what is left to report. */ + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Every send entry point refuses after a disconnect, not just the stream + * calls. wolfsshd and echoserver drive their channels through the + * channel-id and extended-data calls and never touch wolfSSH_stream_send(). */ +static void TestDisconnectBlocksEverySend(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + byte data[8]; + word32 quietSz; + word32 channelId; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channelId = ssh->channelList->channel; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertTrue(ssh->disconnected); + quietSz = io.outSz; + + WMEMSET(data, 0, sizeof(data)); + + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_ChannelIdSend(ssh, channelId, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_ChannelIdSendExt(ssh, channelId, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_extended_data_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_global_request(ssh, data, sizeof(data), 0); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_stream_exit(ssh, 0); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + /* Not one byte left the session after the disconnect. */ + AssertIntEQ(io.outSz, quietSz); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + /* Sending SSH_MSG_DISCONNECT ends the session the same way receiving one * does: RFC 4253 section 11.1 says the connection is over once the message * goes out, so the stream calls must refuse afterwards. */ @@ -2732,6 +2854,206 @@ static void TestSendDisconnectIsTerminal(void) wolfSSH_CTX_free(ctx); } + +/* The reads that drain what arrived before a disconnect must not put a + * window adjust on the wire. The credit is parked on the channel instead, + * so the read still reports its bytes rather than a send failure. */ +static void TestDisconnectQuietWindowAdjust(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte in[128]; + byte out[256]; + byte payload[600]; + byte extPayload[64]; + byte data[600]; + word32 inSz; + word32 windowSz; + word32 pendingSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + /* Past userauth, or the message filter blocks the adjust on its own and + * the wire check below proves nothing. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* More than half the channel buffer, so draining it trips the window + * update in _UpdateChannelWindow(). */ + WMEMSET(payload, 'a', sizeof(payload)); + AssertIntEQ(ChannelPutData(channel, payload, sizeof(payload)), WS_SUCCESS); + + /* Buffered stderr for the extended-data drain. */ + WMEMSET(extPayload, 'e', sizeof(extPayload)); + AssertIntEQ(GrowBuffer(&channel->extDataBuffer, sizeof(extPayload)), + WS_SUCCESS); + WMEMCPY(channel->extDataBuffer.buffer, extPayload, sizeof(extPayload)); + channel->extDataBuffer.length = sizeof(extPayload); + channel->extDataBuffer.idx = 0; + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = DoReceive(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertTrue(ssh->disconnected); + io.outSz = 0; + + /* Two reads: the first leaves the index non-zero, the second is the one + * with credit to return. */ + ret = wolfSSH_stream_read(ssh, data, 300); + AssertIntEQ(ret, 300); + ret = wolfSSH_stream_read(ssh, data, 300); + AssertIntEQ(ret, 300); + AssertIntEQ(io.outSz, 0); + + /* The credit is owed, not lost. The window update runs before the read + * advances the index, so the second read is the one that credits the + * first read's 300 bytes. */ + AssertIntEQ(channel->pendingWindowAdjust, 300); + pendingSz = channel->pendingWindowAdjust; + windowSz = channel->windowSz; + + ret = wolfSSH_extended_data_read(ssh, data, sizeof(extPayload)); + AssertIntEQ(ret, (int)sizeof(extPayload)); + AssertIntEQ(io.outSz, 0); + AssertIntEQ(channel->pendingWindowAdjust, + pendingSz + (word32)sizeof(extPayload)); + AssertIntEQ(channel->windowSz, windowSz + (word32)sizeof(extPayload)); + + /* wolfsshd and echoserver read by channel ID, so cover that drain too. */ + AssertIntEQ(ChannelPutData(channel, payload, sizeof(payload)), WS_SUCCESS); + ret = wolfSSH_ChannelIdRead(ssh, channel->channel, data, sizeof(payload)); + AssertIntEQ(ret, (int)sizeof(payload)); + AssertIntEQ(io.outSz, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* The channel-pointer and forwarding APIs are send calls too. */ +static void TestDisconnectBlocksChannelAndFwdSends(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte out[256]; + byte data[8]; + word32 quietSz; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + quietSz = io.outSz; + + WMEMSET(data, 0, sizeof(data)); + + AssertIntEQ(wolfSSH_ChannelSend(channel, data, sizeof(data)), + WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + AssertIntEQ(wolfSSH_ChannelSendExt(channel, data, sizeof(data)), + WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + AssertIntEQ(wolfSSH_ChannelExit(channel), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + +#ifdef WOLFSSH_FWD + AssertIntEQ(wolfSSH_FwdRemoteSetup(ssh, "127.0.0.1", 22, 0), + WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + AssertIntEQ(wolfSSH_FwdRemoteCancel(ssh, "127.0.0.1", 22, 0), + WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + AssertNull(wolfSSH_ChannelFwdNewLocal(ssh, "127.0.0.1", 22, + "127.0.0.1", 22)); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + AssertNull(wolfSSH_ChannelFwdNewRemote(ssh, "127.0.0.1", 22, + "127.0.0.1", 22)); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); +#endif + + /* The channel is still whole: nothing was torn down either. */ + AssertIntEQ(channel->eofTxd, 0); + AssertIntEQ(channel->closeTxd, 0); + AssertIntEQ(io.outSz, quietSz); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* wolfSSH_stream_exit() answers like the rest of its family on a session + * whose channel is already gone: the disconnect, not a bad argument. */ +static void TestStreamExitReportsDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertIntEQ(ChannelRemove(ssh, ssh->channelList->channel, + WS_CHANNEL_ID_SELF), WS_SUCCESS); + AssertNull(ssh->channelList); + + AssertIntEQ(wolfSSH_stream_exit(ssh, 0), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + #ifdef WOLFSSH_SFTP static void TestOct2DecRejectsInvalidNonLeadingDigit(void) { @@ -6463,7 +6785,12 @@ int main(int argc, char** argv) #endif TestDisconnectSetsDisconnectError(); TestDisconnectTerminalWithChannel(); + TestDisconnectDrainsBufferedData(); + TestDisconnectBlocksEverySend(); TestSendDisconnectIsTerminal(); + TestDisconnectQuietWindowAdjust(); + TestDisconnectBlocksChannelAndFwdSends(); + TestStreamExitReportsDisconnect(); #if !(defined(WOLFSSH_NO_RSA) && defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256)) TestClientBuffersIdempotent(); #endif diff --git a/wolfssh/internal.h b/wolfssh/internal.h index ee728489b..914e0159a 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1073,9 +1073,11 @@ struct WOLFSSH { #endif byte connReset; byte isClosed; - /* Set when a DISCONNECT is sent or received. Only wolfSSH_stream_read() - * and wolfSSH_stream_send() are gated on it; the channel-id calls and - * wolfSSH_worker() are not, since the shutdown paths still pump them. */ + /* Set when a DISCONNECT is sent or received. Gates every public send + * call, so + * nothing more goes out. The read calls are not gated: data that + * arrived before the disconnect can still be drained. wolfSSH_worker() + * is not gated either, since the shutdown paths still pump it. */ byte disconnected; byte clientOpenSSH; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 842de3049..bc1b83137 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -558,9 +558,12 @@ WOLFSSH_API int wolfSSH_CTX_SetWindowPacketSize(WOLFSSH_CTX* ctx, WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); -/* A disconnect, sent or received, ends the session: wolfSSH_stream_read() - * and wolfSSH_stream_send() report WS_DISCONNECT from then on, and channel - * data that arrived before it but was never drained is dropped. */ +/* A disconnect, sent or received, ends the session. Nothing more goes out: + * every send call in this header, above this comment and below it, + * reports WS_DISCONNECT from then on. Reads are not + * gated, so channel data that arrived before the disconnect can still be + * drained; wolfSSH_stream_read() reports WS_DISCONNECT once its buffer + * runs dry. RFC 4253 section 11.1. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); @@ -588,7 +591,8 @@ WOLFSSH_API int wolfSSH_extended_data_send(WOLFSSH* ssh, byte* buf, word32 bufSz * can be short: the byte count is still returned, but wolfSSH_get_error() is * left at WS_WANT_WRITE to show a flush is owed. An app that only reads must * then flush, with wolfSSH_worker(), or the peer's window is never replenished - * and the channel stalls. + * and the channel stalls. After a disconnect there is nothing to flush: the + * credit is parked on the channel rather than sent. * * The buffer lives on the channel: anything unread when the channel is removed * (the peer's CHANNEL_CLOSE) is discarded with it. */ From d4ebb824996bdecfe856127edeba31a10229383d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 21 Aug 2026 15:20:33 -0700 Subject: [PATCH 04/11] Report a disconnect from stream_peek wolfSSH_stream_peek() is how the shell loops decide whether a channel is drained. It had no disconnect check, so a dead session looked exactly like a drained one: zero bytes available, nothing to tell them apart. - Report WS_DISCONNECT once the buffered data runs dry, the same shape wolfSSH_stream_read() uses. What is still buffered comes back first. - ssh.h and internal.h name peek alongside the read call, and no longer claim the read side is ungated outright. - regress.c: peek sees the buffered byte, then sees the disconnect. Raised from the channel-eof branch, where peek becomes the drain gate for the wolfsshd and echoserver shell loops. Issue: F-8837 --- src/ssh.c | 16 ++++++++++++++-- tests/regress.c | 9 +++++++++ wolfssh/internal.h | 8 ++++---- wolfssh/ssh.h | 4 ++-- 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index a1c3915e0..54da548f6 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1198,6 +1198,7 @@ int wolfSSH_TriggerKeyExchange(WOLFSSH* ssh) int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz) { WOLFSSH_BUFFER* inputBuffer; + word32 avail; WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_peek()"); @@ -1214,11 +1215,22 @@ int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz) } inputBuffer = &ssh->channelList->inputBuffer; - bufSz = min(bufSz, inputBuffer->length - inputBuffer->idx); + avail = inputBuffer->length - inputBuffer->idx; + + /* Report the disconnect only once the buffered data is drained, the + * same way wolfSSH_stream_read() does. Callers use this to tell a + * drained channel from one with more to come, and a dead session is + * neither. */ + if (avail == 0 && ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } + + bufSz = min(bufSz, avail); if (buf != NULL) { WMEMCPY(buf, inputBuffer->buffer + inputBuffer->idx, bufSz); } - return bufSz; + return (int)bufSz; } diff --git a/tests/regress.c b/tests/regress.c index 0b597e588..efcc02286 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2725,12 +2725,21 @@ static void TestDisconnectDrainsBufferedData(void) AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); AssertTrue(ssh->disconnected); + /* Peek is the drain gate the shell loops use, so it has to tell a + * channel with data left from a session that is over. */ + ret = wolfSSH_stream_peek(ssh, NULL, 1); + AssertIntEQ(ret, 1); + WMEMSET(data, 0, sizeof(data)); ret = wolfSSH_stream_read(ssh, data, sizeof(data)); AssertIntEQ(ret, (int)sizeof(payload)); AssertIntEQ(WMEMCMP(data, payload, sizeof(payload)), 0); /* Buffer is dry now, so the disconnect is what is left to report. */ + ret = wolfSSH_stream_peek(ssh, NULL, 1); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); AssertIntEQ(ret, WS_FATAL_ERROR); AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 914e0159a..f778878dd 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1074,10 +1074,10 @@ struct WOLFSSH { byte connReset; byte isClosed; /* Set when a DISCONNECT is sent or received. Gates every public send - * call, so - * nothing more goes out. The read calls are not gated: data that - * arrived before the disconnect can still be drained. wolfSSH_worker() - * is not gated either, since the shutdown paths still pump it. */ + * call, so nothing more goes out. Reads still hand back what arrived + * before the disconnect; the head-of-list reads report it once their + * buffer runs dry. wolfSSH_worker() is not gated, the shutdown paths + * pump it. */ byte disconnected; byte clientOpenSSH; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index bc1b83137..ecb1926d1 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -562,8 +562,8 @@ WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); * every send call in this header, above this comment and below it, * reports WS_DISCONNECT from then on. Reads are not * gated, so channel data that arrived before the disconnect can still be - * drained; wolfSSH_stream_read() reports WS_DISCONNECT once its buffer - * runs dry. RFC 4253 section 11.1. */ + * drained; wolfSSH_stream_read() and wolfSSH_stream_peek() report + * WS_DISCONNECT once their buffer runs dry. RFC 4253 section 11.1. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); From 587c8d2995747471ad9fe372f8db5eeff7118b8e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 24 Aug 2026 14:25:17 -0700 Subject: [PATCH 05/11] Gate the last three senders on a disconnect The ssh.h comment promised that every send call below it reports WS_DISCONNECT, but three did not: wolfSSH_TriggerKeyExchange(), wolfSSH_SendIgnore() and wolfSSH_SendDisconnect(). - All three now take the SendAfterDisconnect() gate, so the sentence in ssh.h describes the code rather than the intent. - TriggerKeyExchange() is the highwater callback's rekey trigger, so this also stops a rekey starting on a session the peer has ended. - SendIgnore() and SendDisconnect() gained the NULL check the gate needs; both already reported WS_BAD_ARGUMENT for that from the callee. - A second disconnect is refused: one ends the session. - regress.c: the three calls join the send sweep. Issue: F-8837 --- src/ssh.c | 19 +++++++++++++++++++ tests/regress.c | 13 +++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/ssh.c b/src/ssh.c index 54da548f6..605ba8170 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1185,6 +1185,9 @@ int wolfSSH_TriggerKeyExchange(WOLFSSH* ssh) if (ssh == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) ret = ssh->error = SendKexInit(ssh); @@ -1547,6 +1550,13 @@ int wolfSSH_SendIgnore(WOLFSSH* ssh, const byte* buf, word32 bufSz) WOLFSSH_UNUSED(buf); WOLFSSH_UNUSED(bufSz); + + if (ssh == NULL) + return WS_BAD_ARGUMENT; + + if (SendAfterDisconnect(ssh)) + return WS_FATAL_ERROR; + WMEMSET(scratch, 0, sizeof(scratch)); return SendIgnore(ssh, scratch, sizeof(scratch)); @@ -1556,6 +1566,15 @@ int wolfSSH_SendIgnore(WOLFSSH* ssh, const byte* buf, word32 bufSz) int wolfSSH_SendDisconnect(WOLFSSH *ssh, word32 reason) { WLOG(WS_LOG_DEBUG, "Entering wolfSSH_SendDisconnect"); + + if (ssh == NULL) + return WS_BAD_ARGUMENT; + + /* One disconnect ends the session; a second is more traffic on a + * connection that is already over. */ + if (SendAfterDisconnect(ssh)) + return WS_FATAL_ERROR; + return SendDisconnect(ssh, reason); } diff --git a/tests/regress.c b/tests/regress.c index efcc02286..3d60ff119 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2809,6 +2809,19 @@ static void TestDisconnectBlocksEverySend(void) AssertIntEQ(ret, WS_FATAL_ERROR); AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + ret = wolfSSH_TriggerKeyExchange(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_SendIgnore(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + /* Including a second disconnect. */ + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + /* Not one byte left the session after the disconnect. */ AssertIntEQ(io.outSz, quietSz); From 0a969d08f53aca2c67238d1975110eb9b1ce3888 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 24 Aug 2026 15:27:12 -0700 Subject: [PATCH 06/11] Close the last three post-disconnect sends The disconnect gate left three ways for traffic to reach a peer that had already ended the session, and it made the default highwater callback report a failure for a packet that had gone out fine. - wolfSSH_shutdown() drops the channel when ssh->disconnected is set, so the EOF, exit status and close are skipped along with the wait for a close the peer will never send - wsHighwater() skips the rekey request on a disconnected session, so a firing high water mark no longer turns SendDisconnect() and SendChannelEof() into failures - wolfSSH_ChangeTerminalSize() gained the SendAfterDisconnect() gate, making the ssh.h contract true for every send declared below it - regress covers all three, including that shutdown leaves eofTxd and closeTxd clear and puts nothing on the wire A disconnect of our own left queued by a short send still reaches the peer. SendDisconnect() records disconnectTxd once the packet is bundled, and one FlushQueuedDisconnect() helper gates the retry in wolfSSH_SendDisconnect() and wolfSSH_shutdown() on that. Keying it on disconnected alone would push whatever was queued, since the peer's disconnect sets that flag too and leaves only unrelated traffic behind. wolfSSH_shutdown() flushes ahead of the channel-list test, so the peer's close retiring the last channel does not strand the disconnect, and an unfinished flush outranks WS_CHANNEL_CLOSED. Its WS_WANT_WRITE stays in ssh->error as well, since callers gate their shutdown retry on that. The highwater guard sits in HighwaterCheck(), not in the default callback: the return that fails the send comes from whatever callback the application installed, and it propagates out through wolfSSH_SendPacket(). Issue: F-8837 --- src/internal.c | 11 ++ src/ssh.c | 59 +++++- tests/regress.c | 462 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 5 + wolfssh/ssh.h | 16 +- 5 files changed, 542 insertions(+), 11 deletions(-) diff --git a/src/internal.c b/src/internal.c index 7ff15ad25..520f18abc 100644 --- a/src/internal.c +++ b/src/internal.c @@ -612,6 +612,12 @@ static INLINE int HighwaterCheck(WOLFSSH* ssh, byte side) } + /* A mark firing on a dead session must not fail the send that fired it: + * this return propagates out of wolfSSH_SendPacket(). Callbacks rekey, + * which is gated after a disconnect, so do not fire one at all. */ + if (fire && ssh->disconnected) + fire = 0; + if (fire && ssh->ctx->highwaterCb) ret = ssh->ctx->highwaterCb(side, ssh->highwaterCtx); @@ -16764,6 +16770,11 @@ int SendDisconnect(WOLFSSH* ssh, word32 reason) ret = BundlePacket(ssh); } + /* The packet is in the output buffer now, so a short send below leaves + * something worth flushing. */ + if (ret == WS_SUCCESS) + ssh->disconnectTxd = 1; + if (ret == WS_SUCCESS) ret = wolfSSH_SendPacket(ssh); diff --git a/src/ssh.c b/src/ssh.c index 605ba8170..c4c5766d5 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1118,14 +1118,38 @@ static int SendAfterDisconnect(WOLFSSH* ssh) } +/* A disconnect of ours left queued by a short send still has to reach the + * peer, and flushing bytes that are already bundled is not the new traffic + * RFC 4253 section 11.1 forbids. Only our own disconnect qualifies: a + * disconnect from the peer leaves nothing queued but unrelated traffic, + * which the session is over for. Call only after a NULL check of ssh. */ +static int FlushQueuedDisconnect(WOLFSSH* ssh) +{ + if (!ssh->disconnectTxd || !wolfSSH_OutputPending(ssh)) + return 0; + + WLOG(WS_LOG_DEBUG, "Flushing a disconnect left queued by a short send"); + return 1; +} + + int wolfSSH_shutdown(WOLFSSH* ssh) { int ret = WS_SUCCESS; + int flushRet = WS_SUCCESS; WOLFSSH_CHANNEL* channel = NULL; WLOG(WS_LOG_DEBUG, "Entering wolfSSH_shutdown()"); - if (ssh == NULL || ssh->channelList == NULL) + if (ssh == NULL) + ret = WS_BAD_ARGUMENT; + + /* This is a teardown call, so a disconnect of ours left queued by a + * short send goes out here, with or without a channel to tear down. */ + if (ret == WS_SUCCESS && FlushQueuedDisconnect(ssh)) + flushRet = wolfSSH_SendPacket(ssh); + + if (ret == WS_SUCCESS && ssh->channelList == NULL) ret = WS_BAD_ARGUMENT; /* The session channel is the head of the list. */ @@ -1133,16 +1157,27 @@ int wolfSSH_shutdown(WOLFSSH* ssh) channel = ssh->channelList; } + /* Session already over. Drop the channel to skip the teardown sends + * and the wait for a close that will not come. RFC 4253 section 11.1. */ + if (channel != NULL && ssh->disconnected) { + WLOG(WS_LOG_DEBUG, "Session already disconnected, nothing to send"); + /* An unfinished flush owns ssh->error. Callers gate their retry on + * WS_WANT_WRITE, so overwriting it strands the queued disconnect. */ + if (flushRet == WS_SUCCESS) + ssh->error = WS_DISCONNECT; + channel = NULL; + } + /* if channel close was not already sent then send it */ if (channel != NULL && !channel->closeTxd) { if (ret == WS_SUCCESS) { - ret = SendChannelEof(ssh, ssh->channelList->peerChannel); + ret = SendChannelEof(ssh, channel->peerChannel); } /* continue on success and in case where queueing up send packets */ if (ret == WS_SUCCESS || (ret != WS_BAD_ARGUMENT && ssh->error == WS_WANT_WRITE)) { - ret = SendChannelExit(ssh, ssh->channelList->peerChannel, + ret = SendChannelExit(ssh, channel->peerChannel, #if defined(WOLFSSH_TERM) || defined(WOLFSSH_SHELL) ssh->exitStatus); #else @@ -1153,7 +1188,7 @@ int wolfSSH_shutdown(WOLFSSH* ssh) /* continue on success and in case where queueing up send packets */ if (ret == WS_SUCCESS || (ret != WS_BAD_ARGUMENT && ssh->error == WS_WANT_WRITE)) - ret = SendChannelClose(ssh, ssh->channelList->peerChannel); + ret = SendChannelClose(ssh, channel->peerChannel); } @@ -1172,6 +1207,11 @@ int wolfSSH_shutdown(WOLFSSH* ssh) ret = WS_CHANNEL_CLOSED; } + /* An unfinished flush outranks the channel status: the caller has to + * come back for the rest of the disconnect. */ + if (flushRet != WS_SUCCESS) + ret = flushRet; + WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_shutdown(), ret = %d", ret); return ret; } @@ -1571,9 +1611,13 @@ int wolfSSH_SendDisconnect(WOLFSSH *ssh, word32 reason) return WS_BAD_ARGUMENT; /* One disconnect ends the session; a second is more traffic on a - * connection that is already over. */ - if (SendAfterDisconnect(ssh)) + * connection that is already over. A short send leaves the first one + * queued, though, so that retry goes through. */ + if (SendAfterDisconnect(ssh)) { + if (FlushQueuedDisconnect(ssh)) + return wolfSSH_SendPacket(ssh); return WS_FATAL_ERROR; + } return SendDisconnect(ssh, reason); } @@ -1676,6 +1720,9 @@ int wolfSSH_ChangeTerminalSize(WOLFSSH* ssh, word32 columns, word32 rows, if (ssh == NULL) ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { ret = SendChannelTerminalResize(ssh, columns, rows, widthPixels, heightPixels); diff --git a/tests/regress.c b/tests/regress.c index 3d60ff119..f21381c6c 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3076,6 +3076,456 @@ static void TestStreamExitReportsDisconnect(void) wolfSSH_CTX_free(ctx); } + +/* wolfSSH_shutdown() is a send path too: no teardown on the wire, and no + * wait for a close that will not come. */ +static void TestShutdownQuietAfterDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte in[128]; + byte out[256]; + word32 inSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = DoReceive(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertTrue(ssh->disconnected); + io.outSz = 0; + + /* Nothing left to tear down. */ + ret = wolfSSH_shutdown(ssh); + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, 0); + AssertIntEQ(channel->eofTxd, 0); + AssertIntEQ(channel->closeTxd, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} +/* A send callback that reports "would block" for its first ProbeWantWrite + * calls, the way a full socket does. */ +static int MemSendWantWriteCount; + +static int MemSendWantWrite(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + if (MemSendWantWriteCount > 0) { + MemSendWantWriteCount--; + return WS_CBIO_ERR_WANT_WRITE; + } + return MemSend(ssh, buf, sz, ctx); +} + + + + +/* The mark firing on the disconnect packet must not fail a send that + * went out fine. */ +static void TestHighwaterQuietAfterDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* Low enough that the disconnect packet trips it. */ + AssertIntEQ(wolfSSH_SetHighwater(ssh, 1), WS_SUCCESS); + + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_SUCCESS); + AssertTrue(ssh->highwaterFlag); + AssertTrue(ssh->disconnected); + + /* The mark fired, but no key exchange was started. */ + AssertIntEQ(ssh->isKeying, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* A disconnect from the peer sets the same flag ours does, but leaves only + * unrelated traffic queued. That traffic belongs to a session that is over, + * so neither teardown call may push it out. */ +static void TestPeerDisconnectKeepsTrafficQueued(int useShutdown) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[128]; + byte out[512]; + byte data[32]; + word32 inSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* Channel data the socket would not take. */ + WMEMSET(data, 'x', sizeof(data)); + MemSendWantWriteCount = 1; + AssertIntEQ(wolfSSH_stream_send(ssh, data, sizeof(data)), + (int)sizeof(data)); + AssertIntEQ(wolfSSH_get_error(ssh), WS_WANT_WRITE); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(io.outSz, 0); + + /* The peer disconnects while those bytes are still queued. */ + AssertIntEQ(DoReceive(ssh), WS_FATAL_ERROR); + AssertTrue(ssh->disconnected); + AssertFalse(ssh->disconnectTxd); + AssertTrue(wolfSSH_OutputPending(ssh)); + + if (useShutdown) { + /* The socket takes bytes now, so only the gate keeps them back. */ + ret = wolfSSH_shutdown(ssh); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + } + else { + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + } + + /* Not one byte of the stale channel data reached the peer. */ + AssertIntEQ(io.outSz, 0); + AssertTrue(wolfSSH_OutputPending(ssh)); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* The queued-disconnect flush does not need a channel: the peer's close can + * retire the last one between the short send and the shutdown. */ +static void TestShutdownFlushesWithNoChannel(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + MemSendWantWriteCount = 1; + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_WANT_WRITE); + AssertTrue(ssh->disconnectTxd); + AssertIntEQ(io.outSz, 0); + + AssertIntEQ(ChannelRemove(ssh, ssh->channelList->channel, + WS_CHANNEL_ID_SELF), WS_SUCCESS); + AssertNull(ssh->channelList); + + ret = wolfSSH_shutdown(ssh); + AssertIntEQ(ret, WS_CHANNEL_CLOSED); + AssertFalse(wolfSSH_OutputPending(ssh)); + AssertIntEQ(out[LENGTH_SZ + 1], MSGID_DISCONNECT); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* The highwater callback is the application's, and its return propagates out + * of wolfSSH_SendPacket(). A mark firing on the disconnect packet must not + * fail a send that went out fine, whoever owns the callback. */ +static int RekeyingHighwaterCb(byte side, void* ctx) +{ + WOLFSSH* ssh = (WOLFSSH*)ctx; + + WOLFSSH_UNUSED(side); + return wolfSSH_TriggerKeyExchange(ssh); +} + +static void TestAppHighwaterQuietAfterDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + /* Low enough that the disconnect packet trips it. */ + wolfSSH_SetHighwaterCb(ctx, 1, RekeyingHighwaterCb); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + wolfSSH_SetHighwaterCtx(ssh, ssh); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertTrue(io.outSz > 0); + AssertIntEQ(ssh->isKeying, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* A disconnect left queued by a short send still has to reach the peer. + * The gate refuses new traffic, not a flush of what is already bundled. */ +static void TestQueuedDisconnectFlushes(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + word32 sentSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + MemSendWantWriteCount = 1; + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_WANT_WRITE); + AssertTrue(ssh->disconnected); + AssertIntEQ(io.outSz, 0); + AssertTrue(wolfSSH_OutputPending(ssh)); + + /* Calling again retries the queued packet rather than refusing it. */ + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_SUCCESS); + AssertFalse(wolfSSH_OutputPending(ssh)); + AssertTrue(io.outSz > 0); + /* Unencrypted framing: length, padding length, then the message ID. */ + AssertIntEQ(out[LENGTH_SZ + 1], MSGID_DISCONNECT); + sentSz = io.outSz; + + /* Nothing queued now, so a second disconnect is refused as before. */ + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, sentSz); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* wolfSSH_shutdown() after a short disconnect send: no teardown on the wire, + * but the queued disconnect goes out. */ +static void TestShutdownFlushesQueuedDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte out[256]; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + MemSendWantWriteCount = 1; + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_WANT_WRITE); + AssertIntEQ(io.outSz, 0); + + ret = wolfSSH_shutdown(ssh); + AssertIntEQ(ret, WS_SUCCESS); + AssertFalse(wolfSSH_OutputPending(ssh)); + AssertIntEQ(out[LENGTH_SZ + 1], MSGID_DISCONNECT); + + /* The disconnect, and only the disconnect. */ + AssertIntEQ(channel->eofTxd, 0); + AssertIntEQ(channel->closeTxd, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* A flush that is itself short owns ssh->error. Callers gate their retry on + * WS_WANT_WRITE, so the disconnect gate must not overwrite it. */ +static void TestShutdownKeepsFlushWantWrite(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte out[256]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* One short send for the disconnect, another for the flush below. */ + MemSendWantWriteCount = 2; + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_WANT_WRITE); + AssertIntEQ(io.outSz, 0); + + /* The channel is still listed, so the disconnect gate runs, but the + * unfinished flush is what the caller has to act on. */ + AssertNotNull(ssh->channelList); + AssertIntEQ(wolfSSH_shutdown(ssh), WS_WANT_WRITE); + AssertIntEQ(wolfSSH_get_error(ssh), WS_WANT_WRITE); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(io.outSz, 0); + + /* Retrying gets it out, and only it. */ + AssertIntEQ(wolfSSH_shutdown(ssh), WS_SUCCESS); + AssertFalse(wolfSSH_OutputPending(ssh)); + AssertIntEQ(out[LENGTH_SZ + 1], MSGID_DISCONNECT); + AssertIntEQ(channel->eofTxd, 0); + AssertIntEQ(channel->closeTxd, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +#if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM) +/* A window-change request is a send like any other. */ +static void TestTerminalResizeBlockedAfterDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + word32 quietSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + quietSz = io.outSz; + + ret = wolfSSH_ChangeTerminalSize(ssh, 80, 24, 0, 0); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, quietSz); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} +#endif /* WOLFSSH_TERM && !NO_FILESYSTEM */ + #ifdef WOLFSSH_SFTP static void TestOct2DecRejectsInvalidNonLeadingDigit(void) { @@ -6813,6 +7263,18 @@ int main(int argc, char** argv) TestDisconnectQuietWindowAdjust(); TestDisconnectBlocksChannelAndFwdSends(); TestStreamExitReportsDisconnect(); + TestShutdownQuietAfterDisconnect(); + TestHighwaterQuietAfterDisconnect(); + TestAppHighwaterQuietAfterDisconnect(); + TestPeerDisconnectKeepsTrafficQueued(0); + TestPeerDisconnectKeepsTrafficQueued(1); + TestShutdownFlushesWithNoChannel(); + TestQueuedDisconnectFlushes(); + TestShutdownFlushesQueuedDisconnect(); + TestShutdownKeepsFlushWantWrite(); +#if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM) + TestTerminalResizeBlockedAfterDisconnect(); +#endif #if !(defined(WOLFSSH_NO_RSA) && defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256)) TestClientBuffersIdempotent(); #endif diff --git a/wolfssh/internal.h b/wolfssh/internal.h index f778878dd..512f3e343 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1079,6 +1079,11 @@ struct WOLFSSH { * buffer runs dry. wolfSSH_worker() is not gated, the shutdown paths * pump it. */ byte disconnected; + /* Set once SendDisconnect() has bundled our own DISCONNECT into the + * output buffer, so a short send can still be flushed. The flag above + * cannot stand in for this: it does not say whose disconnect it was, + * and a peer's leaves only unrelated traffic queued. */ + byte disconnectTxd; byte clientOpenSSH; byte kexId; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index ecb1926d1..f5e2a0c8b 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -559,11 +559,11 @@ WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); /* A disconnect, sent or received, ends the session. Nothing more goes out: - * every send call in this header, above this comment and below it, - * reports WS_DISCONNECT from then on. Reads are not - * gated, so channel data that arrived before the disconnect can still be - * drained; wolfSSH_stream_read() and wolfSSH_stream_peek() report - * WS_DISCONNECT once their buffer runs dry. RFC 4253 section 11.1. */ + * every send call in this header, above this comment and below it, reports + * WS_DISCONNECT from then on. Reads are not gated, so channel data that + * arrived before the disconnect can still be drained; wolfSSH_stream_read() + * and wolfSSH_stream_peek() report WS_DISCONNECT once their buffer runs + * dry. RFC 4253 section 11.1. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); @@ -600,6 +600,12 @@ WOLFSSH_API int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz); WOLFSSH_API int wolfSSH_TriggerKeyExchange(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_SendIgnore(WOLFSSH* ssh, const byte* buf, word32 bufSz); +/* One disconnect ends the session, so a second call reports WS_DISCONNECT. + * The exception is a disconnect of this side's own, left short by a + * non-blocking socket: while it is still queued, calling again retries the + * flush, since bundled bytes are not new traffic. wolfSSH_shutdown() retries + * it too, with or without a channel. A disconnect from the peer is not that + * case: it leaves only unrelated traffic queued, and that stays put. */ WOLFSSH_API int wolfSSH_SendDisconnect(WOLFSSH* ssh, word32 reason); WOLFSSH_API int wolfSSH_global_request(WOLFSSH* ssh, const unsigned char* data, word32 dataSz, int reply); From 98f3de86e7676deb1d80c625a441569f04301aeb Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 25 Aug 2026 11:31:00 -0700 Subject: [PATCH 07/11] Make the disconnect tests prove what they claim The two disconnect tests ran on a session that had never finished user auth, so IsMessageAllowed() blocked the sends on its own and the "nothing on the wire" assertions held even with the gates removed. Both now sit past user auth. With only the shutdown gate reverted the test measures 72 bytes out and both teardown flags set, where before it measured nothing. - wolfSSH_stream_peek() reports WS_DISCONNECT when the channel is gone, the way wolfSSH_stream_read() already did; a missing channel used to read as a bad argument on a session that had simply ended - the drain test covers the no-channel case for both calls Issue: F-8837 --- src/ssh.c | 11 ++++++++++- tests/regress.c | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/ssh.c b/src/ssh.c index c4c5766d5..360d0b416 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1245,8 +1245,17 @@ int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_peek()"); - if (ssh == NULL || ssh->channelList == NULL) + if (ssh == NULL) + return WS_BAD_ARGUMENT; + + if (ssh->channelList == NULL) { + /* No channel left to drain, so the disconnect is all there is. */ + if (ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } return WS_BAD_ARGUMENT; + } if (ssh->isKeying) { ssh->error = WS_REKEYING; diff --git a/tests/regress.c b/tests/regress.c index f21381c6c..294610aad 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2744,6 +2744,20 @@ static void TestDisconnectDrainsBufferedData(void) AssertIntEQ(ret, WS_FATAL_ERROR); AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + /* With the channel gone there is no buffer left to drain, so both + * report the disconnect rather than a bad argument. */ + AssertIntEQ(ChannelRemove(ssh, ssh->channelList->channel, + WS_CHANNEL_ID_SELF), WS_SUCCESS); + AssertNull(ssh->channelList); + + ret = wolfSSH_stream_peek(ssh, NULL, 1); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); } @@ -2773,6 +2787,9 @@ static void TestDisconnectBlocksEverySend(void) AssertNotNull(ssh); AddSessionChannel(ssh); channelId = ssh->channelList->channel; + /* Past userauth, or the message filter blocks the sends on its own and + * the wire check below proves nothing. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; MemIoInit(&io, NULL, 0, out, sizeof(out)); wolfSSH_SetIOReadCtx(ssh, &io); @@ -3100,6 +3117,9 @@ static void TestShutdownQuietAfterDisconnect(void) AssertNotNull(ssh); AddSessionChannel(ssh); channel = ssh->channelList; + /* Past userauth, or the message filter blocks the teardown on its own + * and the wire check below proves nothing. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, in, sizeof(in)); From 9e845f614db338e0654e3f318b012a6ecccc6bf7 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 26 Aug 2026 13:55:23 -0700 Subject: [PATCH 08/11] Disconnect outranks a stuck rekey wolfSSH_stream_peek() and wolfSSH_stream_read() test isKeying before disconnected, so a peer that rekeys then disconnects wedges both: only NEWKEYS clears isKeying and none is coming. Callers spin on WS_REKEYING and never get the buffered data. Both gates and read's copy step now defer to disconnected. Covered by TestDisconnectOutranksRekey. Issue: F-8837 --- src/ssh.c | 16 ++++++++--- tests/regress.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index 360d0b416..f828d1c21 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1257,7 +1257,10 @@ int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz) return WS_BAD_ARGUMENT; } - if (ssh->isKeying) { + /* A rekey the peer abandoned with a disconnect never completes, since + * only NEWKEYS clears the flag. Report the dead session instead, or the + * caller turns the crank forever. */ + if (ssh->isKeying && !ssh->disconnected) { ssh->error = WS_REKEYING; return WS_REKEYING; } @@ -1324,7 +1327,9 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) return WS_ERROR; } - if (ssh->isKeying) { + /* See wolfSSH_stream_peek(): a disconnect ends a rekey that can no + * longer finish, so it outranks it here too. */ + if (ssh->isKeying && !ssh->disconnected) { ssh->error = WS_REKEYING; return WS_FATAL_ERROR; } @@ -1374,8 +1379,11 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) } } - /* update internal input buffer based on data read */ - if (ret == WS_SUCCESS && !ssh->isKeying) { + /* update internal input buffer based on data read. DoReceive() above may + * have started a rekey, which holds the copy back -- unless a disconnect + * came with it, since then the rekey never finishes and the buffered data + * would never be handed back. */ + if (ret == WS_SUCCESS && (!ssh->isKeying || ssh->disconnected)) { int n; n = min(bufSz, inputBuffer->length - inputBuffer->idx); diff --git a/tests/regress.c b/tests/regress.c index 294610aad..e4714c309 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3455,6 +3455,80 @@ static void TestShutdownFlushesQueuedDisconnect(void) } +/* A rekey that the peer abandons with a DISCONNECT never completes: only + * NEWKEYS clears isKeying, and nothing more arrives. The read calls test + * isKeying first, so they report WS_REKEYING forever and the caller's + * "keep turning the crank" branch spins for the life of the connection. + * A dead session outranks a rekey that can no longer finish. */ +static void TestDisconnectOutranksRekey(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte in[128]; + byte out[256]; + byte payload[32]; + byte data[64]; + word32 inSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* Channel data that arrived before the rekey started. */ + WMEMSET(payload, 'a', sizeof(payload)); + AssertIntEQ(ChannelPutData(channel, payload, sizeof(payload)), WS_SUCCESS); + + /* The peer's KEXINIT, the way DoKexInit records it. */ + ssh->isKeying |= WOLFSSH_PEER_IS_KEYING; + + /* DISCONNECT is a transport-generic message, so the rekey filter in + * IsMessageAllowed() lets it through. */ + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(DoReceive(ssh), WS_FATAL_ERROR); + AssertTrue(ssh->disconnected); + /* The rekey is stuck: no NEWKEYS is ever coming. */ + AssertTrue(ssh->isKeying != 0); + io.outSz = 0; + + /* What arrived before the disconnect is still the caller's. */ + ret = wolfSSH_stream_peek(ssh, data, sizeof(data)); + AssertIntEQ(ret, (int)sizeof(payload)); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, (int)sizeof(payload)); + + /* Drained, so both report the disconnect instead of a rekey that will + * never finish. */ + ret = wolfSSH_stream_peek(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + /* The drain stayed quiet, as it does outside a rekey. */ + AssertIntEQ(io.outSz, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + /* A flush that is itself short owns ssh->error. Callers gate their retry on * WS_WANT_WRITE, so the disconnect gate must not overwrite it. */ static void TestShutdownKeepsFlushWantWrite(void) @@ -7292,6 +7366,7 @@ int main(int argc, char** argv) TestQueuedDisconnectFlushes(); TestShutdownFlushesQueuedDisconnect(); TestShutdownKeepsFlushWantWrite(); + TestDisconnectOutranksRekey(); #if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM) TestTerminalResizeBlockedAfterDisconnect(); #endif From 39e3fb8acd937ef571034d15966261564ab7f42e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 26 Aug 2026 14:34:49 -0700 Subject: [PATCH 09/11] Say what the disconnect contract covers The contract in ssh.h claimed more than the code does. A CHANNEL_EOF already received outranks the drain, so both stream reads report WS_EOF with data still buffered, and wolfSSH_accept()/wolfSSH_connect() never look at the flag at all. Both fixes belong with the channel EOF work in #1195; until then the header says what is really gated. Issue: F-8837 --- wolfssh/internal.h | 9 +++++---- wolfssh/ssh.h | 9 ++++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 512f3e343..76e8b2251 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1073,11 +1073,12 @@ struct WOLFSSH { #endif byte connReset; byte isClosed; - /* Set when a DISCONNECT is sent or received. Gates every public send - * call, so nothing more goes out. Reads still hand back what arrived + /* Set when a DISCONNECT is sent or received. Gates the public send + * calls, so nothing more goes out. Reads still hand back what arrived * before the disconnect; the head-of-list reads report it once their - * buffer runs dry. wolfSSH_worker() is not gated, the shutdown paths - * pump it. */ + * buffer runs dry, unless a CHANNEL_EOF arrived first. wolfSSH_worker(), + * wolfSSH_accept() and wolfSSH_connect() are not gated; the shutdown + * paths pump the worker. */ byte disconnected; /* Set once SendDisconnect() has bundled our own DISCONNECT into the * output buffer, so a short send can still be flushed. The flag above diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index f5e2a0c8b..083931a57 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -559,11 +559,14 @@ WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); /* A disconnect, sent or received, ends the session. Nothing more goes out: - * every send call in this header, above this comment and below it, reports - * WS_DISCONNECT from then on. Reads are not gated, so channel data that + * wolfSSH_shutdown() above this comment, and every send call below it, + * report WS_DISCONNECT from then on. wolfSSH_accept() and + * wolfSSH_connect() are not gated; do not drive the handshake after a + * disconnect. Reads are not gated either, so channel data that * arrived before the disconnect can still be drained; wolfSSH_stream_read() * and wolfSSH_stream_peek() report WS_DISCONNECT once their buffer runs - * dry. RFC 4253 section 11.1. */ + * dry. A CHANNEL_EOF already received outranks that drain: both report + * WS_EOF with data possibly still buffered. RFC 4253 section 11.1. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); From 47cabe5c1b458343f31fbe51ace70ccb8ba60a84 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 26 Aug 2026 14:34:49 -0700 Subject: [PATCH 10/11] Report the disconnect with no channel to drop wolfSSH_shutdown() set ssh->error to WS_DISCONNECT only inside the channel branch, so a flush that emptied the buffer with the channel already retired left behind the WS_WANT_WRITE that queued it. echoserver and sftpclient read that error and burn ten wolfSSH_worker() calls on a write that is already done. TestShutdownFlushesWithNoChannel asserts it. Issue: F-8837 --- src/ssh.c | 11 +++++++---- tests/regress.c | 3 +++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index f828d1c21..6dcd71773 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1161,13 +1161,16 @@ int wolfSSH_shutdown(WOLFSSH* ssh) * and the wait for a close that will not come. RFC 4253 section 11.1. */ if (channel != NULL && ssh->disconnected) { WLOG(WS_LOG_DEBUG, "Session already disconnected, nothing to send"); - /* An unfinished flush owns ssh->error. Callers gate their retry on - * WS_WANT_WRITE, so overwriting it strands the queued disconnect. */ - if (flushRet == WS_SUCCESS) - ssh->error = WS_DISCONNECT; channel = NULL; } + /* Report the dead session with or without a channel to drop: callers + * gate their retry on ssh->error, and the flush above may have just + * emptied the output buffer they would be retrying for. An unfinished + * flush owns the error instead, since that retry is still owed. */ + if (ssh != NULL && ssh->disconnected && flushRet == WS_SUCCESS) + ssh->error = WS_DISCONNECT; + /* if channel close was not already sent then send it */ if (channel != NULL && !channel->closeTxd) { if (ret == WS_SUCCESS) { diff --git a/tests/regress.c b/tests/regress.c index e4714c309..f765ba309 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3304,6 +3304,9 @@ static void TestShutdownFlushesWithNoChannel(void) AssertIntEQ(ret, WS_CHANNEL_CLOSED); AssertFalse(wolfSSH_OutputPending(ssh)); AssertIntEQ(out[LENGTH_SZ + 1], MSGID_DISCONNECT); + /* The flush finished, so the WS_WANT_WRITE that queued it is stale. + * Leaving it sends the caller back for a write that is already done. */ + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); From e3ed78ecdb457ba5ee6a22320759b5d5a24ee8de Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 26 Aug 2026 14:34:57 -0700 Subject: [PATCH 11/11] Clear disconnectTxd once the buffer drains The flag was set in SendDisconnect() and never cleared, so it meant "a disconnect was sent" rather than "a flush is owed". Once ours had gone out, the next teardown call still pushed whatever the internal senders had queued behind it: measured, a CHANNEL_EOF from DoChannelEof() went on the wire after the disconnect. wolfSSH_SendPacket() now clears it. Issue: F-8837 --- src/internal.c | 5 ++++ tests/regress.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 3 ++- 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index 520f18abc..a437aaea7 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4319,6 +4319,11 @@ int wolfSSH_SendPacket(WOLFSSH* ssh) ssh->outputBuffer.plainSz = 0; + /* The buffer is empty, so our disconnect, if one was in it, has gone + * out and no flush is owed. Leaving this set hands the next teardown + * call a licence to push whatever gets queued next. */ + ssh->disconnectTxd = 0; + WLOG(WS_LOG_DEBUG, "SB: Shrinking output buffer"); ShrinkBuffer(&ssh->outputBuffer, 0); return HighwaterCheck(ssh, WOLFSSH_HWSIDE_TRANSMIT); diff --git a/tests/regress.c b/tests/regress.c index f765ba309..c507622a9 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3532,6 +3532,70 @@ static void TestDisconnectOutranksRekey(void) } +/* disconnectTxd means "a flush is owed", not "a disconnect was sent". Once + * ours has gone out, a teardown call must not push whatever the internal + * senders queued behind it. */ +static void TestDisconnectTxdClearsOnFlush(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte out[512]; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + channel = ssh->channelList; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* Our disconnect short-sends, so a flush is owed. */ + MemSendWantWriteCount = 1; + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_WANT_WRITE); + AssertTrue(ssh->disconnectTxd); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(io.outSz, 0); + + /* The caller's retry loop pumps it out in full, so nothing is owed. */ + AssertIntEQ(wolfSSH_SendPacket(ssh), WS_SUCCESS); + AssertFalse(wolfSSH_OutputPending(ssh)); + AssertFalse(ssh->disconnectTxd); + AssertIntEQ(out[LENGTH_SZ + 1], MSGID_DISCONNECT); + io.outSz = 0; + + /* An in-flight CHANNEL_EOF draws a reply out of DoChannelEof(); the + * internal senders are not behind the disconnect gate. It short-sends, + * so it sits in the output buffer. */ + MemSendWantWriteCount = 1; + AssertIntEQ(SendChannelEof(ssh, channel->peerChannel), WS_WANT_WRITE); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(io.outSz, 0); + + /* Teardown must leave it there: the disconnect is already gone, so this + * would be traffic after it. RFC 4253 section 11.1. */ + ret = wolfSSH_shutdown(ssh); + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, 0); + AssertTrue(wolfSSH_OutputPending(ssh)); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + /* A flush that is itself short owns ssh->error. Callers gate their retry on * WS_WANT_WRITE, so the disconnect gate must not overwrite it. */ static void TestShutdownKeepsFlushWantWrite(void) @@ -7369,6 +7433,7 @@ int main(int argc, char** argv) TestQueuedDisconnectFlushes(); TestShutdownFlushesQueuedDisconnect(); TestShutdownKeepsFlushWantWrite(); + TestDisconnectTxdClearsOnFlush(); TestDisconnectOutranksRekey(); #if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM) TestTerminalResizeBlockedAfterDisconnect(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 76e8b2251..b0b30a437 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1081,7 +1081,8 @@ struct WOLFSSH { * paths pump the worker. */ byte disconnected; /* Set once SendDisconnect() has bundled our own DISCONNECT into the - * output buffer, so a short send can still be flushed. The flag above + * output buffer, and cleared once wolfSSH_SendPacket() drains it, so it + * means "a flush is owed" rather than "one was sent". The flag above * cannot stand in for this: it does not say whose disconnect it was, * and a peer's leaves only unrelated traffic queued. */ byte disconnectTxd;