From 16f3e3ee6ba1ef6b09bcfa7bd8c2b45c2a819efa Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 12:53:54 -0700 Subject: [PATCH 01/17] Hoist the shared unit test doubles The receive mock and the packet builders sit ahead of every endpoint region rather than inside the server half. A staged-packet IORecv and a plaintext packet builder have nothing to do with which endpoint is built, and the tests that follow reach for them from both halves. - Marked WS_MAYBE_UNUSED, since either half can be the only user of any one. - WantWriteIoSend() joins DiscardIoSend() among the send mocks. - A relocation only: no test body changes, and the moved text is unchanged bar the annotation. Read it with --color-moved. --- ide/mplabx/wolfssh.c | 14 +++- tests/unit.c | 157 ++++++++++++++++++++++--------------------- 2 files changed, 92 insertions(+), 79 deletions(-) diff --git a/ide/mplabx/wolfssh.c b/ide/mplabx/wolfssh.c index 70a75d5de..a2bbbd774 100644 --- a/ide/mplabx/wolfssh.c +++ b/ide/mplabx/wolfssh.c @@ -785,9 +785,19 @@ void APP_Tasks ( void ) break; /* no need to spend time attempting to pull data * if there is still pending sends */ } + /* Drain what the peer sent before tearing down, the same as + * the worker site below. The two later WS_EOF checks need no + * guard: one sits behind a peek that already found data and + * breaks to re-peek next pass, the other behind a peek that + * already reported the channel dry. */ if (error == WS_EOF) { - appData.state = APP_SSH_CLEANUP; - break; + int peekRet = wolfSSH_stream_peek(ssh, peek_buf, + sizeof(peek_buf)); + + if (peekRet != WS_REKEYING && peekRet <= 0) { + appData.state = APP_SSH_CLEANUP; + break; + } } } diff --git a/tests/unit.c b/tests/unit.c index beea0ea59..9f12a6fe8 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -4357,6 +4357,15 @@ static int DiscardIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) (void)ssh; (void)buf; (void)ctx; return (int)sz; } +/* Reports a short (would-block) write, so an adjust bundled into the output + * buffer reaches the transport but does not fully flush: wolfSSH_SendPacket() + * maps WS_CBIO_ERR_WANT_WRITE to WS_WANT_WRITE. */ +static WS_MAYBE_UNUSED int WantWriteIoSend(WOLFSSH* ssh, void* buf, word32 sz, + void* ctx) +{ + (void)ssh; (void)buf; (void)sz; (void)ctx; + return WS_CBIO_ERR_WANT_WRITE; +} static int test_DoChannelExtendedData_overflow(void) { @@ -4617,6 +4626,77 @@ static int test_DoChannelWindowAdjust_overflow(void) /* The tests below drive a server-side session that sends a window adjust. * With NO_WOLFSSH_SERVER the message filter has no server branch, so every * message on such a session is refused and the tests cannot run. */ +/* Shared test doubles. Endpoint-agnostic on purpose: a receive mock and a + * handful of plaintext packet builders belong to both halves of this file, so + * they sit ahead of every NO_WOLFSSH_SERVER / NO_WOLFSSH_CLIENT region rather + * than inside one of them. */ + +/* A crafted transport packet staged for the receive path, and the running + * offset PacketIoRecv has delivered. */ +static const byte* s_recvPkt = NULL; +static word32 s_recvPktSz = 0; +static word32 s_recvPktOff = 0; + +/* IORecv mock that hands out the staged packet, then reports WS_WANT_READ once + * it is drained so a further DoReceive() does not block on a live socket. */ +static WS_MAYBE_UNUSED int PacketIoRecv(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + word32 avail, n; + + WOLFSSH_UNUSED(ssh); + WOLFSSH_UNUSED(ctx); + + avail = s_recvPktSz - s_recvPktOff; + if (avail == 0) + return WS_CBIO_ERR_WANT_READ; + + n = (sz < avail) ? sz : avail; + WMEMCPY(buf, s_recvPkt + s_recvPktOff, n); + s_recvPktOff += n; + return (int)n; +} + +/* Builds a plaintext CHANNEL_EXTENDED_DATA (stderr) SSH packet addressed to + * channelId, carrying 10 bytes of payload set to fill, into pkt (needs 32 + * bytes) and returns its size. A bare session negotiates no cipher + * (peerEncryptId ID_NONE, so Decrypt() is a passthrough) and no MAC + * (peerMacSz 0), so the packet goes on the wire in the clear. The total size is + * a multiple of the 8-byte MIN_BLOCK_SZ and the padding meets MIN_PAD_LENGTH. + * + * Layout: [len=28][pad=4][msgid=95][chan][type=1][dataSz=10][data*10][pad*4]. */ +static WS_MAYBE_UNUSED word32 BuildExtDataStderrPacket(byte* pkt, word32 channelId, byte fill) +{ + word32 i = 0; + + /* packet_length = padLen(1) + msgid(1) + chan(4) + type(4) + dataSz(4) + * + data(10) + padding(4) = 28. */ + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x1C; + pkt[i++] = 0x04; /* padding length */ + pkt[i++] = MSGID_CHANNEL_EXTENDED_DATA; + + pkt[i++] = (byte)((channelId >> 24) & 0xFF); + pkt[i++] = (byte)((channelId >> 16) & 0xFF); + pkt[i++] = (byte)((channelId >> 8) & 0xFF); + pkt[i++] = (byte)( channelId & 0xFF); + + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; + pkt[i++] = (byte)CHANNEL_EXTENDED_DATA_STDERR; /* data type = stderr (1) */ + + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x0A; /* 10 */ + + WMEMSET(pkt + i, fill, 10); i += 10; + WMEMSET(pkt + i, 0x00, 4); i += 4; /* padding bytes */ + + return i; /* 32 */ +} +/* Fails every send, discarding whatever was bundled (WS_CBIO_ERR_GENERAL makes + * wolfSSH_SendPacket() shrink the output buffer), so an adjust sent through it + * never reaches the peer. */ +static WS_MAYBE_UNUSED int FailIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + (void)ssh; (void)buf; (void)sz; (void)ctx; + return WS_CBIO_ERR_GENERAL; +} #ifndef NO_WOLFSSH_SERVER /* An unknown extended data type must be ignored (consumed and discarded) per @@ -5469,14 +5549,6 @@ static int test_ChannelWindowSharedStdoutStderr(void) } -/* Fails every send, discarding whatever was bundled (WS_CBIO_ERR_GENERAL makes - * wolfSSH_SendPacket() shrink the output buffer), so an adjust sent through it - * never reaches the peer. */ -static int FailIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) -{ - (void)ssh; (void)buf; (void)sz; (void)ctx; - return WS_CBIO_ERR_GENERAL; -} /* The peer's window only grows by the WINDOW_ADJUSTs it receives, so credit * counted locally but never sent stalls the channel for good. Covers the two @@ -5625,15 +5697,6 @@ static int test_ChannelExtDataCredit(void) return result; } -/* Reports a short (would-block) write, so an adjust bundled into the output - * buffer reaches the transport but does not fully flush: wolfSSH_SendPacket() - * maps WS_CBIO_ERR_WANT_WRITE to WS_WANT_WRITE. */ -static int WantWriteIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) -{ - (void)ssh; (void)buf; (void)sz; (void)ctx; - return WS_CBIO_ERR_WANT_WRITE; -} - /* A drain whose window-adjust send only partially completes (WS_WANT_WRITE) * must still report the bytes copied -- they are already in the caller's buffer * -- and leave wolfSSH_get_error() at WS_WANT_WRITE so the caller knows a flush @@ -5861,64 +5924,6 @@ static int test_ChannelReadExtBadArgs(void) #ifndef NO_WOLFSSH_SERVER -/* A crafted transport packet staged for the receive path, and the running - * offset PacketIoRecv has delivered. */ -static const byte* s_recvPkt = NULL; -static word32 s_recvPktSz = 0; -static word32 s_recvPktOff = 0; - -/* IORecv mock that hands out the staged packet, then reports WS_WANT_READ once - * it is drained so a further DoReceive() does not block on a live socket. */ -static int PacketIoRecv(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) -{ - word32 avail, n; - - WOLFSSH_UNUSED(ssh); - WOLFSSH_UNUSED(ctx); - - avail = s_recvPktSz - s_recvPktOff; - if (avail == 0) - return WS_CBIO_ERR_WANT_READ; - - n = (sz < avail) ? sz : avail; - WMEMCPY(buf, s_recvPkt + s_recvPktOff, n); - s_recvPktOff += n; - return (int)n; -} - -/* Builds a plaintext CHANNEL_EXTENDED_DATA (stderr) SSH packet addressed to - * channelId, carrying 10 bytes of payload set to fill, into pkt (needs 32 - * bytes) and returns its size. A bare session negotiates no cipher - * (peerEncryptId ID_NONE, so Decrypt() is a passthrough) and no MAC - * (peerMacSz 0), so the packet goes on the wire in the clear. The total size is - * a multiple of the 8-byte MIN_BLOCK_SZ and the padding meets MIN_PAD_LENGTH. - * - * Layout: [len=28][pad=4][msgid=95][chan][type=1][dataSz=10][data*10][pad*4]. */ -static word32 BuildExtDataStderrPacket(byte* pkt, word32 channelId, byte fill) -{ - word32 i = 0; - - /* packet_length = padLen(1) + msgid(1) + chan(4) + type(4) + dataSz(4) - * + data(10) + padding(4) = 28. */ - pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x1C; - pkt[i++] = 0x04; /* padding length */ - pkt[i++] = MSGID_CHANNEL_EXTENDED_DATA; - - pkt[i++] = (byte)((channelId >> 24) & 0xFF); - pkt[i++] = (byte)((channelId >> 16) & 0xFF); - pkt[i++] = (byte)((channelId >> 8) & 0xFF); - pkt[i++] = (byte)( channelId & 0xFF); - - pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; - pkt[i++] = (byte)CHANNEL_EXTENDED_DATA_STDERR; /* data type = stderr (1) */ - - pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x0A; /* 10 */ - - WMEMSET(pkt + i, fill, 10); i += 10; - WMEMSET(pkt + i, 0x00, 4); i += 4; /* padding bytes */ - - return i; /* 32 */ -} /* Integration (M-3): a peer sending stderr on a channel that is not the head of * the channel list must not surface as stream data. wolfSSH_stream_read() reads @@ -6384,9 +6389,7 @@ static int test_ChannelReadExtClearsStaleWantWrite(void) wolfSSH_CTX_free(ctx); return result; } - #endif /* NO_WOLFSSH_SERVER */ - static int test_SendChannelData_eofTxd(void) { WOLFSSH_CTX* ctx = NULL; From ad09c2d25d81e2f11640449c6857143cefd3b29d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:03:20 -0700 Subject: [PATCH 02/17] Stop echoing the peer's channel EOF DoChannelEof() latches eofRxd and reports WS_EOF. It used to answer with an EOF of its own, which latched eofTxd, after which the "Cannot send data after EOF" gates in SendChannelData() and SendChannelExtendedData() refused every later send: a peer that half-closed could never be replied to. RFC 4254 section 5.3 leaves that reply to the application. - wolfSSH_worker() carries WS_EOF out with the channel it belongs to, and does not mask it during a rekey; the event is raised once, on arrival. - DoReceiveHandshake() absorbs it in the three accept/connect states that can run with a channel already open. A legal EOF is not a handshake failure. - wolfSSH_shutdown() treats it as the response it was waiting for. - Tests cover the half-close, the channel id, the rekey case, the callback, which had no test anywhere in the tree, and a handshake that survives an EOF. - test_ConnectSurvivesChannelEof() sits in its own client region, since wolfSSH_connect() is not built with NO_WOLFSSH_CLIENT. Issue: F-1687 --- src/internal.c | 7 +- src/ssh.c | 52 ++++-- tests/unit.c | 497 +++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 28 +++ 4 files changed, 565 insertions(+), 19 deletions(-) diff --git a/src/internal.c b/src/internal.c index 60f682bb0..91a826f24 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11353,10 +11353,8 @@ static int DoChannelEof(WOLFSSH* ssh, if (ret == WS_SUCCESS) { channel->eofRxd = 1; - if (!channel->eofTxd) { - ret = SendChannelEof(ssh, channel->peerChannel); - } ssh->lastRxId = channelId; + ret = WS_EOF; } WLOG(WS_LOG_DEBUG, "Leaving DoChannelEof(), ret = %d", ret); @@ -13063,7 +13061,8 @@ int DoReceive(WOLFSSH* ssh) ssh->error = ret; if (ret < 0 && !(ret == WS_CHAN_RXD || ret == WS_EXTDATA || ret == WS_CHANNEL_CLOSED || ret == WS_WANT_WRITE || - ret == WS_REKEYING || ret == WS_WANT_READ)) { + ret == WS_REKEYING || ret == WS_WANT_READ || + ret == WS_EOF)) { ret = WS_FATAL_ERROR; } break; diff --git a/src/ssh.c b/src/ssh.c index 32c234b2a..6337cf7ba 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -550,6 +550,23 @@ int wolfSSH_CTX_UseTpmHostKey(WOLFSSH_CTX* ctx, #endif /* WOLFSSH_TPM */ +#if !defined(NO_WOLFSSH_SERVER) || !defined(NO_WOLFSSH_CLIENT) + +/* A peer's CHANNEL_EOF is legal once its channel is open, RFC 4254 section + * 5.3, not a handshake failure. */ +static int DoReceiveHandshake(WOLFSSH* ssh) +{ + int ret = DoReceive(ssh); + + if (ret == WS_EOF) + ret = WS_SUCCESS; + + return ret; +} + +#endif /* !NO_WOLFSSH_SERVER || !NO_WOLFSSH_CLIENT */ + + /* Defined below, ahead of both drivers; either can be the only one built. */ static int SendAfterDisconnect(WOLFSSH* ssh); @@ -720,7 +737,7 @@ int wolfSSH_accept(WOLFSSH* ssh) case ACCEPT_SERVER_CHANNEL_ACCEPT_SENT: while (ssh->clientState < CLIENT_DONE) { - if (DoReceive(ssh) < 0) { + if (DoReceiveHandshake(ssh) < 0) { WLOG(WS_LOG_DEBUG, acceptError, "SERVER_CHANNEL_ACCEPT_SENT", ssh->error); return WS_FATAL_ERROR; @@ -1042,7 +1059,7 @@ int wolfSSH_connect(WOLFSSH* ssh) case CONNECT_CLIENT_CHANNEL_OPEN_SESSION_SENT: while (ssh->serverState < SERVER_CHANNEL_OPEN_DONE) { - if (DoReceive(ssh) < WS_SUCCESS) { + if (DoReceiveHandshake(ssh) < WS_SUCCESS) { WLOG(WS_LOG_DEBUG, connectError, "CLIENT_CHANNEL_OPEN_SESSION_SENT", ssh->error); return WS_FATAL_ERROR; @@ -1099,7 +1116,7 @@ int wolfSSH_connect(WOLFSSH* ssh) case CONNECT_CLIENT_CHANNEL_REQUEST_SENT: while (ssh->serverState < SERVER_DONE) { - if (DoReceive(ssh) < WS_SUCCESS) { + if (DoReceiveHandshake(ssh) < WS_SUCCESS) { WLOG(WS_LOG_DEBUG, connectError, "CLIENT_CHANNEL_REQUEST_SENT", ssh->error); return WS_FATAL_ERROR; @@ -1213,7 +1230,7 @@ int wolfSSH_shutdown(WOLFSSH* ssh) * response to SendChannelClose */ if (channel != NULL && ret == WS_SUCCESS) { ret = wolfSSH_worker(ssh, NULL); - if (ret == WS_CHAN_RXD) { + if (ret == WS_CHAN_RXD || ret == WS_EOF) { /* received response */ ret = WS_SUCCESS; } @@ -3631,17 +3648,20 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) /* If receive only wanted read or delivered channel data, still try to * flush any pending outbound packets. */ - if (ret == WS_SUCCESS || ret == WS_WANT_READ || ret == WS_CHAN_RXD) { + if (ret == WS_SUCCESS || ret == WS_WANT_READ || ret == WS_CHAN_RXD + || ret == WS_EOF) { int sendRet = WS_SUCCESS; if (ssh->outputBuffer.length != 0) sendRet = wolfSSH_SendPacket(ssh); /* If send is back-pressured, immediately try another receive to pick - * up potential window-adjusts and then return the send status. */ + * up potential window-adjusts and then return the send status. The + * send status wins; a peer EOF stays latched on the channel. */ if (sendRet == WS_WANT_WRITE || sendRet == WS_WINDOW_FULL) { int recv2 = DoReceive(ssh); - if (recv2 == WS_SUCCESS || recv2 == WS_WANT_READ || recv2 == WS_CHAN_RXD) + if (recv2 == WS_SUCCESS || recv2 == WS_WANT_READ || recv2 == WS_CHAN_RXD + || recv2 == WS_EOF) ret = sendRet; else ret = recv2; @@ -3655,18 +3675,20 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) } #endif /* WOLFSSH_TEST_BLOCK */ - /* WS_EXTDATA reports the channel too, so a multi-channel caller can route - * the drain to wolfSSH_ChannelIdReadExt(). */ - if (ret == WS_SUCCESS || ret == WS_CHAN_RXD || ret == WS_EXTDATA) { + /* WS_EXTDATA and WS_EOF report the channel too, so a multi-channel caller + * can route the drain, or see which channel half-closed. */ + if (ret == WS_SUCCESS || ret == WS_CHAN_RXD || ret == WS_EXTDATA + || ret == WS_EOF) { if (channelId != NULL) { *channelId = ssh->lastRxId; } - /* WS_EXTDATA is raised once, on arrival; masking it would strand the - * buffered stderr and its window credit. A disconnect cannot be seen - * here: the gate at the top returns before this, and the DISCONNECT - * that sets the flag mid-pass leaves ret fatal. */ - if (ssh->isKeying && ret != WS_EXTDATA) { + /* WS_EXTDATA and WS_EOF are raised once, on arrival; masking either + * strands the event, and the stderr window credit with it. A + * disconnect cannot be seen here: the gate at the top returns before + * this, and the DISCONNECT that sets the flag mid-pass leaves ret + * fatal. */ + if (ssh->isKeying && ret != WS_EXTDATA && ret != WS_EOF) { ssh->error = WS_REKEYING; return WS_REKEYING; } diff --git a/tests/unit.c b/tests/unit.c index 9f12a6fe8..3dbaa9279 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -4689,6 +4689,57 @@ static WS_MAYBE_UNUSED word32 BuildExtDataStderrPacket(byte* pkt, word32 channel return i; /* 32 */ } + +/* Builds a plaintext CHANNEL_DATA packet for channelId carrying 10 bytes of + * fill into pkt (needs 32 bytes), returning its size. Same plaintext-session + * reasoning as BuildExtDataStderrPacket(). + * + * Layout: [len=28][pad=8][msgid=94][chan][dataSz=10][data*10][pad*8]. */ +static WS_MAYBE_UNUSED word32 BuildChannelDataPacket(byte* pkt, word32 channelId, byte fill) +{ + word32 i = 0; + + /* packet_length = padLen(1) + msgid(1) + chan(4) + dataSz(4) + data(10) + * + padding(8) = 28. */ + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x1C; + pkt[i++] = 0x08; /* padding length */ + pkt[i++] = MSGID_CHANNEL_DATA; + + pkt[i++] = (byte)((channelId >> 24) & 0xFF); + pkt[i++] = (byte)((channelId >> 16) & 0xFF); + pkt[i++] = (byte)((channelId >> 8) & 0xFF); + pkt[i++] = (byte)( channelId & 0xFF); + + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x0A; /* 10 */ + + WMEMSET(pkt + i, fill, 10); i += 10; + WMEMSET(pkt + i, 0x00, 8); i += 8; /* padding bytes */ + + return i; /* 32 */ +} + +/* Builds a plaintext CHANNEL_EOF packet for channelId into pkt (needs 16 + * bytes), returning its size. + * + * Layout: [len=12][pad=6][msgid=96][chan][pad*6]. */ +static WS_MAYBE_UNUSED word32 BuildChannelEofPacket(byte* pkt, word32 channelId) +{ + word32 i = 0; + + /* packet_length = padLen(1) + msgid(1) + chan(4) + padding(6) = 12. */ + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x0C; + pkt[i++] = 0x06; /* padding length */ + pkt[i++] = MSGID_CHANNEL_EOF; + + pkt[i++] = (byte)((channelId >> 24) & 0xFF); + pkt[i++] = (byte)((channelId >> 16) & 0xFF); + pkt[i++] = (byte)((channelId >> 8) & 0xFF); + pkt[i++] = (byte)( channelId & 0xFF); + + WMEMSET(pkt + i, 0x00, 6); i += 6; /* padding bytes */ + + return i; /* 16 */ +} /* Fails every send, discarding whatever was bundled (WS_CBIO_ERR_GENERAL makes * wolfSSH_SendPacket() shrink the output buffer), so an adjust sent through it * never reaches the peer. */ @@ -6389,6 +6440,416 @@ static int test_ChannelReadExtClearsStaleWantWrite(void) wolfSSH_CTX_free(ctx); return result; } + +/* A received SSH_MSG_CHANNEL_EOF is a notification, not a command to answer in + * kind: echoing it sets our eofTxd, and SendChannelData() refuses to send once + * that is set. RFC 4254 section 5.3 closes each direction independently. + * + * Covers the read side too: data buffered ahead of the EOF is still owed to + * the caller, so the EOF is reported only once that buffer is drained. */ +static int test_ChannelEofHalfClose(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 reportedId = 0xFFFFFFFF; + word32 pktSz; + byte pkt[48]; + byte payload[4] = { 0x10, 0x11, 0x12, 0x13 }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1470; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1471; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1472; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1473; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + /* The peer sends its last data and then stops talking, back to back. */ + pktSz = BuildChannelDataPacket(pkt, ch->channel, 0x44); + pktSz += BuildChannelEofPacket(pkt + pktSz, ch->channel); + s_recvPkt = pkt; + s_recvPktSz = pktSz; + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, &reportedId); + if (ret != WS_CHAN_RXD) { result = -1474; goto done; } + if (reportedId != ch->channel) { result = -1475; goto done; } + + reportedId = 0xFFFFFFFF; + ret = wolfSSH_worker(ssh, &reportedId); + if (ret != WS_EOF) { result = -1476; goto done; } + /* The caller must be able to tell which channel half-closed. */ + if (reportedId != ch->channel) { result = -1477; goto done; } + if (!ch->eofRxd) { result = -1478; goto done; } + /* No auto-echo: our sending direction is untouched. */ + if (ch->eofTxd) { result = -1479; goto done; } + + /* Half-closed one way: we can still answer the peer. */ + ret = wolfSSH_stream_send(ssh, payload, (word32)sizeof(payload)); + if (ret != (int)sizeof(payload)) { result = -1487; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +/* A peer may half-close its channel before it makes its shell/exec/subsystem + * request, RFC 4254 section 5.3. DoChannelEof() reports that as WS_EOF, and + * DoReceive() passes it through, but the accept loop must not read a negative + * return as a dead handshake and tear the session down. */ +static int test_AcceptSurvivesChannelEof(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1640; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1641; goto done; } + + /* Userauth is done and the peer's channel is open. The accept loop is + * waiting on the channel request. */ + ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; + ssh->clientState = CLIENT_CHANNEL_OPEN_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1642; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1643; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch->channel); + s_recvPktOff = 0; + + ret = wolfSSH_accept(ssh); + + /* The staged packet is the only input, so the accept stops wanting more + * of it. What matters is that WS_EOF is not what it stopped on. */ + if (ret != WS_FATAL_ERROR) { result = -1644; goto done; } + if (wolfSSH_get_error(ssh) == WS_EOF) { result = -1645; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_READ) { result = -1646; goto done; } + + /* The half-close still reached the channel. */ + if (!ch->eofRxd) { result = -1647; goto done; } + if (!wolfSSH_ChannelGetEof(ch)) { result = -1648; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + + +/* The client's two handshake loops take DoReceiveHandshake() as well, and a + * server may half-close before answering the channel request. Same shape as + * test_AcceptSurvivesChannelEof(), from the other end. Guarded on its own: + * wolfSSH_connect() is not built with NO_WOLFSSH_CLIENT. */ +#ifndef NO_WOLFSSH_CLIENT +static int test_ConnectSurvivesChannelEof(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1770; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1771; goto done; } + + /* The connect loop is waiting on the channel request to be answered. */ + ssh->connectState = CONNECT_CLIENT_CHANNEL_REQUEST_SENT; + ssh->serverState = SERVER_CHANNEL_OPEN_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1772; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1773; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch->channel); + s_recvPktOff = 0; + + ret = wolfSSH_connect(ssh); + + /* As with accept: it stops wanting more input, not on the EOF. */ + if (ret != WS_FATAL_ERROR) { result = -1774; goto done; } + if (wolfSSH_get_error(ssh) == WS_EOF) { result = -1775; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_READ) { result = -1776; goto done; } + + if (!ch->eofRxd) { result = -1777; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + + +#endif /* NO_WOLFSSH_CLIENT */ + +/* wolfSSH_worker() exempts WS_EOF from the rekey mask, the same way it exempts + * WS_EXTDATA: the EOF is raised once, on arrival, so masking it as WS_REKEYING + * would strand the event. The drain guards in wolfsshd and the echoservers + * depend on this combination being reachable. */ +static int test_WorkerReportsEofChannelKeying(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 reportedId = 0xFFFFFFFF; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1600; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1601; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1602; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1603; + goto done; + } + + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch->channel); + s_recvPktOff = 0; + + /* A rekey is underway when the EOF lands. */ + ssh->isKeying = WOLFSSH_SELF_IS_KEYING; + + ret = wolfSSH_worker(ssh, &reportedId); + if (ret != WS_EOF) { result = -1604; goto done; } + if (reportedId != ch->channel) { result = -1605; goto done; } + if (!ch->eofRxd) { result = -1606; goto done; } + + /* And peek still says "rekeying", not "drained" -- which is why the drain + * guards have to tell the two apart. */ + ret = wolfSSH_stream_peek(ssh, NULL, 1); + if (ret != WS_REKEYING) { result = -1607; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +#endif /* NO_WOLFSSH_SERVER */ +#ifndef NO_WOLFSSH_SERVER +static int s_eofCbCalls = 0; +static word32 s_eofCbChannel = 0; +static void* s_eofCbCtx = NULL; + +static int EofRecordingCb(WOLFSSH_CHANNEL* channel, void* ctx) +{ + s_eofCbCalls++; + s_eofCbChannel = (channel != NULL) ? channel->channel : 0; + s_eofCbCtx = ctx; + return WS_SUCCESS; +} + +/* The channel EOF callback is the durable half of the contract: the WS_EOF + * from wolfSSH_worker() is raised once and a back-pressure status can take + * its place, but the callback fires from DoChannelEof() itself. */ +static int test_ChannelEofCallback(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + int cbCtx = 0; + byte pkt[16]; + + s_eofCbCalls = 0; + s_eofCbChannel = 0; + s_eofCbCtx = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1707; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + if (wolfSSH_CTX_SetChannelEofCb(ctx, EofRecordingCb) != WS_SUCCESS) { + result = -1708; + goto done; + } + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1709; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + if (wolfSSH_SetChannelEofCtx(ssh, &cbCtx) != WS_SUCCESS) { + result = -1710; + goto done; + } + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1711; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1712; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch->channel); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_EOF) { result = -1713; goto done; } + if (s_eofCbCalls != 1) { result = -1714; goto done; } + if (s_eofCbChannel != ch->channel) { result = -1715; goto done; } + if (s_eofCbCtx != &cbCtx) { result = -1716; goto done; } + if (!wolfSSH_ChannelGetEof(ch)) { result = -1717; goto done; } + + /* Nothing more arrives, so nothing fires it again. */ + ret = wolfSSH_worker(ssh, NULL); + if (ret == WS_EOF) { result = -1718; goto done; } + if (s_eofCbCalls != 1) { result = -1719; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_SERVER */ + + +#ifndef NO_WOLFSSH_SERVER +/* WS_EOF names its channel the way WS_CHAN_RXD does, so a caller with more + * than one channel open knows which one half-closed rather than assuming the + * head of the list. */ +static int test_WorkerReportsEofChannelId(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch0 = NULL; + WOLFSSH_CHANNEL* ch1 = NULL; + int result = 0; + int ret; + word32 reported = 0xFFFFFFFF; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1720; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1721; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch0 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch0 == NULL) { result = -1722; goto done; } + if (ChannelAppend(ssh, ch0) != WS_SUCCESS) { + ChannelDelete(ch0, ssh->ctx->heap); + result = -1723; + goto done; + } + ch0->openConfirmed = 1; + ch0->peerWindowSz = 1024; + ch0->peerMaxPacketSz = 1024; + + ch1 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch1 == NULL) { result = -1724; goto done; } + if (ChannelAppend(ssh, ch1) != WS_SUCCESS) { + ChannelDelete(ch1, ssh->ctx->heap); + result = -1725; + goto done; + } + ch1->openConfirmed = 1; + ch1->peerWindowSz = 1024; + ch1->peerMaxPacketSz = 1024; + + /* The half-close lands on the channel that is not the head. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch1->channel); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, &reported); + if (ret != WS_EOF) { result = -1726; goto done; } + if (reported != ch1->channel) { result = -1727; goto done; } + if (!ch1->eofRxd) { result = -1728; goto done; } + if (ch0->eofRxd) { result = -1729; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} #endif /* NO_WOLFSSH_SERVER */ static int test_SendChannelData_eofTxd(void) { @@ -17672,6 +18133,42 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_ChannelEofHalfClose(); + printf("ChannelEofHalfClose: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_AcceptSurvivesChannelEof(); + printf("AcceptSurvivesChannelEof: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_ConnectSurvivesChannelEof(); + printf("ConnectSurvivesChannelEof: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + + unitResult = test_WorkerReportsEofChannelKeying(); + printf("WorkerReportsEofChannelKeying: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + +#endif /* NO_WOLFSSH_SERVER */ + +#ifndef NO_WOLFSSH_SERVER + unitResult = test_ChannelEofCallback(); + printf("ChannelEofCallback: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_SERVER */ + +#ifndef NO_WOLFSSH_SERVER + unitResult = test_WorkerReportsEofChannelId(); + printf("WorkerReportsEofChannelId: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; #endif /* NO_WOLFSSH_SERVER */ unitResult = test_SendChannelData_eofTxd(); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 8e653c48f..541a6d294 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -68,6 +68,34 @@ WOLFSSH_API void wolfSSH_CTX_free(WOLFSSH_CTX* ctx); WOLFSSH_API WOLFSSH* wolfSSH_new(WOLFSSH_CTX* ctx); WOLFSSH_API void wolfSSH_free(WOLFSSH* ssh); +/* Services the connection: reads what is pending and flushes what is queued. + * Returns WS_SUCCESS, or one of several non-fatal statuses that callers must + * not treat as errors: + * WS_CHAN_RXD channel data arrived; read it with wolfSSH_stream_read() + * or wolfSSH_ChannelIdRead() + * WS_EXTDATA extended (stderr) data arrived; drain it with + * wolfSSH_ChannelIdReadExt() + * WS_EOF the peer half-closed a channel; it sends no more data, + * but the channel is still open for sending. Raised once, + * on arrival, and a back-pressure status from the flush + * that follows can supersede it, so an application that + * must not miss one tests wolfSSH_ChannelGetEof() or takes + * the channel EOF callback. Reply, if the protocol wants + * one, with wolfSSH_ChannelSendEof(); the library does + * not. + * WS_CHANNEL_CLOSED the peer closed a channel, which has been retired + * WS_WANT_READ / WS_WANT_WRITE / WS_REKEYING / WS_WINDOW_FULL + * transient; call again + * Anything else 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. + * + * For WS_CHAN_RXD, WS_EXTDATA, WS_EOF and WS_SUCCESS, channelId (when not + * NULL) names the channel the event belongs to. It is left alone for every + * other status, WS_CHANNEL_CLOSED included; use wolfSSH_GetLastRxId() there. + * + * Note that after a peer half-close wolfSSH_stream_send() keeps working: the + * library latches only the EOF it sends, not the one it receives. */ WOLFSSH_API int wolfSSH_worker(WOLFSSH* ssh, word32* channelId); WOLFSSH_API int wolfSSH_GetLastRxId(WOLFSSH* ssh, word32* channelId); From cb40b61ab28558ac13aee1eb54a72f1742694d57 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:03:43 -0700 Subject: [PATCH 03/17] Drain buffered data before the EOF wolfSSH_stream_read() and wolfSSH_stream_peek() hand back what the peer sent before they report its EOF. The eofRxd test now sits behind the buffered-data test, so a half-close no longer strands whatever arrived with it or just ahead of it. - stream_read() takes the head channel id before DoReceive() and keeps waiting when the EOF belongs to another channel: the head is still open and still has nothing buffered, so reporting it would be indistinguishable. Multi- channel callers see that one through the worker or the callback. - Looping only while the head is unchanged, since DoChannelClose() can free it and the buffer pointer lives in it. - The disconnect contract in ssh.h and internal.h drops the caveat that the EOF outranks the drain. - test_ChannelEofHalfClose() picks up the drain assertions, which the commit ahead of this one cannot satisfy. --- src/ssh.c | 36 +++-- tests/unit.c | 317 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 11 +- wolfssh/ssh.h | 6 +- 4 files changed, 353 insertions(+), 17 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index 6337cf7ba..60d4298f9 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1298,18 +1298,20 @@ int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz) ssh->error = WS_REKEYING; return WS_REKEYING; } - if (ssh->channelList->eofRxd) { - ssh->error = WS_EOF; - return WS_ERROR; - } inputBuffer = &ssh->channelList->inputBuffer; avail = inputBuffer->length - inputBuffer->idx; + /* Report the EOF only once the buffered data is drained. */ + if (avail == 0 && ssh->channelList->eofRxd) { + ssh->error = WS_EOF; + return WS_ERROR; + } + /* 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. */ + * neither. The EOF above outranks it: it names the channel. */ if (avail == 0 && ssh->disconnected) { ssh->error = WS_DISCONNECT; return WS_FATAL_ERROR; @@ -1341,6 +1343,7 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) { int ret = WS_SUCCESS; WOLFSSH_BUFFER* inputBuffer; + word32 headId; WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_read()"); @@ -1356,7 +1359,13 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) return WS_BAD_ARGUMENT; } - if (ssh->channelList->eofRxd) { + inputBuffer = &ssh->channelList->inputBuffer; + /* inputBuffer belongs to this channel; DoReceive() can retire it. */ + headId = ssh->channelList->channel; + + /* Report the EOF only once the buffered data is drained. */ + if (inputBuffer->length - inputBuffer->idx == 0 + && ssh->channelList->eofRxd) { ssh->error = WS_EOF; return WS_ERROR; } @@ -1368,7 +1377,6 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) return WS_FATAL_ERROR; } - inputBuffer = &ssh->channelList->inputBuffer; ssh->error = WS_SUCCESS; /* Hand back whatever arrived before the disconnect, then report it once @@ -1386,8 +1394,20 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) "Starting to receive data at current index of %u", inputBuffer->idx); ret = DoReceive(ssh); - if (ssh->channelList == NULL || ssh->channelList->eofRxd) + /* Off the current head: DoReceive() may have retired the old. */ + if (ssh->channelList == NULL + || (ssh->channelList->eofRxd + && ssh->channelList->inputBuffer.length + - ssh->channelList->inputBuffer.idx == 0)) ret = WS_EOF; + if (ret == WS_EOF && ssh->channelList != NULL + && ssh->channelList->channel == headId + && ssh->lastRxId != headId) { + /* Another channel's EOF is not this read's; the head is + * still open. Loop only while the head is unchanged, since + * inputBuffer points into it. */ + continue; + } if (ret == WS_EXTDATA && ssh->lastRxId != ssh->channelList->channel) { /* Extended data for another channel. wolfSSH_extended_data_read() diff --git a/tests/unit.c b/tests/unit.c index 3dbaa9279..240b883ef 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -4740,6 +4740,63 @@ static WS_MAYBE_UNUSED word32 BuildChannelEofPacket(byte* pkt, word32 channelId) return i; /* 16 */ } + +/* Builds a plaintext CHANNEL_CLOSE packet for channelId into pkt (needs 16 + * bytes), returning its size. + * + * Layout: [len=12][pad=6][msgid=97][chan][pad*6]. */ +static WS_MAYBE_UNUSED word32 BuildChannelClosePacket(byte* pkt, word32 channelId) +{ + word32 i = 0; + + /* packet_length = padLen(1) + msgid(1) + chan(4) + padding(6) = 12. */ + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x0C; + pkt[i++] = 0x06; /* padding length */ + pkt[i++] = MSGID_CHANNEL_CLOSE; + + pkt[i++] = (byte)((channelId >> 24) & 0xFF); + pkt[i++] = (byte)((channelId >> 16) & 0xFF); + pkt[i++] = (byte)((channelId >> 8) & 0xFF); + pkt[i++] = (byte)( channelId & 0xFF); + + WMEMSET(pkt + i, 0x00, 6); i += 6; /* padding bytes */ + + return i; /* 16 */ +} + +/* Builds a plaintext CHANNEL_OPEN_FAILURE for channelId into pkt (needs 32 + * bytes), returning its size. Empty description and language strings. + * + * Layout: [len=28][pad=10][msgid=92][chan][reason][0][0][pad*10]. */ +static WS_MAYBE_UNUSED word32 BuildChannelOpenFailPacket(byte* pkt, word32 channelId, + word32 reason) +{ + word32 i = 0; + + /* packet_length = padLen(1) + msgid(1) + chan(4) + reason(4) + * + description(4) + language(4) + padding(10) = 28. */ + pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x00; pkt[i++] = 0x1C; + pkt[i++] = 0x0A; /* padding length */ + pkt[i++] = MSGID_CHANNEL_OPEN_FAIL; + + pkt[i++] = (byte)((channelId >> 24) & 0xFF); + pkt[i++] = (byte)((channelId >> 16) & 0xFF); + pkt[i++] = (byte)((channelId >> 8) & 0xFF); + pkt[i++] = (byte)( channelId & 0xFF); + + pkt[i++] = (byte)((reason >> 24) & 0xFF); + pkt[i++] = (byte)((reason >> 16) & 0xFF); + pkt[i++] = (byte)((reason >> 8) & 0xFF); + pkt[i++] = (byte)( reason & 0xFF); + + WMEMSET(pkt + i, 0x00, 4); i += 4; /* description, empty */ + WMEMSET(pkt + i, 0x00, 4); i += 4; /* language, empty */ + + WMEMSET(pkt + i, 0x00, 10); i += 10; /* padding bytes */ + + return i; /* 32 */ +} + /* Fails every send, discarding whatever was bundled (WS_CBIO_ERR_GENERAL makes * wolfSSH_SendPacket() shrink the output buffer), so an adjust sent through it * never reaches the peer. */ @@ -6454,9 +6511,11 @@ static int test_ChannelEofHalfClose(void) WOLFSSH_CHANNEL* ch = NULL; int result = 0; int ret; + int i; word32 reportedId = 0xFFFFFFFF; word32 pktSz; byte pkt[48]; + byte out[32]; byte payload[4] = { 0x10, 0x11, 0x12, 0x13 }; ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); @@ -6500,6 +6559,24 @@ static int test_ChannelEofHalfClose(void) /* No auto-echo: our sending direction is untouched. */ if (ch->eofTxd) { result = -1479; goto done; } + /* The data that arrived ahead of the EOF is still deliverable. */ + ret = wolfSSH_stream_peek(ssh, out, (word32)sizeof(out)); + if (ret != 10) { result = -1480; goto done; } + + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret != 10) { result = -1481; goto done; } + for (i = 0; i < 10; i++) + if (out[i] != 0x44) { result = -1482; goto done; } + + /* Drained: the reader now sees the EOF. */ + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret != WS_ERROR) { result = -1483; goto done; } + if (wolfSSH_get_error(ssh) != WS_EOF) { result = -1484; goto done; } + + ret = wolfSSH_stream_peek(ssh, out, (word32)sizeof(out)); + if (ret != WS_ERROR) { result = -1485; goto done; } + if (wolfSSH_get_error(ssh) != WS_EOF) { result = -1486; goto done; } + /* Half-closed one way: we can still answer the peer. */ ret = wolfSSH_stream_send(ssh, payload, (word32)sizeof(payload)); if (ret != (int)sizeof(payload)) { result = -1487; goto done; } @@ -6512,6 +6589,229 @@ static int test_ChannelEofHalfClose(void) wolfSSH_CTX_free(ctx); return result; } +/* An EOF arriving on a channel that is not the head of the list is not the + * head's EOF. DoChannelEof() reports WS_EOF for whichever channel it lands on, + * so wolfSSH_stream_read() has to tell the two apart: the head is still open + * and still has nothing buffered, and WS_ERROR with WS_EOF latched is exactly + * what a real head EOF looks like. It keeps waiting instead. */ +static int test_StreamReadEofOtherChannel(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch0 = NULL; + WOLFSSH_CHANNEL* ch1 = NULL; + int result = 0; + int ret; + byte pkt[32]; + byte out[32]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1550; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1551; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch0 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch0 == NULL) { result = -1552; goto done; } + if (ChannelAppend(ssh, ch0) != WS_SUCCESS) { + ChannelDelete(ch0, ssh->ctx->heap); + result = -1553; + goto done; + } + + ch1 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch1 == NULL) { result = -1554; goto done; } + if (ChannelAppend(ssh, ch1) != WS_SUCCESS) { + ChannelDelete(ch1, ssh->ctx->heap); + result = -1555; + goto done; + } + + /* The first channel appended is the head that stream_read() drains. */ + if (ssh->channelList != ch0) { result = -1556; goto done; } + + /* Stage an EOF for the non-head channel only. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch1->channel); + s_recvPktOff = 0; + + /* The read consumes it, sees it is for another channel, and goes back for + * more. The mock has nothing left, so this ends as a want-read, not as + * the head channel's EOF. */ + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret >= 0) { result = -1557; goto done; } + if (wolfSSH_get_error(ssh) == WS_EOF) { result = -1558; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_READ) { result = -1559; goto done; } + + /* It landed where it belongs, and the head is untouched. */ + if (!ch1->eofRxd) { result = -1560; goto done; } + if (ch0->eofRxd) { result = -1561; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* DoReceive() can retire the head channel mid-read: DoChannelClose() frees it + * while wolfSSH_stream_read() still holds its inputBuffer. If the next head + * already has its EOF latched and drained, the EOF override fires while + * lastRxId names the channel that just went away, so the "keep waiting" path + * must not loop -- doing so spins forever on freed memory, and both messages + * that get there are peer-controlled. */ +static int test_StreamReadHeadRemoved(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch0 = NULL; + WOLFSSH_CHANNEL* ch1 = NULL; + int result = 0; + int ret; + byte pkt[16]; + byte out[32]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1590; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1591; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch0 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch0 == NULL) { result = -1592; goto done; } + if (ChannelAppend(ssh, ch0) != WS_SUCCESS) { + ChannelDelete(ch0, ssh->ctx->heap); + result = -1593; + goto done; + } + ch0->openConfirmed = 1; + ch0->peerWindowSz = 1024; + ch0->peerMaxPacketSz = 1024; + + ch1 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch1 == NULL) { result = -1594; goto done; } + if (ChannelAppend(ssh, ch1) != WS_SUCCESS) { + ChannelDelete(ch1, ssh->ctx->heap); + result = -1595; + goto done; + } + /* The other channel is already half-closed, with nothing buffered. */ + ch1->eofRxd = 1; + + /* The peer closes the channel the read is draining. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelClosePacket(pkt, ch0->channel); + s_recvPktOff = 0; + + /* Must return rather than spin. */ + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret >= 0) { result = -1596; goto done; } + + /* ch0 is retired; ch1 is untouched and still on the list. */ + if (ssh->channelListSz != 1) { result = -1597; goto done; } + if (ssh->channelList != ch1) { result = -1598; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + + +#ifndef NO_WOLFSSH_CLIENT +/* The same retirement through DoChannelOpenFail(), which is the case that + * needs the head test: it removes the channel without touching lastRxId, so + * lastRxId still names another channel and the "keep waiting" path would fire + * on a head that has just been freed. The staged packets arrive in one read: + * the EOF for the other channel sets lastRxId away from the head, then the + * open failure takes the head out from under the read in progress. */ +static int test_StreamReadHeadOpenFailed(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch0 = NULL; + WOLFSSH_CHANNEL* ch1 = NULL; + int result = 0; + int ret; + byte pkt[48]; + byte out[32]; + word32 pktSz; + word32 headId = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1696; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1697; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* The head, a channel of ours whose open the peer has not answered. */ + ch0 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch0 == NULL) { result = -1698; goto done; } + if (ChannelAppend(ssh, ch0) != WS_SUCCESS) { + ChannelDelete(ch0, ssh->ctx->heap); + result = -1699; + goto done; + } + + ch1 = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch1 == NULL) { result = -1700; goto done; } + if (ChannelAppend(ssh, ch1) != WS_SUCCESS) { + ChannelDelete(ch1, ssh->ctx->heap); + result = -1701; + goto done; + } + ch1->openConfirmed = 1; + ch1->peerWindowSz = 1024; + ch1->peerMaxPacketSz = 1024; + + /* ch0 is freed by the open failure, so keep what is needed from it. */ + headId = ch0->channel; + + pktSz = BuildChannelEofPacket(pkt, ch1->channel); + pktSz += BuildChannelOpenFailPacket(pkt + pktSz, headId, + OPEN_ADMINISTRATIVELY_PROHIBITED); + s_recvPkt = pkt; + s_recvPktSz = pktSz; + s_recvPktOff = 0; + + /* Must return rather than spin on the freed head. */ + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret >= 0) { result = -1702; goto done; } + + /* ch0 is gone; the other channel took the head and kept its EOF. */ + if (ssh->channelListSz != 1) { result = -1703; goto done; } + if (ssh->channelList != ch1) { result = -1704; goto done; } + if (!ch1->eofRxd) { result = -1705; goto done; } + if (ssh->lastRxId == headId) { result = -1706; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + + /* A peer may half-close its channel before it makes its shell/exec/subsystem * request, RFC 4254 section 5.3. DoChannelEof() reports that as WS_EOF, and * DoReceive() passes it through, but the accept loop must not read a negative @@ -18138,6 +18438,11 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_StreamReadEofOtherChannel(); + printf("StreamReadEofOtherChannel: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_AcceptSurvivesChannelEof(); printf("AcceptSurvivesChannelEof: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); @@ -18155,6 +18460,18 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_StreamReadHeadRemoved(); + printf("StreamReadHeadRemoved: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_StreamReadHeadOpenFailed(); + printf("StreamReadHeadOpenFailed: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + #endif /* NO_WOLFSSH_SERVER */ #ifndef NO_WOLFSSH_SERVER diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 589c73191..2627602e0 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1105,12 +1105,11 @@ struct WOLFSSH { /* 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, unless a CHANNEL_EOF arrived first. - * wolfSSH_worker() reports it as well, so a drive loop stops turning; - * wolfSSH_shutdown() drops the channel first and so never pumps it. - * DoPacket() skips the inbound dispatch too, for every message but a - * DISCONNECT, so nothing arriving afterward is buffered, answered or - * reported through the channel callbacks. */ + * buffer runs dry. wolfSSH_worker() reports it as well, so a drive loop + * stops turning; wolfSSH_shutdown() drops the channel first and so never + * pumps it. DoPacket() skips the inbound dispatch too, for every message + * but a DISCONNECT, so nothing arriving afterward is buffered, answered + * or reported through the channel callbacks. */ byte disconnected; /* Set once SendDisconnect() has bundled our own DISCONNECT into the * output buffer, and cleared once wolfSSH_SendPacket() drains it, so it diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 541a6d294..599545261 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -600,9 +600,9 @@ WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); * data is discarded and the channel callbacks stop firing. 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. A CHANNEL_EOF already received - * outranks that drain: both report WS_EOF with data possibly still - * buffered. RFC 4253 section 11.1. */ + * WS_DISCONNECT once their buffer runs dry. A CHANNEL_EOF received on that + * channel is drained the same way, and is the one reported when both are + * pending. RFC 4253 section 11.1. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); /* Returns the bytes read; the next read clears the status. WS_WANT_WRITE * from wolfSSH_get_error() means the adjust is queued; it goes out on the From 4211e84e42e482e5ad6e900eee825150bd65d55c Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:08:00 -0700 Subject: [PATCH 04/17] Add the channel half-close API wolfSSH_ChannelSendEof() and wolfSSH_stream_send_eof() close an application's sending direction and leave its receiving direction open, the half-close of RFC 4254 section 5.3. Reads keep working until the peer sends its own EOF or closes; data sends on the channel then report WS_EOF, while requests, the exit status and the teardown messages still go out. - Both refuse a channel whose open the peer has not confirmed: peerChannel is 0 until then and the send resolves by peer id, so the EOF would land on whichever channel holds peer id 0. - Both sit behind the disconnect gate and refuse to put a packet between KEXINIT and NEWKEYS. - stream_send_eof() reports WS_REKEYING itself rather than latching it, the way stream_peek() does; ssh.h says so, since stream_send() differs. --- src/ssh.c | 71 ++++++++++++ tests/unit.c | 313 ++++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 27 +++++ 3 files changed, 411 insertions(+) diff --git a/src/ssh.c b/src/ssh.c index 60d4298f9..3f423004a 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1569,6 +1569,43 @@ int wolfSSH_ChannelIdSendExt(WOLFSSH* ssh, word32 channelId, } +int wolfSSH_stream_send_eof(WOLFSSH* ssh) +{ + int ret = WS_SUCCESS; + + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_send_eof()"); + + 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; + + /* Only KEX traffic may go out mid-rekey, RFC 4253 section 7.1. */ + if (ret == WS_SUCCESS && ssh->isKeying) { + ssh->error = WS_REKEYING; + ret = WS_REKEYING; + } + + /* Same peer-id lookup as wolfSSH_ChannelSendEof(), so the same guard. */ + if (ret == WS_SUCCESS && !ssh->channelList->openConfirmed) { + WLOG(WS_LOG_DEBUG, "Channel not confirmed yet."); + ret = WS_CHANNEL_NOT_CONF; + } + + if (ret == WS_SUCCESS) + ret = SendChannelEof(ssh, ssh->channelList->peerChannel); + + WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_stream_send_eof(), ret = %d", ret); + return ret; +} + + int wolfSSH_stream_exit(WOLFSSH* ssh, int status) { int ret = WS_SUCCESS; @@ -4380,6 +4417,40 @@ int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel) } +int wolfSSH_ChannelSendEof(WOLFSSH_CHANNEL* channel) +{ + int ret = WS_SUCCESS; + + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_ChannelSendEof()"); + + /* Every gate below reaches through ssh, so take it up front. */ + if (channel == NULL || channel->ssh == NULL) + ret = WS_BAD_ARGUMENT; + + if (ret == WS_SUCCESS && SendAfterDisconnect(channel->ssh)) + ret = WS_FATAL_ERROR; + + /* Only KEX traffic may go out mid-rekey, RFC 4253 section 7.1. */ + if (ret == WS_SUCCESS && channel->ssh->isKeying) { + channel->ssh->error = WS_REKEYING; + ret = WS_REKEYING; + } + + /* peerChannel is 0 until the open is confirmed, and SendChannelEof() + * resolves by peer id. */ + if (ret == WS_SUCCESS && !channel->openConfirmed) { + WLOG(WS_LOG_DEBUG, "Channel not confirmed yet."); + ret = WS_CHANNEL_NOT_CONF; + } + + if (ret == WS_SUCCESS) + ret = SendChannelEof(channel->ssh, channel->peerChannel); + + WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_ChannelSendEof(), ret = %d", ret); + return ret; +} + + WOLFSSH_CHANNEL* wolfSSH_ChannelNext(WOLFSSH* ssh, WOLFSSH_CHANNEL* channel) { WOLFSSH_CHANNEL* nextChannel = NULL; diff --git a/tests/unit.c b/tests/unit.c index 240b883ef..ddd2dd953 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -4357,6 +4357,19 @@ static int DiscardIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) (void)ssh; (void)buf; (void)ctx; return (int)sz; } + +static int s_ioSendCalls = 0; + +/* Discards like DiscardIoSend, but counts the calls so a test can assert that + * nothing at all was handed to the transport. */ +static WS_MAYBE_UNUSED int CountingIoSend(WOLFSSH* ssh, void* buf, word32 sz, + void* ctx) +{ + (void)ssh; (void)buf; (void)ctx; + s_ioSendCalls++; + return (int)sz; +} + /* Reports a short (would-block) write, so an adjust bundled into the output * buffer reaches the transport but does not fully flush: wolfSSH_SendPacket() * maps WS_CBIO_ERR_WANT_WRITE to WS_WANT_WRITE. */ @@ -6988,6 +7001,12 @@ static int test_WorkerReportsEofChannelKeying(void) ret = wolfSSH_stream_peek(ssh, NULL, 1); if (ret != WS_REKEYING) { result = -1607; goto done; } + /* The new send-EOF API refuses to put a packet between KEXINIT and + * NEWKEYS. */ + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_REKEYING) { result = -1608; goto done; } + if (ch->eofTxd) { result = -1609; goto done; } + done: s_recvPkt = NULL; s_recvPktSz = 0; @@ -6998,6 +7017,219 @@ static int test_WorkerReportsEofChannelKeying(void) } #endif /* NO_WOLFSSH_SERVER */ + +#ifndef NO_WOLFSSH_CLIENT +/* wolfSSH_stream_send_eof() and wolfSSH_ChannelSendEof() close the + * application's own sending direction. Sends afterwards must fail, reads must + * not, and a second call must not put a second EOF on the wire. */ +static int test_SendEofApi(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte buf[4] = { 0x00, 0x01, 0x02, 0x03 }; + + if (wolfSSH_stream_send_eof(NULL) != WS_BAD_ARGUMENT) + return -1490; + if (wolfSSH_ChannelSendEof(NULL) != WS_BAD_ARGUMENT) + return -1491; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1492; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1493; goto done; } + /* Connection-protocol messages are only allowed once userauth is done. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* No channel yet: nothing to half-close. */ + if (wolfSSH_stream_send_eof(ssh) != WS_BAD_ARGUMENT) { + result = -1494; + goto done; + } + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -1495; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1496; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + ret = wolfSSH_stream_send(ssh, buf, (word32)sizeof(buf)); + if (ret != (int)sizeof(buf)) { result = -1497; goto done; } + + ret = wolfSSH_stream_send_eof(ssh); + if (ret != WS_SUCCESS) { result = -1498; goto done; } + if (!ch->eofTxd) { result = -1499; goto done; } + + /* Our direction is closed. */ + ret = wolfSSH_stream_send(ssh, buf, (word32)sizeof(buf)); + if (ret != WS_EOF) { result = -1500; goto done; } + + /* Idempotent: the second call must not reach the transport at all. + * DiscardIoSend flushes everything, so outputBuffer.length is 0 either + * way and could not tell a second EOF from none. */ + wolfSSH_SetIOSend(ctx, CountingIoSend); + s_ioSendCalls = 0; + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_SUCCESS) { result = -1501; goto done; } + if (s_ioSendCalls != 0) { result = -1502; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT +/* A disconnect, sent or received, ends the session, so neither send-EOF + * entry point may put a CHANNEL_EOF on the wire afterwards. RFC 4253 + * section 11.1. */ +static int test_SendEofAfterDisconnect(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1660; + wolfSSH_SetIOSend(ctx, CountingIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1661; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -1662; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1663; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + /* Our own disconnect is the last thing that may go out. */ + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + if (ret != WS_SUCCESS) { result = -1664; goto done; } + if (!ssh->disconnected) { result = -1665; goto done; } + + s_ioSendCalls = 0; + + ret = wolfSSH_stream_send_eof(ssh); + if (ret != WS_FATAL_ERROR) { result = -1666; goto done; } + if (wolfSSH_get_error(ssh) != WS_DISCONNECT) { result = -1667; goto done; } + + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_FATAL_ERROR) { result = -1668; goto done; } + if (wolfSSH_get_error(ssh) != WS_DISCONNECT) { result = -1669; goto done; } + + /* Nothing reached the transport and the channel is unmarked, so a + * later drain cannot mistake it for a half-close of ours. */ + if (s_ioSendCalls != 0) { result = -1670; goto done; } + if (ch->eofTxd) { result = -1671; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT +/* SendChannelEof() addresses the channel by peer id, and peerChannel is 0 + * until the peer confirms the open. wolfSSH_ChannelSendEof() on a channel + * still awaiting its confirmation must refuse rather than let the lookup land + * on whichever channel already holds peer id 0. */ +static int test_SendEofUnconfirmedChannel(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* sess = NULL; + WOLFSSH_CHANNEL* pend = NULL; + int result = 0; + int ret; + byte buf[4] = { 0x00, 0x01, 0x02, 0x03 }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1630; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1631; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* The live session channel, confirmed, holding peer id 0. */ + sess = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (sess == NULL) { result = -1632; goto done; } + if (ChannelAppend(ssh, sess) != WS_SUCCESS) { + ChannelDelete(sess, ssh->ctx->heap); + result = -1633; + goto done; + } + sess->openConfirmed = 1; + sess->peerChannel = 0; + sess->peerWindowSz = 1024; + sess->peerMaxPacketSz = 1024; + + /* A second channel whose open has only been sent, as + * wolfSSH_ChannelFwdNewLocal() hands back. */ + pend = ChannelNew(ssh, ID_CHANTYPE_TCPIP_DIRECT, 1024, 1024); + if (pend == NULL) { result = -1634; goto done; } + if (ChannelAppend(ssh, pend) != WS_SUCCESS) { + ChannelDelete(pend, ssh->ctx->heap); + result = -1635; + goto done; + } + if (pend->openConfirmed || pend->peerChannel != 0) { + result = -1636; + goto done; + } + + ret = wolfSSH_ChannelSendEof(pend); + if (ret != WS_CHANNEL_NOT_CONF) { result = -1637; goto done; } + + /* Neither channel was half-closed, and the session can still send. */ + if (sess->eofTxd || pend->eofTxd) { result = -1638; goto done; } + ret = wolfSSH_ChannelSend(sess, buf, (word32)sizeof(buf)); + if (ret != (int)sizeof(buf)) { result = -1639; goto done; } + + /* wolfSSH_stream_send_eof() runs the same lookup on the head, so it + * needs the same guard. An unconfirmed head resolves to whichever + * channel holds peer id 0, itself included, and latching eofTxd there + * would silence the real channel's send direction for good. */ + sess->openConfirmed = 0; + ret = wolfSSH_stream_send_eof(ssh); + if (ret != WS_CHANNEL_NOT_CONF) { result = -1650; goto done; } + if (sess->eofTxd || pend->eofTxd) { result = -1651; goto done; } + + sess->openConfirmed = 1; + ret = wolfSSH_stream_send_eof(ssh); + if (ret != WS_SUCCESS) { result = -1652; goto done; } + if (!sess->eofTxd) { result = -1653; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ #ifndef NO_WOLFSSH_SERVER static int s_eofCbCalls = 0; static word32 s_eofCbChannel = 0; @@ -7151,6 +7383,60 @@ static int test_WorkerReportsEofChannelId(void) return result; } #endif /* NO_WOLFSSH_SERVER */ + + +#ifndef NO_WOLFSSH_CLIENT +/* Only KEX traffic may go out mid-rekey, RFC 4253 section 7.1, and the head + * variant needs the same gate as wolfSSH_ChannelSendEof(). It reports the + * rekey itself rather than latching it behind WS_FATAL_ERROR, which is what + * the header promises. */ +static int test_StreamSendEofRekeying(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1730; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1731; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1732; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1733; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + ssh->isKeying = 1; + ret = wolfSSH_stream_send_eof(ssh); + if (ret != WS_REKEYING) { result = -1734; goto done; } + if (wolfSSH_get_error(ssh) != WS_REKEYING) { result = -1735; goto done; } + if (ch->eofTxd) { result = -1736; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1737; goto done; } + + /* And it goes out once the rekey is done. */ + ssh->isKeying = 0; + ret = wolfSSH_stream_send_eof(ssh); + if (ret != WS_SUCCESS) { result = -1738; goto done; } + if (!ch->eofTxd) { result = -1739; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ static int test_SendChannelData_eofTxd(void) { WOLFSSH_CTX* ctx = NULL; @@ -18474,6 +18760,26 @@ int wolfSSH_UnitTest(int argc, char** argv) #endif /* NO_WOLFSSH_SERVER */ +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SendEofApi(); + printf("SendEofApi: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SendEofUnconfirmedChannel(); + printf("SendEofUnconfirmedChannel: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SendEofAfterDisconnect(); + printf("SendEofAfterDisconnect: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + #ifndef NO_WOLFSSH_SERVER unitResult = test_ChannelEofCallback(); printf("ChannelEofCallback: %s\n", @@ -18488,6 +18794,13 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif /* NO_WOLFSSH_SERVER */ +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_StreamSendEofRekeying(); + printf("StreamSendEofRekeying: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + unitResult = test_SendChannelData_eofTxd(); printf("SendChannelData_eofTxd: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 599545261..eb714dc7b 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -324,6 +324,28 @@ WOLFSSH_API int wolfSSH_ChannelReadExt(WOLFSSH_CHANNEL* channel, byte* buf, WOLFSSH_API int wolfSSH_ChannelSendExt(WOLFSSH_CHANNEL* channel, const byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel); +/* Sends SSH_MSG_CHANNEL_EOF, closing the sending direction and leaving the + * receiving direction open (the half-close of RFC 4254 section 5.3). Data + * sends on the channel then fail with WS_EOF -- wolfSSH_ChannelSend(), + * wolfSSH_stream_send() and the extended-data variants; requests, the exit + * status and the teardown messages still go out. Reads work until the peer + * sends its own EOF or closes. Idempotent: a second call puts no second EOF + * on the wire. + * + * The library never answers a received EOF with one of its own. It reports it + * as WS_EOF and through the channel EOF callback, and the application decides + * whether to reply, with this call or wolfSSH_stream_send_eof(). A + * back-pressure status can supersede the WS_EOF from wolfSSH_worker(); + * wolfSSH_ChannelGetEof() is the durable check. + * wolfSSH_ChannelExit() and wolfSSH_shutdown() send an EOF themselves while + * tearing the channel down. + * + * Returns WS_SUCCESS, WS_BAD_ARGUMENT on a NULL channel, + * WS_CHANNEL_NOT_CONF if the peer has not confirmed the channel open yet, + * WS_REKEYING during a key exchange, WS_FATAL_ERROR with WS_DISCONNECT + * latched once the session is over, or a send-path status such as + * WS_WANT_WRITE. */ +WOLFSSH_API int wolfSSH_ChannelSendEof(WOLFSSH_CHANNEL* channel); WOLFSSH_API int wolfSSH_ChannelGetEof(WOLFSSH_CHANNEL* channel); WOLFSSH_API const char* wolfSSH_ChannelGetType( const WOLFSSH_CHANNEL* channel); @@ -609,6 +631,11 @@ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); * next send or a wolfSSH_worker() whose receive succeeded. Others failed. */ WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); +/* Half-closes the first channel in the list. See wolfSSH_ChannelSendEof(). + * Unlike wolfSSH_stream_send(), which returns WS_FATAL_ERROR with the cause + * latched, this reports WS_REKEYING itself, the way wolfSSH_stream_peek() + * does. */ +WOLFSSH_API int wolfSSH_stream_send_eof(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_stream_exit(WOLFSSH* ssh, int status); WOLFSSH_API int wolfSSH_extended_data_send(WOLFSSH* ssh, byte* buf, word32 bufSz); /* Reads the buffered stderr of the first channel in the channel list into out; From 9c4a9ccefe26aad499751806fd10ed7c52777ba9 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:08:00 -0700 Subject: [PATCH 05/17] Latch eofTxd on the bundled EOF SendChannelEof() commits eofTxd once the EOF is in the output buffer, not only once the flush reports success. A short write leaves the bytes queued and they go out on the next flush, so the retry an application makes on WS_WANT_WRITE must not put a second EOF behind the first. - The exception is the send failure that discards the output buffer, taking the EOF with it: latching there would leave the channel refusing every later send for an EOF that never went anywhere. - A reset or a closed peer keeps the bytes, so they still count. The test is what the buffer holds, since wolfSSH_SendPacket() reports all three the same way. - Both arms have a test: the discard through FailIoSend, and the reset that keeps the bytes queued and latches. Issue: F-8826 --- src/internal.c | 13 +++- tests/unit.c | 193 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index 91a826f24..fbd78f5e5 100644 --- a/src/internal.c +++ b/src/internal.c @@ -20271,11 +20271,16 @@ int SendChannelEof(WOLFSSH* ssh, word32 peerChannelId) ret = BundlePacket(ssh); } - if (ret == WS_SUCCESS) + if (ret == WS_SUCCESS) { ret = wolfSSH_SendPacket(ssh); - - if (ret == WS_SUCCESS) - channel->eofTxd = 1; + /* Committed once bundled, so a retry queues no second EOF. Unless + * the send discarded the buffer and took the EOF with it, which only + * an emptied buffer tells us apart -- and that reading is knowingly + * imprecise: a flush that sent the EOF and then failed on a later + * packet empties the buffer too, and leaves this clear. */ + if (ret != WS_SOCKET_ERROR_E || wolfSSH_OutputPending(ssh)) + channel->eofTxd = 1; + } WLOG(WS_LOG_DEBUG, "Leaving SendChannelEof(), ret = %d", ret); return ret; diff --git a/tests/unit.c b/tests/unit.c index ddd2dd953..a3f29c53e 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -7224,12 +7224,184 @@ static int test_SendEofUnconfirmedChannel(void) if (ret != WS_SUCCESS) { result = -1652; goto done; } if (!sess->eofTxd) { result = -1653; goto done; } +#ifndef NO_WOLFSSH_CLIENT done: wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); return result; } #endif /* NO_WOLFSSH_CLIENT */ +/* A bundled but unflushed EOF is committed: the bytes are in the output + * buffer and go out on the next flush. The WS_WANT_WRITE retry an application + * makes must not queue a second EOF behind the first. */ +static int test_SendChannelEofWantWrite(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 queued; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1510; + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1511; goto done; } + /* Connection-protocol messages are only allowed once userauth is done. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -1512; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1513; + goto done; + } + ch->openConfirmed = 1; + + /* The socket will not take it, but the packet is built and buffered. */ + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_WANT_WRITE) { result = -1514; goto done; } + if (!ch->eofTxd) { result = -1515; goto done; } + queued = ssh->outputBuffer.length; + if (queued == 0) { result = -1516; goto done; } + + /* The retry an application makes on WS_WANT_WRITE. */ + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_SUCCESS) { result = -1517; goto done; } + if (ssh->outputBuffer.length != queued) { result = -1518; goto done; } + + /* One EOF goes out, and the buffer empties. */ + wolfSSH_SetIOSend(ctx, DiscardIoSend); + ret = wolfSSH_SendPacket(ssh); + if (ret != WS_SUCCESS) { result = -1519; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1520; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + + +#ifndef NO_WOLFSSH_CLIENT +/* A hard send failure is the other side of that latch. WS_CBIO_ERR_GENERAL + * makes wolfSSH_SendPacket() discard the output buffer, so the bundled EOF is + * gone; latching eofTxd there would leave the channel refusing every later + * send for an EOF that never went anywhere. */ +static int test_SendChannelEofSendFails(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1687; + wolfSSH_SetIOSend(ctx, FailIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1688; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -1689; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1690; + goto done; + } + ch->openConfirmed = 1; + + /* The send fails and takes the bundled EOF with it. */ + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_SOCKET_ERROR_E) { result = -1691; goto done; } + if (ch->eofTxd) { result = -1692; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1693; goto done; } + + /* So the channel is not half-closed, and an EOF can still be built and + * sent once the socket takes bytes again. */ + wolfSSH_SetIOSend(ctx, DiscardIoSend); + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_SUCCESS) { result = -1694; goto done; } + if (!ch->eofTxd) { result = -1695; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + + +#ifndef NO_WOLFSSH_CLIENT +/* A connection reset fails the send without discarding the buffer, so the EOF + * is still queued and eofTxd has to latch: the retry flushes those bytes and + * must not build a second EOF behind them. The other arm of the same test. */ +static int ConnResetIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + (void)ssh; (void)buf; (void)sz; (void)ctx; + return WS_CBIO_ERR_CONN_RST; +} + +static int test_SendChannelEofConnReset(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 queued; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1750; + wolfSSH_SetIOSend(ctx, ConnResetIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1751; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -1752; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1753; + goto done; + } + ch->openConfirmed = 1; + + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_SOCKET_ERROR_E) { result = -1754; goto done; } + if (!wolfSSH_OutputPending(ssh)) { result = -1755; goto done; } + if (!ch->eofTxd) { result = -1756; goto done; } + + /* The queued bytes are the EOF; a second call adds nothing to them. */ + queued = ssh->outputBuffer.length - ssh->outputBuffer.idx; + ret = wolfSSH_ChannelSendEof(ch); + if (ret != WS_SUCCESS) { result = -1757; goto done; } + if (ssh->outputBuffer.length - ssh->outputBuffer.idx != queued) { + result = -1758; + goto done; + } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + + #ifndef NO_WOLFSSH_SERVER static int s_eofCbCalls = 0; static word32 s_eofCbChannel = 0; @@ -18780,6 +18952,27 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif /* NO_WOLFSSH_CLIENT */ +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SendChannelEofWantWrite(); + printf("SendChannelEofWantWrite: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SendChannelEofSendFails(); + printf("SendChannelEofSendFails: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SendChannelEofConnReset(); + printf("SendChannelEofConnReset: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + #ifndef NO_WOLFSSH_SERVER unitResult = test_ChannelEofCallback(); printf("ChannelEofCallback: %s\n", From 3a884fefeeea9ac190500d714f80e84fb2f7180d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:08:01 -0700 Subject: [PATCH 06/17] Drop closeTxd on a discarded close SendChannelClose() latches closeTxd on the same terms as the EOF beside it: on anything but the send failure that discards the output buffer. It latched unconditionally, so a closeTxd claiming a close that never left made wolfSSH_shutdown()'s gate skip the teardown altogether. - Split from the EOF latch on purpose: that one widens, from success-only to bundled, and this one narrows. Read as one change they read wrong. --- src/internal.c | 5 ++++- tests/unit.c | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index fbd78f5e5..a9a1eeb04 100644 --- a/src/internal.c +++ b/src/internal.c @@ -20438,7 +20438,10 @@ int SendChannelClose(WOLFSSH* ssh, word32 peerChannelId) if (ret == WS_SUCCESS) { ret = wolfSSH_SendPacket(ssh); - channel->closeTxd = 1; + /* Same terms as SendChannelEof(). A closeTxd for a close that never + * left makes wolfSSH_shutdown() skip the teardown. */ + if (ret != WS_SOCKET_ERROR_E || wolfSSH_OutputPending(ssh)) + channel->closeTxd = 1; } WLOG(WS_LOG_DEBUG, "Leaving SendChannelClose(), ret = %d", ret); diff --git a/tests/unit.c b/tests/unit.c index a3f29c53e..1d1b09cf8 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -7327,6 +7327,12 @@ static int test_SendChannelEofSendFails(void) if (ch->eofTxd) { result = -1692; goto done; } if (ssh->outputBuffer.length != 0) { result = -1693; goto done; } + /* SendChannelClose() has the same shape, and a closeTxd claiming a close + * that never left makes wolfSSH_shutdown() skip the teardown. */ + ret = SendChannelClose(ssh, ch->peerChannel); + if (ret != WS_SOCKET_ERROR_E) { result = -1762; goto done; } + if (ch->closeTxd) { result = -1763; goto done; } + /* So the channel is not half-closed, and an EOF can still be built and * sent once the socket takes bytes again. */ wolfSSH_SetIOSend(ctx, DiscardIoSend); From 81b7f206a96dcfa91034fbfd24cd429db97f9d74 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:08:01 -0700 Subject: [PATCH 07/17] Keep the channel until the peer's close wolfSSH_ChannelExit() leaves the channel on the list once it has sent the EOF and the close, so the application's pointer stays valid until the peer answers and wolfSSH_worker() reports WS_CHANNEL_CLOSED. Removing it locally freed the pointer under the caller and left the peer's close matching nothing. - DoChannelClose() sends the EOF ahead of the close, per RFC 4254 section 5.3, and sends both whatever the flush reports: DoPacket() consumes the peer's close either way, so a message skipped over a blocked flush is never sent. - It retires the channel and names it on a short write too. The debt belongs to the output buffer, not the channel, and withholding the close signal would leave the caller timing the teardown out. - The worker flushes that reply, keeps WS_CHANNEL_CLOSED as the return value, and leaves WS_WANT_WRITE latched so the caller knows to drain wolfSSH_OutputPending() before closing the socket. ssh.h says so. Issue: F-8839 --- src/internal.c | 25 ++- src/ssh.c | 14 +- tests/unit.c | 462 +++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 9 + 4 files changed, 500 insertions(+), 10 deletions(-) diff --git a/src/internal.c b/src/internal.c index a9a1eeb04..4e63d4d5a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11390,17 +11390,30 @@ static int DoChannelClose(WOLFSSH* ssh, if (ret == WS_SUCCESS) { if (!channel->closeTxd) { + /* EOF ahead of the close, RFC 4254 section 5.3. Both go + * unconditionally: DoPacket() consumes the peer's close whatever + * this returns, so nothing runs it again. */ + int eofRet = SendChannelEof(ssh, channel->peerChannel); + ret = SendChannelClose(ssh, channel->peerChannel); + if (ret == WS_SUCCESS) + ret = eofRet; } } - if (ret == WS_SUCCESS) { - ret = ChannelRemove(ssh, channelId, WS_CHANNEL_ID_SELF); - } + /* Retire it once the close is bundled: a blocked flush belongs to the + * output buffer, not the channel. */ + if (ret == WS_SUCCESS || ret == WS_WANT_WRITE) { + int removeRet = ChannelRemove(ssh, channelId, WS_CHANNEL_ID_SELF); - if (ret == WS_SUCCESS) { - ret = WS_CHANNEL_CLOSED; - ssh->lastRxId = channelId; + if (removeRet != WS_SUCCESS) + ret = removeRet; + else { + /* Report the close even on a short flush; the caller needs the + * graceful-close signal. */ + ret = WS_CHANNEL_CLOSED; + ssh->lastRxId = channelId; + } } WLOG(WS_LOG_DEBUG, "Leaving DoChannelClose(), ret = %d", ret); diff --git a/src/ssh.c b/src/ssh.c index 3f423004a..e3ff96762 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3732,6 +3732,16 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) } #endif /* WOLFSSH_TEST_BLOCK */ + /* DoChannelClose() bundles the reply inside DoReceive(), and callers + * treat the close as terminal, so flush it here. The close stays the + * return value; a short flush leaves WS_WANT_WRITE latched. */ + if (ret == WS_CHANNEL_CLOSED && ssh->outputBuffer.length != 0) { + int closeErr = ssh->error; + + if (wolfSSH_SendPacket(ssh) == WS_SUCCESS) + ssh->error = closeErr; + } + /* WS_EXTDATA and WS_EOF report the channel too, so a multi-channel caller * can route the drain, or see which channel half-closed. */ if (ret == WS_SUCCESS || ret == WS_CHAN_RXD || ret == WS_EXTDATA @@ -4408,10 +4418,6 @@ int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel) if (ret == WS_SUCCESS) ret = SendChannelClose(channel->ssh, channel->peerChannel); - if (ret == WS_SUCCESS) - ret = ChannelRemove(channel->ssh, - channel->peerChannel, WS_CHANNEL_ID_PEER); - WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_ChannelExit(), ret = %d", ret); return ret; } diff --git a/tests/unit.c b/tests/unit.c index 1d1b09cf8..1162f28e1 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -4818,6 +4818,33 @@ static WS_MAYBE_UNUSED int FailIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* (void)ssh; (void)buf; (void)sz; (void)ctx; return WS_CBIO_ERR_GENERAL; } + +/* Walks a run of plaintext SSH packets and reports whether any of them carries + * msgId. A bare session negotiates no cipher, so queued and sent packets are + * both in the clear. */ +static WS_MAYBE_UNUSED int PlainPacketsHaveMsg(const byte* buf, word32 begin, word32 end, + byte msgId) +{ + word32 i; + + for (i = begin; i + LENGTH_SZ + PAD_LENGTH_SZ + MSG_ID_SZ <= end; ) { + word32 packetSz; + + packetSz = ((word32)buf[i] << 24) + | ((word32)buf[i + 1] << 16) + | ((word32)buf[i + 2] << 8) + | (word32)buf[i + 3]; + if (packetSz == 0 || i + LENGTH_SZ + packetSz > end) + break; + if (buf[i + LENGTH_SZ + 1] == msgId) + return 1; + i += LENGTH_SZ + packetSz; + } + + return 0; +} + + #ifndef NO_WOLFSSH_SERVER /* An unknown extended data type must be ignored (consumed and discarded) per @@ -6602,6 +6629,74 @@ static int test_ChannelEofHalfClose(void) wolfSSH_CTX_free(ctx); return result; } + +/* wolfSSH_ChannelExit() sends EOF and close, then leaves the channel on the + * list for the peer's own close to retire. Removing it there would free the + * caller's channel pointer and leave the peer's CHANNEL_CLOSE matching nothing, + * which DoReceive() turns into a fatal error on an ordinary shutdown. */ +static int test_ChannelExitKeepsChannel(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 pktSz; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1530; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1531; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1532; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1533; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + ret = wolfSSH_ChannelExit(ch); + if (ret != WS_SUCCESS) { result = -1534; goto done; } + if (!ch->eofTxd) { result = -1535; goto done; } + if (!ch->closeTxd) { result = -1536; goto done; } + + /* Still on the list, so the pointer the caller passed is still good. */ + if (ssh->channelList != ch) { result = -1537; goto done; } + if (ssh->channelListSz != 1) { result = -1538; goto done; } + + /* The peer answers with its own close. */ + pktSz = BuildChannelClosePacket(pkt, ch->channel); + s_recvPkt = pkt; + s_recvPktSz = pktSz; + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_CHANNEL_CLOSED) { result = -1539; goto done; } + + /* Retired now, and by the peer's close rather than by the exit. */ + if (ssh->channelList != NULL) { result = -1540; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + + + /* An EOF arriving on a channel that is not the head of the list is not the * head's EOF. DoChannelEof() reports WS_EOF for whichever channel it lands on, * so wolfSSH_stream_read() has to tell the two apart: the head is still open @@ -6673,6 +6768,195 @@ static int test_StreamReadEofOtherChannel(void) return result; } + + +static byte s_sentBuf[512]; +static word32 s_sentSz = 0; +static int s_sendRefusals = 0; + +/* Refuses the first s_sendRefusals writes with a would-block, then takes + * everything and keeps a copy of what reached the transport. */ +static int RefuseThenCaptureIoSend(WOLFSSH* ssh, void* buf, word32 sz, + void* ctx) +{ + WOLFSSH_UNUSED(ssh); + WOLFSSH_UNUSED(ctx); + + if (s_sendRefusals > 0) { + s_sendRefusals--; + return WS_CBIO_ERR_WANT_WRITE; + } + if (s_sentSz + sz > (word32)sizeof(s_sentBuf)) + return WS_CBIO_ERR_GENERAL; + WMEMCPY(s_sentBuf + s_sentSz, buf, sz); + s_sentSz += sz; + return (int)sz; +} + + +/* DoPacket() consumes the peer's CHANNEL_CLOSE whatever DoChannelClose() + * returns, so the reply gets one chance to be built. A blocked socket must not + * cost it: the EOF and the close both have to be bundled, and the channel + * retired, with the flush left owed to the caller. */ +static int test_DoChannelCloseWantWrite(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + int sawEof = 0; + int sawClose = 0; + word32 chanId; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1570; + /* The socket takes nothing, so everything bundled stays in the buffer. */ + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1571; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1572; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1573; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + chanId = ch->channel; + + /* The peer closes first. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelClosePacket(pkt, ch->channel); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, NULL); + /* The channel is closed and named even though the flush is owed: a + * back-pressured socket is not a reason to withhold the close signal. */ + if (ret != WS_CHANNEL_CLOSED) { result = -1574; goto done; } + if (ssh->lastRxId != chanId) { result = -1575; goto done; } + + /* The channel is retired, so ch is freed and must not be read again. */ + if (ssh->channelList != NULL) { result = -1576; goto done; } + if (ssh->channelListSz != 0) { result = -1579; goto done; } + + /* Both packets are committed, and nothing will revisit the peer's close + * to build them later. Walk the queue and confirm both are there. */ + sawEof = PlainPacketsHaveMsg(ssh->outputBuffer.buffer, + ssh->outputBuffer.idx, ssh->outputBuffer.length, + MSGID_CHANNEL_EOF); + sawClose = PlainPacketsHaveMsg(ssh->outputBuffer.buffer, + ssh->outputBuffer.idx, ssh->outputBuffer.length, + MSGID_CHANNEL_CLOSE); + if (!sawEof) { result = -1577; goto done; } + if (!sawClose) { result = -1578; goto done; } + + /* And the owed flush is visible: the close is the return value, so + * WS_WANT_WRITE has to reach the caller some other way or the reply sits + * in the buffer while the caller closes the socket. */ + if (wolfSSH_get_error(ssh) != WS_WANT_WRITE) { result = -1764; goto done; } + if (!wolfSSH_OutputPending(ssh)) { result = -1765; goto done; } + + /* And they really do go out once the socket takes bytes again. */ + wolfSSH_SetIOSend(ctx, DiscardIoSend); + ret = wolfSSH_SendPacket(ssh); + if (ret != WS_SUCCESS) { result = -1580; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1581; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + + +/* The reply to the peer's close still has to reach the wire when the socket + * was full while it was bundled. DoChannelClose() reports WS_CHANNEL_CLOSED + * whatever the flush does and retires the channel, and every caller treats + * that as terminal, so wolfSSH_worker() owes the flush itself. */ +static int test_DoChannelCloseFlushesReply(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1610; + /* The socket refuses both sends DoChannelClose() makes -- the EOF's and + * the close's -- and takes bytes again by the time the worker flushes. */ + s_sendRefusals = 2; + s_sentSz = 0; + wolfSSH_SetIOSend(ctx, RefuseThenCaptureIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1611; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1612; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1613; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + /* The peer closes first. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelClosePacket(pkt, ch->channel); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, NULL); + /* The flush is a courtesy, not the caller's business: it neither replaces + * the close status nor disturbs the latched error. */ + if (ret != WS_CHANNEL_CLOSED) { result = -1614; goto done; } + if (wolfSSH_get_error(ssh) != WS_CHANNEL_CLOSED) { + result = -1615; + goto done; + } + + /* Nothing is left owed, and both packets really went out. */ + if (ssh->outputBuffer.length != 0) { result = -1616; goto done; } + if (!PlainPacketsHaveMsg(s_sentBuf, 0, s_sentSz, MSGID_CHANNEL_EOF)) { + result = -1617; + goto done; + } + if (!PlainPacketsHaveMsg(s_sentBuf, 0, s_sentSz, MSGID_CHANNEL_CLOSE)) { + result = -1618; + goto done; + } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + s_sendRefusals = 0; + s_sentSz = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + + + /* DoReceive() can retire the head channel mid-read: DoChannelClose() frees it * while wolfSSH_stream_read() still holds its inputBuffer. If the next head * already has its EOF latched and drained, the EOF override fires while @@ -7615,6 +7899,155 @@ static int test_StreamSendEofRekeying(void) return result; } #endif /* NO_WOLFSSH_CLIENT */ + + +#ifndef NO_WOLFSSH_CLIENT +/* wolfSSH_shutdown() drains for the peer's answer to its close. A peer that + * half-closes first answers with an EOF, which is an ordinary part of the + * teardown and not a failure of it. */ +static int test_ShutdownAbsorbsEof(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte pkt[16]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1740; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1741; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1742; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1743; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + /* All the peer sends back is its own half-close. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelEofPacket(pkt, ch->channel); + s_recvPktOff = 0; + + ret = wolfSSH_shutdown(ssh); + if (ret != WS_SUCCESS) { result = -1744; goto done; } + + /* The teardown went out and the channel is still there, waiting for the + * peer's close. */ + if (!ch->eofTxd || !ch->closeTxd) { result = -1745; goto done; } + if (!ch->eofRxd) { result = -1746; goto done; } + if (ssh->channelList != ch) { result = -1747; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + + +#ifndef NO_WOLFSSH_CLIENT +/* wolfSSH_ChannelExit() chains its close on the EOF's send, so a socket that + * will not take the EOF leaves the close unbuilt and the teardown half done. + * The call has to be repeated, and the repeat must finish it rather than put + * a second EOF on the wire. */ +static int test_ChannelExitWantWrite(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 queued; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1748; + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1749; goto done; } + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1750; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1751; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + /* The EOF is bundled but not away, so the close was never built. */ + ret = wolfSSH_ChannelExit(ch); + if (ret != WS_WANT_WRITE) { result = -1752; goto done; } + if (!ch->eofTxd) { result = -1753; goto done; } + if (ch->closeTxd) { result = -1754; goto done; } + queued = ssh->outputBuffer.length; + if (queued == 0) { result = -1755; goto done; } + if (!PlainPacketsHaveMsg(ssh->outputBuffer.buffer, ssh->outputBuffer.idx, + ssh->outputBuffer.length, MSGID_CHANNEL_EOF)) { + result = -1756; + goto done; + } + if (PlainPacketsHaveMsg(ssh->outputBuffer.buffer, ssh->outputBuffer.idx, + ssh->outputBuffer.length, MSGID_CHANNEL_CLOSE)) { + result = -1757; + goto done; + } + + /* The repeat builds the close, still against a blocked socket. */ + ret = wolfSSH_ChannelExit(ch); + if (ret != WS_WANT_WRITE) { result = -1766; goto done; } + if (!ch->closeTxd) { result = -1767; goto done; } + + /* A third call has nothing left to build, so it reports success -- with + * both messages still sitting in the buffer. WS_SUCCESS from this call + * means bundled, not delivered, which is why the header tells callers to + * keep driving wolfSSH_worker() before dropping the socket. */ + ret = wolfSSH_ChannelExit(ch); + if (ret != WS_SUCCESS) { result = -1768; goto done; } + if (!wolfSSH_OutputPending(ssh)) { result = -1769; goto done; } + + /* With both flags latched the call builds nothing and sends nothing -- + * it does not even reach the transport, so a working socket does not + * drain what is queued. The caller owns that. */ + wolfSSH_SetIOSend(ctx, DiscardIoSend); + ret = wolfSSH_ChannelExit(ch); + if (ret != WS_SUCCESS) { result = -1758; goto done; } + if (!ch->closeTxd) { result = -1759; goto done; } + if (!wolfSSH_OutputPending(ssh)) { result = -1778; goto done; } + + /* Driven by hand, both messages go out and no duplicate follows. */ + if (wolfSSH_SendPacket(ssh) != WS_SUCCESS) { result = -1779; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1760; goto done; } + + /* And the channel is still there for the peer's close to name. */ + if (ssh->channelList != ch) { result = -1761; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_CLIENT */ + static int test_SendChannelData_eofTxd(void) { WOLFSSH_CTX* ctx = NULL; @@ -18907,6 +19340,16 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_DoChannelCloseWantWrite(); + printf("DoChannelCloseWantWrite: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DoChannelCloseFlushesReply(); + printf("DoChannelCloseFlushesReply: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_AcceptSurvivesChannelEof(); printf("AcceptSurvivesChannelEof: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); @@ -18936,6 +19379,11 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif /* NO_WOLFSSH_CLIENT */ + unitResult = test_ChannelExitKeepsChannel(); + printf("ChannelExitKeepsChannel: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + #endif /* NO_WOLFSSH_SERVER */ #ifndef NO_WOLFSSH_CLIENT @@ -19000,6 +19448,20 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif /* NO_WOLFSSH_CLIENT */ +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_ShutdownAbsorbsEof(); + printf("ShutdownAbsorbsEof: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_ChannelExitWantWrite(); + printf("ChannelExitWantWrite: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_CLIENT */ + unitResult = test_SendChannelData_eofTxd(); printf("SendChannelData_eofTxd: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index eb714dc7b..4577b447a 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -323,6 +323,15 @@ WOLFSSH_API int wolfSSH_ChannelReadExt(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_ChannelSendExt(WOLFSSH_CHANNEL* channel, const byte* buf, word32 bufSz); +/* Sends EOF then SSH_MSG_CHANNEL_CLOSE. The channel stays on the list, and the + * pointer stays valid, until the peer's close arrives and wolfSSH_worker() + * reports WS_CHANNEL_CLOSED. A walk with wolfSSH_ChannelNext() has to step + * past a channel it has exited rather than re-read the head, which no longer + * moves. + * + * A WS_WANT_WRITE means the teardown is incomplete: the close is only built + * once the EOF is away, so call again until it reports something else. The + * retry costs nothing, a bundled EOF is not sent twice. */ WOLFSSH_API int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel); /* Sends SSH_MSG_CHANNEL_EOF, closing the sending direction and leaving the * receiving direction open (the half-close of RFC 4254 section 5.3). Data From a3b1c4b49c19c5ec557bea4e32f1a52f0b32595f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:08:42 -0700 Subject: [PATCH 08/17] Reject teardown on an unconfirmed channel DoChannelClose() and wolfSSH_ChannelExit() answer only a channel whose open the peer has confirmed. peerChannel is 0 until then and both senders resolve by peer id, so a teardown aimed at an unconfirmed channel landed on whichever channel held peer id 0 -- normally the live session -- latching its eofTxd and killing its send direction. - The close reply still retires the channel it names; there is simply nothing to say to a peer that has not answered the open. - wolfSSH_ChannelExit() reports WS_CHANNEL_NOT_CONF, the same as the two send-EOF calls. --- src/internal.c | 4 +- src/ssh.c | 7 +++ tests/unit.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++- wolfssh/ssh.h | 9 +++- 4 files changed, 133 insertions(+), 3 deletions(-) diff --git a/src/internal.c b/src/internal.c index 4e63d4d5a..f37ecf561 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11388,8 +11388,10 @@ static int DoChannelClose(WOLFSSH* ssh, } } + /* An unconfirmed channel has peerChannel 0, so a reply by peer id would + * land on whichever channel holds 0 -- normally the live session. */ if (ret == WS_SUCCESS) { - if (!channel->closeTxd) { + if (!channel->closeTxd && channel->openConfirmed) { /* EOF ahead of the close, RFC 4254 section 5.3. Both go * unconditionally: DoPacket() consumes the peer's close whatever * this returns, so nothing runs it again. */ diff --git a/src/ssh.c b/src/ssh.c index e3ff96762..9388c8d86 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -4412,6 +4412,13 @@ int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel) SendAfterDisconnect(channel->ssh)) ret = WS_FATAL_ERROR; + /* Both sends address the peer id, 0 until the open is confirmed, so + * this would tear down whichever channel holds 0. */ + if (ret == WS_SUCCESS && !channel->openConfirmed) { + WLOG(WS_LOG_DEBUG, "Channel not confirmed yet."); + ret = WS_CHANNEL_NOT_CONF; + } + if (ret == WS_SUCCESS) ret = SendChannelEof(channel->ssh, channel->peerChannel); diff --git a/tests/unit.c b/tests/unit.c index 1162f28e1..8a779d561 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -7489,6 +7489,12 @@ static int test_SendEofUnconfirmedChannel(void) ret = wolfSSH_ChannelSendEof(pend); if (ret != WS_CHANNEL_NOT_CONF) { result = -1637; goto done; } + /* wolfSSH_ChannelExit() sends an EOF through the same lookup, ahead of its + * close, so it needs the same guard. */ + ret = wolfSSH_ChannelExit(pend); + if (ret != WS_CHANNEL_NOT_CONF) { result = -1654; goto done; } + if (sess->closeTxd || pend->closeTxd) { result = -1655; goto done; } + /* Neither channel was half-closed, and the session can still send. */ if (sess->eofTxd || pend->eofTxd) { result = -1638; goto done; } ret = wolfSSH_ChannelSend(sess, buf, (word32)sizeof(buf)); @@ -7508,13 +7514,114 @@ static int test_SendEofUnconfirmedChannel(void) if (ret != WS_SUCCESS) { result = -1652; goto done; } if (!sess->eofTxd) { result = -1653; goto done; } -#ifndef NO_WOLFSSH_CLIENT done: wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); return result; } #endif /* NO_WOLFSSH_CLIENT */ + + +#ifndef NO_WOLFSSH_SERVER +/* The mirror of the above on the receiving side. DoChannelClose() finds its + * channel by self id, so a peer can name a locally opened channel it never + * confirmed. Its reply addresses the peer id, which is still 0 there, so an + * unguarded reply would half-close and close the live session channel + * instead. Retire the named channel and answer nothing. */ +static int test_DoChannelCloseUnconfirmedChannel(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* sess = NULL; + WOLFSSH_CHANNEL* pend = NULL; + int result = 0; + int ret; + word32 pendId; + byte pkt[16]; + byte buf[4] = { 0x00, 0x01, 0x02, 0x03 }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1672; + /* The socket takes nothing, so anything sent stays in the buffer where + * this can see it. */ + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1673; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + /* The live session channel, confirmed, holding peer id 0. */ + sess = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (sess == NULL) { result = -1674; goto done; } + if (ChannelAppend(ssh, sess) != WS_SUCCESS) { + ChannelDelete(sess, ssh->ctx->heap); + result = -1675; + goto done; + } + sess->openConfirmed = 1; + sess->peerChannel = 0; + sess->peerWindowSz = 1024; + sess->peerMaxPacketSz = 1024; + + /* A second channel of ours whose open the peer has not answered. */ + pend = ChannelNew(ssh, ID_CHANTYPE_TCPIP_DIRECT, 1024, 1024); + if (pend == NULL) { result = -1676; goto done; } + if (ChannelAppend(ssh, pend) != WS_SUCCESS) { + ChannelDelete(pend, ssh->ctx->heap); + result = -1677; + goto done; + } + if (pend->openConfirmed || pend->peerChannel != 0) { + result = -1678; + goto done; + } + pendId = pend->channel; + + /* The peer closes the channel it never confirmed. */ + s_recvPkt = pkt; + s_recvPktSz = BuildChannelClosePacket(pkt, pendId); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_CHANNEL_CLOSED) { result = -1679; goto done; } + if (ssh->lastRxId != pendId) { result = -1680; goto done; } + + /* Only the named channel went away. */ + if (ssh->channelListSz != 1) { result = -1681; goto done; } + if (ssh->channelList != sess) { result = -1682; goto done; } + + /* Nothing was addressed to peer id 0 on the session channel's behalf. */ + if (sess->eofTxd || sess->closeTxd) { result = -1683; goto done; } + if (PlainPacketsHaveMsg(ssh->outputBuffer.buffer, ssh->outputBuffer.idx, + ssh->outputBuffer.length, MSGID_CHANNEL_EOF)) { + result = -1684; + goto done; + } + if (PlainPacketsHaveMsg(ssh->outputBuffer.buffer, ssh->outputBuffer.idx, + ssh->outputBuffer.length, MSGID_CHANNEL_CLOSE)) { + result = -1685; + goto done; + } + + /* And the session's send direction is still live. */ + wolfSSH_SetIOSend(ctx, DiscardIoSend); + ret = wolfSSH_ChannelSend(sess, buf, (word32)sizeof(buf)); + if (ret != (int)sizeof(buf)) { result = -1686; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* NO_WOLFSSH_SERVER */ + + +#ifndef NO_WOLFSSH_CLIENT /* A bundled but unflushed EOF is committed: the bytes are in the output * buffer and go out on the next flush. The WS_WANT_WRITE retry an application * makes must not queue a second EOF behind the first. */ @@ -19399,6 +19506,13 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif /* NO_WOLFSSH_CLIENT */ +#ifndef NO_WOLFSSH_SERVER + unitResult = test_DoChannelCloseUnconfirmedChannel(); + printf("DoChannelCloseUnconfirmedChannel: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_SERVER */ + #ifndef NO_WOLFSSH_CLIENT unitResult = test_SendEofAfterDisconnect(); printf("SendEofAfterDisconnect: %s\n", diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 4577b447a..8633eefd3 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -331,7 +331,14 @@ WOLFSSH_API int wolfSSH_ChannelSendExt(WOLFSSH_CHANNEL* channel, * * A WS_WANT_WRITE means the teardown is incomplete: the close is only built * once the EOF is away, so call again until it reports something else. The - * retry costs nothing, a bundled EOF is not sent twice. */ + * retry costs nothing, a bundled EOF is not sent twice. WS_SUCCESS means both + * messages are bundled, not that they reached the peer: keep driving + * wolfSSH_worker() until it stops reporting WS_WANT_WRITE before dropping the + * socket. A channel whose open the peer has not confirmed has no peer id to + * address and reports WS_CHANNEL_NOT_CONF. + * + * A peer that never answers leaves the channel on the list for the life of + * the session; there is no reclaim short of wolfSSH_free(). */ WOLFSSH_API int wolfSSH_ChannelExit(WOLFSSH_CHANNEL* channel); /* Sends SSH_MSG_CHANNEL_EOF, closing the sending direction and leaving the * receiving direction open (the half-close of RFC 4254 section 5.3). Data From 24cf354922b6894a7fbaf5698233ce878ae97a12 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:08:57 -0700 Subject: [PATCH 09/17] Carry the EOF status through SFTP and SCP wolfSSH_SFTP_buffer_send() and ScpStreamSend() keep driving the worker when the peer half-closes. Both return any negative status, so a WS_EOF would have aborted a transfer that is still perfectly able to finish: the peer closed its sending direction, not ours. - wolfSSH_SFTP_buffer_read() reports every negative peek but a rekey instead of spending a receive on it. A drained channel at EOF, a dead session and a channel that is gone all mean no more data can arrive, and the poll only overwrites the latched cause with WS_WANT_READ or blocks on a peer that has hung up. - A rekey is not a drained channel: peek reports it before it looks at the buffer at all, so that one still needs the poll. - DoScpRequest() reads its own EOF case the same way as the rest. --- src/wolfscp.c | 15 +++++------ src/wolfsftp.c | 18 ++++++++++--- tests/unit.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 11 deletions(-) diff --git a/src/wolfscp.c b/src/wolfscp.c index 31dc9cf25..184d057cf 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -151,9 +151,10 @@ static int ScpStreamSend(WOLFSSH* ssh, byte* data, word32 sz) if (err == WS_WANT_READ || err == WS_WANT_WRITE) return err; /* Only a rekey/window/channel-data status means "keep driving". - * Any other negative status is fatal and returned. */ + * Any other negative status is fatal and returned. A peer EOF + * closes their direction only, and is raised once. */ if (ret < 0 && ret != WS_REKEYING && ret != WS_WINDOW_FULL - && ret != WS_CHAN_RXD) + && ret != WS_CHAN_RXD && ret != WS_EOF) return ret; /* otherwise loop and retry the send, which clears the status */ } @@ -944,15 +945,13 @@ int DoScpRequest(WOLFSSH* ssh) /* Peer MUST send back a SSH_MSG_CHANNEL_CLOSE unless already sent*/ ret = ScpStreamRead(ssh, buf, 1); - if (ret == WS_SOCKET_ERROR_E || ret == WS_CHANNEL_CLOSED) { + if (ret == WS_SOCKET_ERROR_E || ret == WS_CHANNEL_CLOSED + || ret == WS_EOF) { WLOG(WS_LOG_DEBUG, scpState, "Peer hung up, but SCP is done"); ret = WS_SUCCESS; } - else if (ret != WS_EOF) { - WLOG(WS_LOG_DEBUG, scpState, "Did not receive EOF packet"); - } else { - ret = WS_SUCCESS; + WLOG(WS_LOG_DEBUG, scpState, "Did not receive EOF packet"); } } @@ -1784,7 +1783,7 @@ int ReceiveScpMessage(WOLFSSH* ssh) } } - /* check if wolfSSH_worker returns 0 from handling a channel eof */ + /* Already at EOF, and the worker had nothing else to report. */ if (err == 0) { WOLFSSH_CHANNEL* channel; channel = wolfSSH_ChannelFind(ssh, lastChannel, WS_CHANNEL_ID_SELF); diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 42eca199f..cce35bff4 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -593,9 +593,9 @@ static int wolfSSH_SFTP_buffer_send(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer) /* Only a rekey/window/channel-data status means "keep driving". Any * other negative status (fatal error, or WS_WANT_READ/WS_WANT_WRITE on * a non-blocking socket) is returned so a stalled or dead rekey cannot - * spin forever. */ + * spin forever. A peer EOF ends their direction only, not ours. */ if (ret < 0 && ret != WS_REKEYING && ret != WS_WINDOW_FULL - && ret != WS_CHAN_RXD) { + && ret != WS_CHAN_RXD && ret != WS_EOF) { return ret; } err = wolfSSH_get_error(ssh); @@ -720,6 +720,7 @@ static int wolfSSH_SFTP_buffer_read(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer, { int ret; int polled; + int peekRet; byte peekBuf[1]; if (buffer == NULL || ssh == NULL) { @@ -759,7 +760,18 @@ static int wolfSSH_SFTP_buffer_read(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer, } } - if (!wolfSSH_stream_peek(ssh, peekBuf, 1)) { + peekRet = wolfSSH_stream_peek(ssh, peekBuf, 1); + + /* Every negative peek but a rekey means no more data can arrive: + * drained and at EOF, session gone, or channel gone. Polling would + * only overwrite the cause with WS_WANT_READ or block on a dead + * peer. A rekey is not a drained channel, so it still polls. */ + if (peekRet < 0 && peekRet != WS_REKEYING) { + return WS_FATAL_ERROR; + } + + /* Nothing buffered. Poll for the real status. */ + if (peekRet <= 0) { /* poll more data off the wire */ ret = wolfSSH_worker(ssh, NULL); polled = 1; diff --git a/tests/unit.c b/tests/unit.c index 8a779d561..77982d33e 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -6956,6 +6956,72 @@ static int test_DoChannelCloseFlushesReply(void) } +#ifdef WOLFSSH_SFTP +static int s_recvCalls = 0; + +/* Counts the polls, so a test can prove the wire was never touched. */ +static int CountingIoRecv(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + WOLFSSH_UNUSED(ssh); + WOLFSSH_UNUSED(buf); + WOLFSSH_UNUSED(sz); + WOLFSSH_UNUSED(ctx); + + s_recvCalls++; + return WS_CBIO_ERR_WANT_READ; +} + +/* The SFTP read must not poll a channel that has latched its EOF and been + * drained. No further data can arrive, so the poll cannot finish the message + * and only overwrites the latched WS_EOF with WS_WANT_READ, which the SFTP + * loops read as "come back later" -- or blocks outright on a blocking + * socket. */ +static int test_SftpReadEofNoPoll(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1620; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, CountingIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1621; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1622; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1623; + goto done; + } + ch->openConfirmed = 1; + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; + + /* The peer half-closed and everything it sent has been read. */ + ch->eofRxd = 1; + s_recvCalls = 0; + + ret = wolfSSH_SFTP_read(ssh); + if (ret != WS_FATAL_ERROR) { result = -1624; goto done; } + if (wolfSSH_get_error(ssh) != WS_EOF) { result = -1625; goto done; } + if (s_recvCalls != 0) { result = -1626; goto done; } + +done: + s_recvCalls = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* WOLFSSH_SFTP */ + /* DoReceive() can retire the head channel mid-read: DoChannelClose() frees it * while wolfSSH_stream_read() still holds its inputBuffer. If the next head @@ -19457,6 +19523,13 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; +#ifdef WOLFSSH_SFTP + unitResult = test_SftpReadEofNoPoll(); + printf("SftpReadEofNoPoll: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + unitResult = test_AcceptSurvivesChannelEof(); printf("AcceptSurvivesChannelEof: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); From d57b5bc58486cbc1f2f93cf550653e05c9862a7e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 13:10:17 -0700 Subject: [PATCH 10/17] Handle the EOF status in apps and examples Every in-tree caller of wolfSSH_worker() now recognises a peer half-close. wolfsshd's shell loop and both echoservers need it: all three ladders end in "else if (rc != WS_WANT_READ) break", and wolfsshd's reaches kill(childPid, SIGKILL), so without it a client half-close kills the command it just finished feeding. - wolfsshd closes the child's stdin off the channel's own EOF state instead of off a worker return of zero, which no longer happens on a half-close. - The echoservers answer the half-close off wolfSSH_ChannelGetEof() rather than the WS_EOF status: the flush inside wolfSSH_worker() can supersede that status, and it is raised once. They hand back the backlog first, finish a short send, and only send the EOF once the channel is empty. Answering is not conditional on the shell build, where an echo session is the default. - The SFTP loops peek before leaving, so a half-close with requests still buffered is served rather than dropped, and they report an ordinary session end as success. - The clients -- examples/client, scpclient, sftpclient, apps/wolfssh -- treat it as the graceful case instead of an error. apps/wolfssh counts it as a finished flush as well, since one worker pass can drain the queue and consume the peer's EOF together. - portfwd relays it to the local socket with shutdown(SHUT_WR) so a local reader waiting on end-of-input returns, once the backlog has genuinely been handed over: a read cut short by a rekey leaves the half-close for a later pass. - The Windows half of wolfsshd does not answer with an EOF of its own. That latches eofTxd and the child's remaining output would be refused, which is the defect this series removes from the library. - The mplabx port drains before tearing down, the way its SFTP read path already did; its worker arm was unreachable for a half-close until now. --- apps/wolfssh/wolfssh.c | 15 +- apps/wolfsshd/test/run_all_sshd_tests.sh | 1 + apps/wolfsshd/test/sshd_stdin_eof_test.sh | 132 ++++++++++++++++++ apps/wolfsshd/wolfsshd.c | 55 ++++++-- examples/client/client.c | 10 +- examples/echoserver/echoserver.c | 91 +++++++++++- examples/portfwd/portfwd.c | 42 +++++- examples/scpclient/scpclient.c | 12 +- examples/sftpclient/sftpclient.c | 8 +- .../wolfssh_echoserver/main/echoserver.c | 96 ++++++++++++- ide/mplabx/wolfssh.c | 9 +- 11 files changed, 432 insertions(+), 39 deletions(-) create mode 100755 apps/wolfsshd/test/sshd_stdin_eof_test.sh diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index 8a5a558f1..9cd153ab4 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -338,7 +338,7 @@ static int FlushQueuedSend(WOLFSSH* ssh, wolfSSL_Mutex* lock) * conversation is for the reader to sort out. A rekey started on the way * through is the reader's as well, the send itself went out. */ if (ret == WS_WANT_READ || ret == WS_CHAN_RXD || ret == WS_EXTDATA - || ret == WS_REKEYING) { + || ret == WS_REKEYING || ret == WS_EOF) { ret = WS_SUCCESS; } @@ -1362,14 +1362,17 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) ret = WS_SUCCESS; } } - else if (ret != WS_CHANNEL_CLOSED && ret != WS_WANT_READ) { + else if (ret != WS_CHANNEL_CLOSED && ret != WS_WANT_READ + && ret != WS_EOF) { WLOG(WS_LOG_DEBUG, "Sending the shutdown messages failed."); } - if (ret == WS_CHANNEL_CLOSED || ret == WS_WANT_READ) { - /* Shutting down. The channel closing isn't a fail, and neither - * is the peer having nothing ready on this non-blocking socket; - * either way there is nothing left to wait for. */ + if (ret == WS_CHANNEL_CLOSED || ret == WS_WANT_READ + || ret == WS_EOF) { + /* Shutting down. The channel closing or the peer's EOF isn't a + * fail, and neither is the peer having nothing ready on this + * non-blocking socket; either way there is nothing left to wait + * for. */ ret = WS_SUCCESS; } else if (ret != WS_SUCCESS) { diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index e9fa66a92..156d167b2 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -10,6 +10,7 @@ test_cases=( "sshd_bad_sftp_test.sh" "sshd_scp_fail.sh" "sshd_term_close_test.sh" + "sshd_stdin_eof_test.sh" "ssh_kex_algos.sh" ) diff --git a/apps/wolfsshd/test/sshd_stdin_eof_test.sh b/apps/wolfsshd/test/sshd_stdin_eof_test.sh new file mode 100755 index 000000000..651324324 --- /dev/null +++ b/apps/wolfsshd/test/sshd_stdin_eof_test.sh @@ -0,0 +1,132 @@ +#!/bin/bash +# bash, unlike the rest of this directory: the option array and PIPESTATUS +# below both need it. + +# A client that half-closes its stdin sends SSH_MSG_CHANNEL_EOF and waits for +# the command to finish. wolfSSHd must close the write end of the child's stdin +# pipe so a command reading to end-of-input returns, and it must hand over +# everything the peer sent before it does. +# +# Needs the OpenSSH client; the wolfSSH example client does not half-close. + +if [ -z "$1" ] || [ -z "$2" ]; then + echo "expecting host and port as arguments" + echo "./sshd_stdin_eof_test.sh 127.0.0.1 22222" + exit 1 +fi + +HOST="$1" +PORT="$2" +USER_NAME="${3:-`whoami`}" + +command -v ssh >/dev/null 2>&1 || { + echo "ssh not found, skipping" + exit 77 +} + +# The RESULT==124 assertions below are the point of the test. +command -v timeout >/dev/null 2>&1 || { + echo "timeout not found, skipping" + exit 77 +} + +# ssh refuses a group/world readable identity file. +KEYDIR=`mktemp -d 2>/dev/null` || KEYDIR=`mktemp -d -t sshdeof` +if [ -z "$KEYDIR" ] || [ ! -d "$KEYDIR" ]; then + echo "could not create temp dir" + exit 1 +fi +trap 'rm -rf "$KEYDIR"' EXIT + +cp ../../../keys/hansel-key-ecc.pem "$KEYDIR/id_ecdsa" || exit 1 +chmod 600 "$KEYDIR/id_ecdsa" + +SSH_OPTS=(-i "$KEYDIR/id_ecdsa" -p "$PORT" + -o IdentitiesOnly=yes + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null + -o PreferredAuthentications=publickey -o PasswordAuthentication=no + -o BatchMode=yes -o ConnectTimeout=5 -o LogLevel=ERROR) + +# An identity the client cannot load, or a host it cannot reach, is not this +# test's subject. sshd_exec_test.sh runs ahead of this one and owns a server +# that cannot run commands at all. +if ! timeout 20 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" true >/dev/null 2>&1 +then + echo "no session with these options, skipping" + exit 77 +fi + +# Case 1: a small input through 'sort'. 'sort' emits nothing until +# end-of-input, so a missed EOF is the timeout and a killed or hung child is +# empty output -- neither can pass by winning a race the way a streaming 'cat' +# can. The input is unsorted so the comparison also proves the remote command +# ran rather than the input being echoed back. +printf 'charlie\nalpha\nbravo\n' > "$KEYDIR/in.txt" + +# wolfsshd runs the user's login shell, so its startup files can print on +# either stream: stderr goes to a file, and stdout is filtered to the lines +# the command itself produced. +OUT=`timeout 20 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" 'sort' \ + < "$KEYDIR/in.txt" 2> "$KEYDIR/in.err" \ + | grep -E '^(alpha|bravo|charlie)$'` +RESULT=${PIPESTATUS[0]} + +if [ "$RESULT" == 124 ]; then + echo "session did not end after the client half-closed its stdin" + cat "$KEYDIR/in.err" + exit 1 +fi + +if [ "$RESULT" != 0 ]; then + echo "ssh failed with $RESULT" + cat "$KEYDIR/in.err" + exit 1 +fi + +if [ "$OUT" != "`sort "$KEYDIR/in.txt"`" ]; then + echo "unexpected output from the remote command" + echo "$OUT" + cat "$KEYDIR/in.err" + exit 1 +fi + +# Case 2: the same half-close with the send window full. The reader below +# stalls, so the client stops draining, the server's window to the peer fills +# while the client is still sending, and the EOF arrives with channel data +# still buffered on the server. Data held back that way has to be handed to +# the child before its stdin closes: dropping it truncates the output, and +# never handing it over leaves 'cat' waiting on a stdin that never closes. +awk 'BEGIN { for (i = 0; i < 200000; i++) + printf "%08d one two three four five six seven\n", (i * 48271) % 99991 + }' > "$KEYDIR/big.txt" + +timeout 90 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" 'cat' \ + < "$KEYDIR/big.txt" 2> "$KEYDIR/big.err" \ + | { sleep 5; cat; } > "$KEYDIR/big.out" +RESULT=${PIPESTATUS[0]} + +if [ "$RESULT" == 124 ]; then + echo "session did not end after a half-close with the window full" + cat "$KEYDIR/big.err" + exit 1 +fi + +if [ "$RESULT" != 0 ]; then + echo "ssh failed with $RESULT" + cat "$KEYDIR/big.err" + exit 1 +fi + +SENT=`wc -c < "$KEYDIR/big.txt"` +GOT=`wc -c < "$KEYDIR/big.out"` + +if [ "$GOT" -lt "$SENT" ] \ + || ! tail -c "$SENT" "$KEYDIR/big.out" | cmp -s - "$KEYDIR/big.txt" +then + echo "the remote command did not see all of the input" + echo "sent $SENT bytes, got $GOT bytes" + cat "$KEYDIR/big.err" + exit 1 +fi + +exit 0 diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index d4a60408e..6a4b2340d 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -908,6 +908,8 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, * if there is still pending sends */ } if (error == WS_EOF) { + /* An ordinary session end, not a failure. */ + ret = 0; break; } } @@ -934,10 +936,19 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, continue; } + /* Drain what is buffered first. A rekey is not a drained + * channel: peek reports it without looking. */ if (error == WS_EOF) { - break; + int peekRet = wolfSSH_stream_peek(ssh, NULL, 1); + + if (peekRet != WS_REKEYING && peekRet <= 0) { + /* An ordinary session end, not a failure. */ + ret = 0; + break; + } } - if (ret != WS_SUCCESS && ret != WS_CHAN_RXD) { + if (ret != WS_SUCCESS && ret != WS_CHAN_RXD + && ret != WS_EOF) { /* If not successful and no channel data, leave. */ break; } @@ -954,8 +965,11 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, error == WS_CHAN_RXD || error == WS_REKEYING || error == WS_WINDOW_FULL) ret = error; - if (error == WS_EOF) + if (error == WS_EOF) { + /* An ordinary session end, not a failure. */ + ret = 0; break; + } continue; } else if (ret == WS_REKEYING) { @@ -964,8 +978,11 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } else if (ret < 0) { error = wolfSSH_get_error(ssh); - if (error == WS_EOF) + if (error == WS_EOF) { + /* An ordinary session end, not a failure. */ + ret = 0; break; + } } if (ret == WS_FATAL_ERROR && error == 0) { @@ -1325,6 +1342,19 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, else if (rc == WS_CHANNEL_CLOSED) { continue; } + else if (rc == 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 + * below treat as fatal. wolfSSH_shutdown() sends it at + * teardown, as the POSIX copy relies on. Closing the + * write end of the child's stdin is still owed on this + * platform, and so is the per-pass drain: ptyIn is the + * terminal-resize context, and this copy reads into + * shellBuffer, which the windowFull resend owes the + * peer. Both want fixing where they can be tested. */ + continue; + } else if (rc != WS_WANT_READ) { break; } @@ -1518,8 +1548,10 @@ static int SHELL_FlushOut(WOLFSSH* ssh, WS_SOCKET_T sshFd, word32 channelId, if (wolfSSH_worker(ssh, NULL) < 0) { int err = wolfSSH_get_error(ssh); + /* A peer EOF during the final flush is expected. */ if (err != WS_WANT_READ && err != WS_WANT_WRITE && - err != WS_CHAN_RXD && err != WS_REKEYING) { + err != WS_CHAN_RXD && err != WS_REKEYING && + err != WS_EOF) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Issue draining connection on final flush"); return -1; @@ -1824,11 +1856,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, word32 shellChannelId = 0; WOLFSSH_CHANNEL* shellChannel; - /* Name the session channel off the channel list rather than trusting - * DEFAULT_NEXT_CHANNEL to be 0, which a build can override. It is the - * only channel open at this point; the agent channel comes later. The - * loop below closes the child's stdin off this channel, so a wrong id - * there drops the peer's input instead of handing it over. */ + /* Off the channel list, not DEFAULT_NEXT_CHANNEL, which a build can + * override. The session channel is the only one open here. lastRxId is + * no use: no request path sets it. A wrong id drops the peer's input. */ shellChannel = wolfSSH_ChannelNext(ssh, NULL); if (shellChannel == NULL || wolfSSH_ChannelGetId(shellChannel, &shellChannelId, WS_CHANNEL_ID_SELF) != WS_SUCCESS) { @@ -1960,6 +1990,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, peerConnected = 0; continue; } + else if (rc == WS_EOF) { + /* Half-close, handled below. */ + } else if (rc == WS_WANT_WRITE) { wantWrite = 1; continue; @@ -2716,7 +2749,7 @@ static void* HandleConnection(void* arg) error = wolfSSH_get_error(ssh); /* peer successfully closed down gracefully */ - if (ret == WS_CHANNEL_CLOSED) { + if (ret == WS_CHANNEL_CLOSED || ret == WS_EOF) { ret = 0; break; } diff --git a/examples/client/client.c b/examples/client/client.c index b397075e6..7abdffe65 100644 --- a/examples/client/client.c +++ b/examples/client/client.c @@ -1179,7 +1179,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) if (ret <= 0) { ret = wolfSSH_get_error(ssh); if (ret != WS_WANT_READ && ret != WS_WANT_WRITE && - ret != WS_CHAN_RXD) { + ret != WS_CHAN_RXD && ret != WS_EOF) { ClientFreeBuffers(pubKeyName, privKeyName, NULL); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); @@ -1198,7 +1198,9 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) #endif } ret = wolfSSH_shutdown(ssh); - /* do not continue on with shutdown process if peer already disconnected */ + /* do not continue on with shutdown process if peer already disconnected. + * A peer EOF is not a disconnect: the channel is still open and its close + * is still owed, so the drain below is exactly what is wanted. */ if (ret != WS_SOCKET_ERROR_E && wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E && wolfSSH_get_error(ssh) != WS_CHANNEL_CLOSED) { if (ret != WS_SUCCESS) { @@ -1209,7 +1211,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) } ret = wolfSSH_worker(ssh, NULL); if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E && - ret != WS_CHANNEL_CLOSED) { + ret != WS_CHANNEL_CLOSED && ret != WS_EOF) { ClientFreeBuffers(pubKeyName, privKeyName, NULL); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); @@ -1226,7 +1228,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E && - ret != WS_CHANNEL_CLOSED) { + ret != WS_CHANNEL_CLOSED && ret != WS_EOF) { err_sys("Closing client stream failed"); } diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 35094b4f8..b5f86c3ed 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -805,6 +805,10 @@ static int ssh_worker(thread_ctx_t* threadCtx) WOLFSSH* ssh; WS_SOCKET_T sshFd; int rc = 0; + int eofAnswered = 0; + /* Without a shell there is no child to outlive the peer's EOF, and the + * read path echoes unconditionally. */ + int echoOnly = 1; #ifdef WOLFSSH_SHELL const char *userName; struct passwd *p_passwd; @@ -822,6 +826,10 @@ static int ssh_worker(thread_ctx_t* threadCtx) if (ssh == NULL) return WS_FATAL_ERROR; +#ifdef WOLFSSH_SHELL + echoOnly = threadCtx->echo; +#endif + sshFd = wolfSSH_get_fd(ssh); #if defined(WOLFSSL_PTHREADS) && defined(WOLFSSL_TEST_GLOBAL_REQ) @@ -997,6 +1005,57 @@ static int ssh_worker(thread_ctx_t* threadCtx) channel. The additional channel is only used with the agent. */ cnt_r = wolfSSH_worker(ssh, &lastChannel); + + /* The peer is done sending: hand back the backlog and answer + * its EOF, or a client that half-closed waits on a server + * that never finishes -- the library no longer answers for + * us. Off the channel's own state, not the WS_EOF status: the + * flush inside wolfSSH_worker() can supersede that, and it is + * raised once. Echo mode only; a shell child on a pty is + * still producing, so its EOF waits for the child to exit. */ + if (!eofAnswered && echoOnly) { + WOLFSSH_CHANNEL* eofChannel; + + eofChannel = wolfSSH_ChannelFind(ssh, + threadCtx->shellCtx.channelId, WS_CHANNEL_ID_SELF); + if (eofChannel != NULL + && wolfSSH_ChannelGetEof(eofChannel)) { + int eofRead; + int eofSent; + int eofOff; + + do { + eofRead = wolfSSH_ChannelIdRead(ssh, + threadCtx->shellCtx.channelId, + threadCtx->channelBuffer, + sizeof threadCtx->channelBuffer); + eofOff = 0; + /* A send is bounded by the peer's window and + * packet size, so a short one is normal and the + * rest of the chunk is still owed. */ + while (eofOff < eofRead) { + eofSent = wolfSSH_ChannelIdSend(ssh, + threadCtx->shellCtx.channelId, + threadCtx->channelBuffer + eofOff, + eofRead - eofOff); + if (eofSent <= 0) + break; + eofOff += eofSent; + } + if (eofOff < eofRead) + break; + } while (eofRead > 0); + + /* Only an emptied channel earns the EOF; anything + * else is retried on a later pass. */ + if (eofRead == 0) { + wolfSSH_ChannelSendEof(eofChannel); + eofAnswered = 1; + ChildRunning = 0; + } + } + } + if (cnt_r < 0) { rc = wolfSSH_get_error(ssh); /* wolfSSH_worker() reports WS_REKEYING in place of @@ -1117,6 +1176,11 @@ static int ssh_worker(thread_ctx_t* threadCtx) #endif continue; } + else if (rc == WS_EOF) { + /* The half-close is answered by the durable check + * above, which has already run this pass. */ + continue; + } else if (rc != WS_WANT_READ) { #ifdef SHELL_DEBUG printf("Break:read sshFd returns %d: errno =%x\n", @@ -1439,6 +1503,8 @@ static int sftp_worker(thread_ctx_t* threadCtx) * if there is still pending sends */ } if (error == WS_EOF) { + /* An ordinary session end, not a failure. */ + ret = 0; break; } } @@ -1465,10 +1531,18 @@ static int sftp_worker(thread_ctx_t* threadCtx) ret = error; } + /* Drain what is buffered before leaving on the EOF. */ if (error == WS_EOF) { - break; + /* A rekey is not a drained channel. */ + int peekRet = wolfSSH_stream_peek(ssh, NULL, 1); + + if (peekRet != WS_REKEYING && peekRet <= 0) { + /* An ordinary session end, not a failure. */ + ret = 0; + break; + } } - if (ret != WS_SUCCESS && ret != WS_CHAN_RXD) { + if (ret != WS_SUCCESS && ret != WS_CHAN_RXD && ret != WS_EOF) { #ifdef WOLFSSH_TEST_BLOCK if (error == WS_WANT_READ) { while (error == WS_WANT_READ) { @@ -1504,8 +1578,10 @@ static int sftp_worker(thread_ctx_t* threadCtx) error == WS_CHAN_RXD || error == WS_REKEYING || error == WS_WINDOW_FULL) ret = error; - if (error == WS_EOF) + if (error == WS_EOF) { + ret = 0; break; + } continue; } else if (ret == WS_REKEYING) { @@ -1675,6 +1751,13 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) ret = 0; } + /* The peer's close already retired the channel: a completed + * shutdown, not a failure. Left non-zero it sets quit, taking the + * server down after one session. */ + if (ret == WS_CHANNEL_CLOSED) { + ret = 0; + } + error = wolfSSH_get_error(threadCtx->ssh); if (error != WS_SOCKET_ERROR_E && (error == WS_WANT_READ || error == WS_WANT_WRITE)) { @@ -1686,7 +1769,7 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) error = wolfSSH_get_error(threadCtx->ssh); /* peer successfully closed down gracefully */ - if (ret == WS_CHANNEL_CLOSED) { + if (ret == WS_CHANNEL_CLOSED || ret == WS_EOF) { ret = 0; break; } diff --git a/examples/portfwd/portfwd.c b/examples/portfwd/portfwd.c index 7e26a9697..e562ac4cf 100644 --- a/examples/portfwd/portfwd.c +++ b/examples/portfwd/portfwd.c @@ -431,6 +431,7 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) int ret; int ch; int appFdSet = 0; + int appFdHalfClosed = 0; int reverse = 0; int fwdFromPortSet = 0; PortfwdState fwdState; @@ -641,7 +642,8 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) ret = wolfSSH_worker(ssh, NULL); if (ret != WS_SUCCESS && ret != WS_CHAN_RXD && ret != WS_WANT_READ && ret != WS_WANT_WRITE && - ret != WS_WINDOW_FULL && ret != WS_REKEYING) + ret != WS_WINDOW_FULL && ret != WS_REKEYING && + ret != WS_EOF) err_sys("Couldn't get the remote forward reply."); } if (!fwdState.replied) @@ -765,6 +767,44 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) break; } + /* Relay the half-close so a local reader waiting on end-of-input + * returns; nothing else relays it. Driven off the latched channel + * state, not the WS_EOF status: the flush inside wolfSSH_worker() + * can supersede that, and it is raised only once. Only the channel + * appFd is wired to, since half-closing the wrong socket truncates + * a live transfer. */ + if (appFdSet && fwdChannel != NULL && !appFdHalfClosed + && wolfSSH_ChannelGetEof(fwdChannel)) { + int drained; + + /* Hand over the backlog first, or the local reader sees a + * clean end-of-input short of what the peer sent. A negative + * read is a rekey or a stalled channel, not a drained one, so + * only an empty read earns the half-close; the latch stays + * clear and a later pass tries again. */ + do { + drained = wolfSSH_ChannelRead(fwdChannel, sshBuffer, + sshBufferSz); + if (drained > 0) { + if ((int)send(appFd, sshBuffer, drained, 0) != drained) + break; + } + } while (drained > 0); + + if (drained == 0) { + appFdHalfClosed = 1; + #ifdef SHUT_WR + shutdown(appFd, SHUT_WR); + #elif defined(SD_SEND) + shutdown(appFd, SD_SEND); + #else + printf("No way to half-close the local socket, " + "the local reader may wait for input that is " + "not coming.\n"); + #endif + } + } + if (ret == WS_CHAN_RXD) { WOLFSSH_CHANNEL* readChannel; diff --git a/examples/scpclient/scpclient.c b/examples/scpclient/scpclient.c index 8b817ad5f..aaa2d7fd4 100644 --- a/examples/scpclient/scpclient.c +++ b/examples/scpclient/scpclient.c @@ -317,7 +317,9 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) } ret = wolfSSH_shutdown(ssh); - /* do not continue on with shutdown process if peer already disconnected */ + /* do not continue on with shutdown process if peer already disconnected. + * A peer EOF is not a disconnect: the channel is still open and its close + * is still owed, so the drain below is exactly what is wanted. */ if (ret != WS_CHANNEL_CLOSED && ret != WS_SOCKET_ERROR_E && wolfSSH_get_error(ssh) != WS_SOCKET_ERROR_E && wolfSSH_get_error(ssh) != WS_CHANNEL_CLOSED) { @@ -326,7 +328,8 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) } else { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret != WS_CHANNEL_CLOSED) { + if (ret != WS_SUCCESS && ret != WS_CHANNEL_CLOSED + && ret != WS_EOF) { WLOG(WS_LOG_DEBUG, "Failed to listen for close messages from the peer."); } @@ -336,7 +339,7 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E && - ret != WS_CHANNEL_CLOSED) { + ret != WS_CHANNEL_CLOSED && ret != WS_EOF) { WLOG(WS_LOG_DEBUG, "Closing scp stream failed. Connection could have been closed by peer"); } @@ -346,7 +349,8 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) wc_ecc_fp_free(); /* free per thread cache */ #endif - if ((ret != WS_SUCCESS) && (ret != WS_CHANNEL_CLOSED)) + if ((ret != WS_SUCCESS) && (ret != WS_CHANNEL_CLOSED) + && (ret != WS_EOF)) ((func_args*)args)->return_code = 1; return 0; } diff --git a/examples/sftpclient/sftpclient.c b/examples/sftpclient/sftpclient.c index f78c3306d..40841f831 100644 --- a/examples/sftpclient/sftpclient.c +++ b/examples/sftpclient/sftpclient.c @@ -1501,7 +1501,7 @@ static int doAutopilot(int cmd, char* local, char* remote) /* wolfSSH_worker returns WS_FATAL_ERROR when the socket * would block (DoReceive -> GetInputData -> WS_WANT_READ), * so check ssh->error rather than ret for blocking conditions. */ - if (ret != WS_SUCCESS && ret != WS_CHAN_RXD && + if (ret != WS_SUCCESS && ret != WS_CHAN_RXD && ret != WS_EOF && wolfSSH_get_error(ssh) != WS_WANT_READ && wolfSSH_get_error(ssh) != WS_WANT_WRITE) break; @@ -1815,7 +1815,9 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args) int err; ret = wolfSSH_shutdown(ssh); - /* peer hung up or channel already closed, stop trying */ + /* peer hung up or channel already closed, stop trying. + * wolfSSH_shutdown() folds a peer EOF into WS_SUCCESS itself, so + * there is no WS_EOF to test for here. */ if (ret == WS_SOCKET_ERROR_E || ret == WS_ERROR || ret == WS_CHANNEL_CLOSED) { ret = 0; @@ -1832,7 +1834,7 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args) err = wolfSSH_get_error(ssh); /* peer successfully closed down gracefully */ - if (ret == WS_CHANNEL_CLOSED) { + if (ret == WS_CHANNEL_CLOSED || ret == WS_EOF) { ret = 0; break; } 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 b84c69c65..a42276ddb 100644 --- a/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c +++ b/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c @@ -793,6 +793,10 @@ static int ssh_worker(thread_ctx_t* threadCtx) WOLFSSH* ssh; WS_SOCKET_T sshFd; int rc = 0; + int eofAnswered = 0; + /* Without a shell there is no child to outlive the peer's EOF, and the + * read path echoes unconditionally. */ + int echoOnly = 1; #ifdef WOLFSSH_SHELL const char *userName; struct passwd *p_passwd; @@ -810,6 +814,10 @@ static int ssh_worker(thread_ctx_t* threadCtx) if (ssh == NULL) return WS_FATAL_ERROR; +#ifdef WOLFSSH_SHELL + echoOnly = threadCtx->echo; +#endif + sshFd = wolfSSH_get_fd(ssh); #if defined(WOLFSSL_PTHREADS) && defined(WOLFSSL_TEST_GLOBAL_REQ) @@ -981,6 +989,57 @@ static int ssh_worker(thread_ctx_t* threadCtx) channel. The additional channel is only used with the agent. */ cnt_r = wolfSSH_worker(ssh, &lastChannel); + + /* The peer is done sending: hand back the backlog and answer + * its EOF, or a client that half-closed waits on a server + * that never finishes -- the library no longer answers for + * us. Off the channel's own state, not the WS_EOF status: the + * flush inside wolfSSH_worker() can supersede that, and it is + * raised once. Echo mode only; a shell child on a pty is + * still producing, so its EOF waits for the child to exit. */ + if (!eofAnswered && echoOnly) { + WOLFSSH_CHANNEL* eofChannel; + + eofChannel = wolfSSH_ChannelFind(ssh, shellChannelId, + WS_CHANNEL_ID_SELF); + if (eofChannel != NULL + && wolfSSH_ChannelGetEof(eofChannel)) { + int eofRead; + int eofSent; + int eofOff; + + do { + eofRead = wolfSSH_ChannelIdRead(ssh, + shellChannelId, + threadCtx->channelBuffer, + sizeof threadCtx->channelBuffer); + eofOff = 0; + /* A send is bounded by the peer's window and + * packet size, so a short one is normal and the + * rest of the chunk is still owed. */ + while (eofOff < eofRead) { + eofSent = wolfSSH_ChannelIdSend(ssh, + shellChannelId, + threadCtx->channelBuffer + eofOff, + eofRead - eofOff); + if (eofSent <= 0) + break; + eofOff += eofSent; + } + if (eofOff < eofRead) + break; + } while (eofRead > 0); + + /* Only an emptied channel earns the EOF; anything + * else is retried on a later pass. */ + if (eofRead == 0) { + wolfSSH_ChannelSendEof(eofChannel); + eofAnswered = 1; + ChildRunning = 0; + } + } + } + if (cnt_r < 0) { rc = wolfSSH_get_error(ssh); if (rc == WS_CHAN_RXD) { @@ -1082,6 +1141,11 @@ static int ssh_worker(thread_ctx_t* threadCtx) #endif continue; } + else if (rc == WS_EOF) { + /* The half-close is answered by the durable check + * above, which has already run this pass. */ + continue; + } else if (rc != WS_WANT_READ) { #ifdef SHELL_DEBUG printf("Break:read sshFd returns %d: errno =%x\n", @@ -1379,6 +1443,8 @@ static int sftp_worker(thread_ctx_t* threadCtx) * if there is still pending sends */ } if (error == WS_EOF) { + /* An ordinary session end, not a failure. */ + ret = 0; break; } } @@ -1405,10 +1471,18 @@ static int sftp_worker(thread_ctx_t* threadCtx) ret = error; } + /* Drain what is buffered before leaving on the EOF. */ if (error == WS_EOF) { - break; + /* A rekey is not a drained channel. */ + int peekRet = wolfSSH_stream_peek(ssh, NULL, 1); + + if (peekRet != WS_REKEYING && peekRet <= 0) { + /* An ordinary session end, not a failure. */ + ret = 0; + break; + } } - if (ret != WS_SUCCESS && ret != WS_CHAN_RXD) { + if (ret != WS_SUCCESS && ret != WS_CHAN_RXD && ret != WS_EOF) { if (ret == WS_WANT_WRITE) { /* recall wolfSSH_worker here because is likely our custom * highwater callback that returned up a WS_WANT_WRITE */ @@ -1431,8 +1505,10 @@ static int sftp_worker(thread_ctx_t* threadCtx) error == WS_CHAN_RXD || error == WS_REKEYING || error == WS_WINDOW_FULL) ret = error; - if (error == WS_EOF) + if (error == WS_EOF) { + ret = 0; break; + } continue; } else if (ret == WS_REKEYING) { @@ -1441,8 +1517,11 @@ static int sftp_worker(thread_ctx_t* threadCtx) } else if (ret < 0) { error = wolfSSH_get_error(ssh); - if (error == WS_EOF) + if (error == WS_EOF) { + /* shutdown is happening, clear peek error */ + ret = 0; break; + } } if (ret == WS_FATAL_ERROR && error == 0) { @@ -1574,6 +1653,13 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) ret = 0; } + /* The peer's close already retired the channel: a completed + * shutdown, not a failure. Left non-zero it sets quit, taking the + * server down after one session. */ + if (ret == WS_CHANNEL_CLOSED) { + ret = 0; + } + error = wolfSSH_get_error(threadCtx->ssh); if (error != WS_SOCKET_ERROR_E && (error == WS_WANT_READ || error == WS_WANT_WRITE)) { @@ -1585,7 +1671,7 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) error = wolfSSH_get_error(threadCtx->ssh); /* peer succesfully closed down gracefully */ - if (ret == WS_CHANNEL_CLOSED) { + if (ret == WS_CHANNEL_CLOSED || ret == WS_EOF) { ret = 0; break; } diff --git a/ide/mplabx/wolfssh.c b/ide/mplabx/wolfssh.c index a2bbbd774..1adfde4b7 100644 --- a/ide/mplabx/wolfssh.c +++ b/ide/mplabx/wolfssh.c @@ -841,8 +841,15 @@ void APP_Tasks ( void ) break; } + /* Drain what the peer sent before tearing down. A rekey is not + * a drained channel: peek reports it without looking. */ if (error == WS_EOF) { - appData.state = APP_SSH_CLEANUP; + int peekRet = wolfSSH_stream_peek(ssh, peek_buf, + sizeof(peek_buf)); + + if (peekRet != WS_REKEYING && peekRet <= 0) { + appData.state = APP_SSH_CLEANUP; + } break; } From cadb03b90fb99a6c2a51971b3490e9f2fe58fd9e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 21:45:46 -0700 Subject: [PATCH 11/17] Never wait on the child in the shell loop SHELL_Subsystem() is the only reader of the child's output, so it must never be the thing the child is waiting for. It was: the pass that writes the peer's input to the child's stdin ran ahead of the pass that reads its stdout, and on a pass with buffered channel data the output descriptors were left out of the select() altogether. A child that fills its stdout pipe stops reading stdin, the write blocks, and nothing is left to empty the pipe that would release it. sshd_stdin_eof_test.sh case 2 is the shape that reaches it: a half-close with the send window full leaves the whole window buffered, and the burst that follows is up to four 32K writes with no read in between. - The child's output is watched on every pass. A pass with work already in hand polls with a zero timeout instead of skipping select(), so it still sees the child's output. - The descriptor written to is non-blocking, and what a short write leaves is carried in channelBuffer to the next pass, which waits for the child in select() rather than inside write(). Only EAGAIN keeps the remainder; any other short write still ends the session. - The child's stdin closes on the peer's EOF once that remainder is gone too, not just once the channel is drained. - A channel retired under us drops the remainder with the descriptor. --- apps/wolfsshd/wolfsshd.c | 160 ++++++++++++++++++++++++++------------- 1 file changed, 109 insertions(+), 51 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 6a4b2340d..0df71032d 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1618,6 +1618,11 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, int peerConnected = 1; int stdoutEmpty = 0; int ptyReq = 0; + int childInSz = 0; /* Bytes read off the channel into channelBuffer + * that the child has yet to take. The read is + * destructive, so what a short write leaves is + * carried to the next pass. */ + int childInIdx = 0; /* How much of those the child has taken. */ childFd = -1; stdoutPipe[0] = -1; @@ -1904,6 +1909,14 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, close(stdinPipe[0]); } + /* The loop below is the only reader of the child's output, so it must + * never be the thing waiting on the child. A descriptor that cannot be + * made non-blocking keeps the old behaviour: the write can stall, which + * is still better than dropping the peer's input on the floor. The read + * paths already treat EAGAIN as nothing to report. */ + (void)SHELL_SetNonBlocking((!ptyReq || forcedCmd) ? + stdinPipe[1] : childFd); + while (ChildRunning || windowFull || !stdoutEmpty || peerConnected) { byte tmp[2]; fd_set readFds; @@ -1913,6 +1926,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, int cnt_w; WOLFSSH_CHANNEL* current; int pending = 0; + int childStalled = 0; /* The child would not take the rest of what it + * is owed on this pass. */ FD_ZERO(&readFds); FD_SET(sshFd, &readFds); @@ -1933,24 +1948,53 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, pending = 1; } - if (!pending && wolfSSH_stream_peek(ssh, tmp, 1) <= 0) { - /* select on stdout/stderr pipes with forced commands */ - if (!ptyReq || forcedCmd) { - FD_SET(stdoutPipe[0], &readFds); - if (stdoutPipe[0] > maxFd) - maxFd = stdoutPipe[0]; + if (!pending && wolfSSH_stream_peek(ssh, tmp, 1) > 0) { + pending = 1; /* found some pending SSH data */ + } + + /* The child's output is watched on every pass, not only the ones + * that have nothing else to do. Draining it is what lets a child + * blocked writing carry on reading its stdin, so a pass that feeds + * the child has to be a pass that empties it as well. */ + if (!ptyReq || forcedCmd) { + FD_SET(stdoutPipe[0], &readFds); + if (stdoutPipe[0] > maxFd) + maxFd = stdoutPipe[0]; + + FD_SET(stderrPipe[0], &readFds); + if (stderrPipe[0] > maxFd) + maxFd = stderrPipe[0]; + } + else { + FD_SET(childFd, &readFds); + if (childFd > maxFd) + maxFd = childFd; + } + + /* Bytes the child has not taken yet: wait for it to make room. */ + if (childInIdx < childInSz) { + int childIn = (!ptyReq || forcedCmd) ? stdinPipe[1] : childFd; - FD_SET(stderrPipe[0], &readFds); - if (stderrPipe[0] > maxFd) - maxFd = stderrPipe[0]; + if (childIn != -1) { + FD_SET(childIn, &writeFds); + if (childIn > maxFd) + maxFd = childIn; } - else { - FD_SET(childFd, &readFds); - if (childFd > maxFd) - maxFd = childFd; + } + + { + struct timeval noWait; + struct timeval* timeout = NULL; + + /* Work already in hand must not wait on the descriptors, but the + * poll still runs so this pass sees the child's output too. */ + if (pending) { + noWait.tv_sec = 0; + noWait.tv_usec = 0; + timeout = &noWait; } - rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, NULL); + rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, timeout); if (rc == -1) { /* Signal (e.g. SIGCHLD from child exit) interrupted select. * Re-evaluate the loop condition so any pending windowFull @@ -1960,11 +2004,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, break; } } - else { - pending = 1; /* found some pending SSH data */ - } - if (wantWrite || windowFull || pending || FD_ISSET(sshFd, &readFds)) { + if (wantWrite || windowFull || pending || childInIdx < childInSz + || FD_ISSET(sshFd, &readFds)) { word32 avail; wantWrite = 0; @@ -1986,6 +2028,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, if (stdinPipe[1] != -1 && (!ptyReq || forcedCmd)) { close(stdinPipe[1]); stdinPipe[1] = -1; + /* Nothing left to write it to. */ + childInIdx = 0; + childInSz = 0; } peerConnected = 0; continue; @@ -2019,51 +2064,63 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, * while the window was full; this is the only copy. Gated on * windowFull, and not because the buffers overlap -- shellBuffer * and channelBuffer are disjoint. While the peer will not take - * the child's output, writing to the child's stdin deadlocks it: - * it blocks on a full stdout pipe, stops reading stdin, and this - * write never returns. The backlog clears as soon as the peer - * reads. One buffer per pass, since the write can block. */ - if (avail > 0 && !windowFull) { - int off = 0; - + * the child's output, pulling more off the channel only parks it + * somewhere the shutdown path cannot see. The backlog clears as + * soon as the peer reads. One buffer at a time: what the child + * will not take yet is carried to the next pass. */ + if (childInIdx == childInSz && avail > 0 && !windowFull) { cnt_r = wolfSSH_ChannelIdRead(ssh, shellChannelId, channelBuffer, sizeof channelBuffer); if (cnt_r <= 0) break; + childInIdx = 0; + childInSz = cnt_r; + /* Data behind the peer's EOF, RFC 4254 section 5.3. Stdin * is gone, so drop it rather than write to fd -1 and end the * session on an EBADF. */ if ((!ptyReq || forcedCmd) && stdinPipe[1] == -1) - off = cnt_r; - - /* The read took the bytes off the channel, so this is the - * only copy: a short write has to be finished, not dropped. - * A PTY master goes short whenever the line discipline fills, - * and a signal can cut a transfer already under way. */ - while (off < cnt_r) { - if (!ptyReq || forcedCmd) { - cnt_w = (int)write(stdinPipe[1], channelBuffer + off, - cnt_r - off); - } - else { - cnt_w = (int)write(childFd, channelBuffer + off, - cnt_r - off); - } - if (cnt_w <= 0) { - /* errno only speaks for a -1 return. */ - if (cnt_w < 0 && errno == EINTR) - continue; - break; - } - off += cnt_w; - } - if (off < cnt_r) - break; + childInIdx = childInSz; avail = current->inputBuffer.length - current->inputBuffer.idx; } + /* The read took the bytes off the channel, so this is the only + * copy: a short write has to be finished, not dropped. The + * descriptor is non-blocking, so a child that has stopped + * reading leaves the rest here for the next pass rather than + * parking this loop inside write() -- which is the deadlock, + * since the child stops reading exactly when its output has + * nowhere to go and only this loop can empty it. */ + while (childInIdx < childInSz) { + if (!ptyReq || forcedCmd) { + cnt_w = (int)write(stdinPipe[1], channelBuffer + childInIdx, + childInSz - childInIdx); + } + else { + cnt_w = (int)write(childFd, channelBuffer + childInIdx, + childInSz - childInIdx); + } + if (cnt_w <= 0) { + /* errno only speaks for a -1 return. */ + if (cnt_w < 0 && errno == EINTR) + continue; + if (cnt_w < 0 + && (errno == EAGAIN || errno == EWOULDBLOCK)) { + childStalled = 1; + } + break; + } + childInIdx += cnt_w; + } + if (childInIdx < childInSz && !childStalled) + break; + if (childInIdx == childInSz) { + childInIdx = 0; + childInSz = 0; + } + /* Peer done sending: close the child's stdin, but only once what * it already sent has been handed over. Closing early drops it * and the next write lands on fd -1. A channel that is gone is @@ -2073,7 +2130,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, * a stdin nothing will ever close. */ if (stdinPipe[1] != -1 && (!ptyReq || forcedCmd)) { if (current == NULL - || (wolfSSH_ChannelGetEof(current) && avail == 0)) { + || (wolfSSH_ChannelGetEof(current) && avail == 0 + && childInIdx == childInSz)) { /* SSH is done, close stdin pipe to child process */ close(stdinPipe[1]); stdinPipe[1] = -1; From c0c7e47f8dd4e511595504b3c97c3b0715fc4cbb Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 28 Aug 2026 12:47:02 -0700 Subject: [PATCH 12/17] Hold the EOF drain's unsent tail across passes A chunk read out of the channel is gone from it, so breaking the drain on a non-positive send dropped whatever the send had not taken, and the echo back to a half-closing peer came up short. - Keep the chunk and its offset across worker passes, and read the next chunk only once the last one is out - Give the drain its own buffer; the read path below it reuses channelBuffer in the same pass - Answer the EOF off a drained flag, since a held tail means a zero read count no longer marks an emptied channel --- examples/echoserver/echoserver.c | 53 ++++++++++++------- .../wolfssh_echoserver/main/echoserver.c | 53 ++++++++++++------- 2 files changed, 68 insertions(+), 38 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index b5f86c3ed..482d184df 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -200,6 +200,9 @@ typedef struct { #endif WS_AppCtx shellCtx; byte channelBuffer[EXAMPLE_BUFFER_SZ]; + /* The EOF drain holds an unsent tail across worker passes, + * so it cannot share channelBuffer with the read path. */ + byte eofBuffer[EXAMPLE_BUFFER_SZ]; char statsBuffer[EXAMPLE_BUFFER_SZ]; } thread_ctx_t; @@ -806,6 +809,9 @@ static int ssh_worker(thread_ctx_t* threadCtx) WS_SOCKET_T sshFd; int rc = 0; int eofAnswered = 0; + /* Held across passes with 0 <= eofOff <= eofRead. */ + int eofRead = 0; + int eofOff = 0; /* Without a shell there is no child to outlive the peer's EOF, and the * read path echoes unconditionally. */ int echoOnly = 1; @@ -1020,35 +1026,44 @@ static int ssh_worker(thread_ctx_t* threadCtx) threadCtx->shellCtx.channelId, WS_CHANNEL_ID_SELF); if (eofChannel != NULL && wolfSSH_ChannelGetEof(eofChannel)) { - int eofRead; int eofSent; - int eofOff; + int eofDrained = 0; - do { - eofRead = wolfSSH_ChannelIdRead(ssh, - threadCtx->shellCtx.channelId, - threadCtx->channelBuffer, - sizeof threadCtx->channelBuffer); - eofOff = 0; + for (;;) { /* A send is bounded by the peer's window and - * packet size, so a short one is normal and the - * rest of the chunk is still owed. */ - while (eofOff < eofRead) { - eofSent = wolfSSH_ChannelIdSend(ssh, + * packet size, so a short one is normal. Read + * the next chunk only once the last one is out: + * the read consumed it from the channel, so its + * tail cannot be dropped. */ + if (eofOff == eofRead) { + int eofRxd; + + eofOff = eofRead = 0; + eofRxd = wolfSSH_ChannelIdRead(ssh, threadCtx->shellCtx.channelId, - threadCtx->channelBuffer + eofOff, - eofRead - eofOff); - if (eofSent <= 0) + threadCtx->eofBuffer, + sizeof threadCtx->eofBuffer); + /* A negative read is a rekey or a stalled + * channel, not a drained one. */ + if (eofRxd <= 0) { + eofDrained = (eofRxd == 0); break; - eofOff += eofSent; + } + eofRead = eofRxd; } - if (eofOff < eofRead) + + eofSent = wolfSSH_ChannelIdSend(ssh, + threadCtx->shellCtx.channelId, + threadCtx->eofBuffer + eofOff, + eofRead - eofOff); + if (eofSent <= 0) break; - } while (eofRead > 0); + eofOff += eofSent; + } /* Only an emptied channel earns the EOF; anything * else is retried on a later pass. */ - if (eofRead == 0) { + if (eofDrained) { wolfSSH_ChannelSendEof(eofChannel); eofAnswered = 1; ChildRunning = 0; 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 a42276ddb..6baa1af00 100644 --- a/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c +++ b/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c @@ -210,6 +210,9 @@ typedef struct { byte shellBuffer[EXAMPLE_BUFFER_SZ]; #endif byte channelBuffer[EXAMPLE_BUFFER_SZ]; + /* The EOF drain holds an unsent tail across worker passes, + * so it cannot share channelBuffer with the read path. */ + byte eofBuffer[EXAMPLE_BUFFER_SZ]; char statsBuffer[EXAMPLE_BUFFER_SZ]; } thread_ctx_t; @@ -794,6 +797,9 @@ static int ssh_worker(thread_ctx_t* threadCtx) WS_SOCKET_T sshFd; int rc = 0; int eofAnswered = 0; + /* Held across passes with 0 <= eofOff <= eofRead. */ + int eofRead = 0; + int eofOff = 0; /* Without a shell there is no child to outlive the peer's EOF, and the * read path echoes unconditionally. */ int echoOnly = 1; @@ -1004,35 +1010,44 @@ static int ssh_worker(thread_ctx_t* threadCtx) WS_CHANNEL_ID_SELF); if (eofChannel != NULL && wolfSSH_ChannelGetEof(eofChannel)) { - int eofRead; int eofSent; - int eofOff; + int eofDrained = 0; - do { - eofRead = wolfSSH_ChannelIdRead(ssh, - shellChannelId, - threadCtx->channelBuffer, - sizeof threadCtx->channelBuffer); - eofOff = 0; + for (;;) { /* A send is bounded by the peer's window and - * packet size, so a short one is normal and the - * rest of the chunk is still owed. */ - while (eofOff < eofRead) { - eofSent = wolfSSH_ChannelIdSend(ssh, + * packet size, so a short one is normal. Read + * the next chunk only once the last one is out: + * the read consumed it from the channel, so its + * tail cannot be dropped. */ + if (eofOff == eofRead) { + int eofRxd; + + eofOff = eofRead = 0; + eofRxd = wolfSSH_ChannelIdRead(ssh, shellChannelId, - threadCtx->channelBuffer + eofOff, - eofRead - eofOff); - if (eofSent <= 0) + threadCtx->eofBuffer, + sizeof threadCtx->eofBuffer); + /* A negative read is a rekey or a stalled + * channel, not a drained one. */ + if (eofRxd <= 0) { + eofDrained = (eofRxd == 0); break; - eofOff += eofSent; + } + eofRead = eofRxd; } - if (eofOff < eofRead) + + eofSent = wolfSSH_ChannelIdSend(ssh, + shellChannelId, + threadCtx->eofBuffer + eofOff, + eofRead - eofOff); + if (eofSent <= 0) break; - } while (eofRead > 0); + eofOff += eofSent; + } /* Only an emptied channel earns the EOF; anything * else is retried on a later pass. */ - if (eofRead == 0) { + if (eofDrained) { wolfSSH_ChannelSendEof(eofChannel); eofAnswered = 1; ChildRunning = 0; From 97d696d48faedd4c1e87488a32d75fbf1b2d1f83 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 28 Aug 2026 12:47:02 -0700 Subject: [PATCH 13/17] Re-resolve the forwarded channel before the half-close check A refused channel open frees the channel and surfaces as a fatal error rather than a close, so the guard that clears fwdChannel never runs and the half-close check read freed memory on every refused -L forward. - Stash the channel id wherever the channel is created or adopted - Look the channel up by id each pass and stop once it is gone --- examples/portfwd/portfwd.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/examples/portfwd/portfwd.c b/examples/portfwd/portfwd.c index e562ac4cf..e406368dc 100644 --- a/examples/portfwd/portfwd.c +++ b/examples/portfwd/portfwd.c @@ -438,6 +438,7 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) int replyTries; struct timeval to; WOLFSSH_CHANNEL* fwdChannel = NULL; + word32 fwdChannelId = 0; byte* appBuffer = NULL; byte* sshBuffer = NULL; word32 appBufferSz = 0; @@ -746,6 +747,7 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) if (fwdState.appFd != (SOCKET_T)-1 && newChannel != NULL) { appFd = fwdState.appFd; fwdChannel = newChannel; + fwdChannelId = fwdState.channelId; FD_SET(appFd, &templateFds); nFds = findMax((int)sshFd, (int)appFd) + 1; appFdSet = 1; @@ -767,6 +769,16 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) break; } + /* A refused open frees the channel and reports a fatal + * error, not a close, so the guard above does not run. + * Re-resolve by id rather than trust the pointer. */ + if (fwdChannel != NULL) { + fwdChannel = wolfSSH_ChannelFind(ssh, fwdChannelId, + WS_CHANNEL_ID_SELF); + if (fwdChannel == NULL) + break; + } + /* Relay the half-close so a local reader waiting on end-of-input * returns; nothing else relays it. Driven off the latched channel * state, not the WS_EOF status: the flush inside wolfSSH_worker() @@ -836,6 +848,10 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) appFdSet = 1; fwdChannel = wolfSSH_ChannelFwdNew(ssh, fwdToHost, fwdToPort, fwdFromHost, fwdFromPort); + if (fwdChannel != NULL + && wolfSSH_ChannelGetId(fwdChannel, &fwdChannelId, + WS_CHANNEL_ID_SELF) != WS_SUCCESS) + fwdChannel = NULL; continue; } if (appBufferUsed > 0) { From 5d37f178af5a2d5a774d049a63377a2e29e1263a Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 28 Aug 2026 15:08:15 -0700 Subject: [PATCH 14/17] Take the worker's status before the EOF drain The drain runs between wolfSSH_worker() and the get_error() that classifies its result, and its reads and sends latch their own status: WS_WINDOW_FULL and WS_WANT_WRITE from a send, WS_REKEYING from a read. The ladder then read the drain's status as the worker's, matched no arm, and ended the session with the backlog unsent and no EOF. - Read the error once, right after the worker returns --- examples/echoserver/echoserver.c | 4 +++- .../ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 482d184df..156ce308d 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -1011,6 +1011,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); + /* Take the worker's status before the drain below: its + * reads and sends latch their own into ssh->error. */ + rc = wolfSSH_get_error(ssh); /* The peer is done sending: hand back the backlog and answer * its EOF, or a client that half-closed waits on a server @@ -1072,7 +1075,6 @@ static int ssh_worker(thread_ctx_t* threadCtx) } if (cnt_r < 0) { - rc = wolfSSH_get_error(ssh); /* wolfSSH_worker() reports WS_REKEYING in place of * WS_CHAN_RXD while a rekey is in flight, and the data * report is never raised again, so drain on both or the 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 6baa1af00..179d02c56 100644 --- a/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c +++ b/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c @@ -995,6 +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); + /* Take the worker's status before the drain below: its + * reads and sends latch their own into ssh->error. */ + rc = wolfSSH_get_error(ssh); /* The peer is done sending: hand back the backlog and answer * its EOF, or a client that half-closed waits on a server @@ -1056,7 +1059,6 @@ static int ssh_worker(thread_ctx_t* threadCtx) } if (cnt_r < 0) { - rc = wolfSSH_get_error(ssh); if (rc == WS_CHAN_RXD) { if (lastChannel == shellChannelId) { cnt_r = wolfSSH_ChannelIdRead(ssh, shellChannelId, From eb61e5bdfecdeaf9602dc0b30d250be5218a4a94 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 28 Aug 2026 15:08:15 -0700 Subject: [PATCH 15/17] Wait on a child that is not taking its stdin Stdin is non-blocking now, so a full pipe leaves an unwritten tail for the next pass. Nothing can come off the channel until it drains, so the channel data that is left unread kept pending set, and pending forced a zero timeout on select(). The loop then polled instead of waiting on the child's stdin, which is already in the write set, and burned a core until the child read. - Take the zero timeout only when the child has no tail owed to it - Add sshd_stdin_stall_test.sh, which fails without this --- apps/wolfsshd/test/run_all_sshd_tests.sh | 1 + apps/wolfsshd/test/sshd_stdin_stall_test.sh | 54 +++++++++++++++++++++ apps/wolfsshd/wolfsshd.c | 7 ++- 3 files changed, 60 insertions(+), 2 deletions(-) create mode 100755 apps/wolfsshd/test/sshd_stdin_stall_test.sh diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index 156d167b2..8f37f2556 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -11,6 +11,7 @@ test_cases=( "sshd_scp_fail.sh" "sshd_term_close_test.sh" "sshd_stdin_eof_test.sh" + "sshd_stdin_stall_test.sh" "ssh_kex_algos.sh" ) diff --git a/apps/wolfsshd/test/sshd_stdin_stall_test.sh b/apps/wolfsshd/test/sshd_stdin_stall_test.sh new file mode 100755 index 000000000..607c8f00a --- /dev/null +++ b/apps/wolfsshd/test/sshd_stdin_stall_test.sh @@ -0,0 +1,54 @@ +#!/bin/bash +# A child that is not reading its stdin must not make the shell loop spin. +# Once the stdin pipe fills, the write returns EAGAIN and the unwritten tail +# is carried to the next pass; nothing can come off the channel until it +# drains, so a pass with data still in hand must wait on the descriptors +# rather than poll. Fails if wolfsshd burns half a core or more while the +# child sleeps on a full pipe. +# +# Needs the OpenSSH client: the wolfSSH example client sends its input and +# then reads, so it never fills the pipe. +HOST="$1" +PORT="$2" +USER_NAME="${3:-`whoami`}" + +command -v ssh >/dev/null 2>&1 || exit 77 +command -v timeout >/dev/null 2>&1 || exit 77 + +KEYDIR=`mktemp -d` || exit 1 +trap 'rm -rf "$KEYDIR"' EXIT +cp ../../../keys/hansel-key-ecc.pem "$KEYDIR/id_ecdsa" || exit 1 +chmod 600 "$KEYDIR/id_ecdsa" + +SSH_OPTS=(-i "$KEYDIR/id_ecdsa" -p "$PORT" + -o IdentitiesOnly=yes + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null + -o PreferredAuthentications=publickey -o PasswordAuthentication=no + -o BatchMode=yes -o ConnectTimeout=5 -o LogLevel=ERROR) + +timeout 20 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" true >/dev/null 2>&1 || exit 77 + +dd if=/dev/zero of="$KEYDIR/big" bs=1M count=8 2>/dev/null + +cpu_ticks() { + local t=0 p f + for p in `pgrep -x wolfsshd`; do + read -r -a f < /proc/$p/stat 2>/dev/null || continue + t=$(( t + ${f[13]} + ${f[14]} )) + done + echo $t +} + +timeout 40 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" 'sleep 12' \ + < "$KEYDIR/big" >/dev/null 2>&1 & +SSHPID=$! +sleep 3 +A=`cpu_ticks` +sleep 6 +B=`cpu_ticks` +wait $SSHPID + +HZ=`getconf CLK_TCK` +PCT=$(( (B - A) * 100 / (6 * HZ) )) +echo "wolfsshd CPU over the 6s window: ${PCT}% ($((B - A)) ticks)" +[ "$PCT" -lt 50 ] diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 0df71032d..6e75cdbee 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1987,8 +1987,11 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, struct timeval* timeout = NULL; /* Work already in hand must not wait on the descriptors, but the - * poll still runs so this pass sees the child's output too. */ - if (pending) { + * poll still runs so this pass sees the child's output too. Data + * the child has not taken yet is not in hand: nothing can be + * pulled off the channel until it drains, so a zero timeout + * would spin. Its stdin is in the write set, so wait there. */ + if (pending && childInIdx == childInSz) { noWait.tv_sec = 0; noWait.tv_usec = 0; timeout = &noWait; From 209449458931019a85aade1a968419727129ed3f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 28 Aug 2026 15:14:21 -0700 Subject: [PATCH 16/17] Tighten the stdin-stall bound A correct loop measures zero and a spinning one saturates a core, so half a core left room for a partial spin to pass. The shorter window costs the suite nothing, since the child's sleep set the runtime. - Bound the reading at a tenth of a core over three seconds - Name the settle, window, sleep and limit rather than spelling each out --- apps/wolfsshd/test/sshd_stdin_stall_test.sh | 26 ++++++++++++++------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/apps/wolfsshd/test/sshd_stdin_stall_test.sh b/apps/wolfsshd/test/sshd_stdin_stall_test.sh index 607c8f00a..e301bd9c2 100755 --- a/apps/wolfsshd/test/sshd_stdin_stall_test.sh +++ b/apps/wolfsshd/test/sshd_stdin_stall_test.sh @@ -3,8 +3,9 @@ # Once the stdin pipe fills, the write returns EAGAIN and the unwritten tail # is carried to the next pass; nothing can come off the channel until it # drains, so a pass with data still in hand must wait on the descriptors -# rather than poll. Fails if wolfsshd burns half a core or more while the -# child sleeps on a full pipe. +# rather than poll. A correct loop uses no measurable CPU while the child +# sleeps on a full pipe and a spinning one saturates a core, so the bound is +# set well below a partial spin rather than just below a full one. # # Needs the OpenSSH client: the wolfSSH example client sends its input and # then reads, so it never fills the pipe. @@ -39,16 +40,25 @@ cpu_ticks() { echo $t } -timeout 40 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" 'sleep 12' \ +# Let the transfer reach the stall before measuring, and leave the child +# asleep past the end of the window. Contention can only push the reading +# down, so a loaded machine misses a regression rather than failing a good +# build. +SETTLE=3 +WINDOW=3 +CHILD_SLEEP=9 +LIMIT=10 + +timeout 40 ssh "${SSH_OPTS[@]}" "$USER_NAME@$HOST" "sleep $CHILD_SLEEP" \ < "$KEYDIR/big" >/dev/null 2>&1 & SSHPID=$! -sleep 3 +sleep "$SETTLE" A=`cpu_ticks` -sleep 6 +sleep "$WINDOW" B=`cpu_ticks` wait $SSHPID HZ=`getconf CLK_TCK` -PCT=$(( (B - A) * 100 / (6 * HZ) )) -echo "wolfsshd CPU over the 6s window: ${PCT}% ($((B - A)) ticks)" -[ "$PCT" -lt 50 ] +PCT=$(( (B - A) * 100 / (WINDOW * HZ) )) +echo "wolfsshd CPU over the ${WINDOW}s window: ${PCT}% ($((B - A)) ticks)" +[ "$PCT" -lt "$LIMIT" ] From 05e8aa7b90f72504ab53711d7685cccab235a057 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 28 Aug 2026 15:56:59 -0700 Subject: [PATCH 17/17] echoserver: keep the EOF reply owed across a rekey wolfSSH_ChannelIdRead() has no rekey guard, so a drained channel still reports zero mid-rekey and the drain loop calls the reply in. That send returns WS_REKEYING before it prepares a packet, so nothing is queued. - take the send's status instead of discarding it - latch eofAnswered and ChildRunning on every status but WS_REKEYING, so the reply is retried on a later pass; the KEX traffic wakes it - a short send is left latching: it bundled the EOF and set eofTxd, so a retry queues nothing and the loop would stall in an untimed select waiting on a peer that has already half-closed - same change in the Espressif copy --- examples/echoserver/echoserver.c | 13 ++++++++++--- .../examples/wolfssh_echoserver/main/echoserver.c | 13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 156ce308d..8da868b33 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -1067,9 +1067,16 @@ static int ssh_worker(thread_ctx_t* threadCtx) /* Only an emptied channel earns the EOF; anything * else is retried on a later pass. */ if (eofDrained) { - wolfSSH_ChannelSendEof(eofChannel); - eofAnswered = 1; - ChildRunning = 0; + int eofRet; + + eofRet = wolfSSH_ChannelSendEof(eofChannel); + /* A rekey queues nothing, so the reply is still + * owed and the KEX traffic wakes the next pass. + * A short send already bundled it. */ + if (eofRet != WS_REKEYING) { + eofAnswered = 1; + ChildRunning = 0; + } } } } 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 179d02c56..835cf670b 100644 --- a/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c +++ b/ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c @@ -1051,9 +1051,16 @@ static int ssh_worker(thread_ctx_t* threadCtx) /* Only an emptied channel earns the EOF; anything * else is retried on a later pass. */ if (eofDrained) { - wolfSSH_ChannelSendEof(eofChannel); - eofAnswered = 1; - ChildRunning = 0; + int eofRet; + + eofRet = wolfSSH_ChannelSendEof(eofChannel); + /* A rekey queues nothing, so the reply is still + * owed and the KEX traffic wakes the next pass. + * A short send already bundled it. */ + if (eofRet != WS_REKEYING) { + eofAnswered = 1; + ChildRunning = 0; + } } } }