channel: half-close on the peer's EOF, add a public send-EOF API - #1195
channel: half-close on the peer's EOF, add a public send-EOF API#1195ejohnstown wants to merge 14 commits into
Conversation
0d7c135 to
4b89fae
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1195
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1195
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
3684cc8 to
c47f622
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1195
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
c47f622 to
c665f02
Compare
c665f02 to
2b8e647
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1195
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1195
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1195
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
The contract in ssh.h claimed more than the code does. A CHANNEL_EOF already received outranks the drain, so both stream reads report WS_EOF with data still buffered, and wolfSSH_accept()/wolfSSH_connect() never look at the flag at all. Both fixes belong with the channel EOF work in wolfSSL#1195; until then the header says what is really gated. Issue: F-8837
The contract in ssh.h claimed more than the code does. A CHANNEL_EOF already received outranks the drain, so both stream reads report WS_EOF with data still buffered, and wolfSSH_accept()/wolfSSH_connect() never look at the flag at all. Both fixes belong with the channel EOF work in #1195; until then the header says what is really gated. Issue: F-8837
1dc7abd to
41d1e52
Compare
wolfSSH_accept() and wolfSSH_connect() drive the handshake only while the session is live. Both gate on SendAfterDisconnect() ahead of the pending-send block, which would otherwise flush a queued disconnect and count it as the next handshake message; the shutdown paths own that flush. - The prototype sits ahead of both drivers, since either can be the only one built. - TestDisconnectGatesAccept() and TestDisconnectGatesConnect() cover a local disconnect, one from the peer, and a queued short send. One test per endpoint, so a single-sided build keeps the coverage that applies to it. - A received disconnect used to reach the error-state test in wolfSSH_accept() and report WS_INVALID_STATE_E; the gate answers WS_FATAL_ERROR first, and both tests pin that. - The contract comments in ssh.h and internal.h drop the ungated note.
b9dcbe8 to
0ab1756
Compare
DoPacket() skips the whole message dispatch once ssh->disconnected is set, for every message but a DISCONNECT. The handlers that answer must not -- a close draws an EOF and a close of ours, a request a success or failure, an open a confirmation, an unknown message an UNIMPLEMENTED -- and what the rest would record is of no use to a caller that can no longer send. RFC 4253 section 11.1. - Inbound data from here on is dropped rather than buffered, so the read path hands back only what arrived before the disconnect. ssh.h and internal.h say so, beside the calls and beside the flag. - A DISCONNECT still reaches DoDisconnect(), which sends nothing and is what latches WS_DISCONNECT; SendDisconnect() sets the flag too, so ours can be the one that raised it. - The frame advance steps over the whole packet, so the stream stays in step with no payload bookkeeping of its own. - wolfSSH_worker() gates on SendAfterDisconnect() the way wolfSSH_accept() and wolfSSH_connect() do. With the dispatch skipped there is no non-success left to return, so it would answer a healthy session for as long as the peer kept talking and the SFTP and SCP drive loops would keep pumping a dead one. The rekey test below the gate needs no disconnect of its own: the gate returns first, and a DISCONNECT arriving mid-pass leaves ret fatal. wolfSSH_shutdown() drops the channel on a disconnect before its own pump, so the gate does not cost it the read it does there. - tests/regress.c pins all four: no reply goes out, the stream stays in step with a disconnect queued behind a skipped close, late channel data is dropped rather than queued, and the worker reports the disconnect on the pass that takes it and on every pass behind it.
SHELL_Subsystem() hands the child whatever the peer sent, whichever pass it arrived on, and closes the write end of its stdin only once that buffer is dry. It works off the shell channel's own inputBuffer, so data held back while the window was full is still handed over; the old read ran only on the worker's WS_CHAN_RXD and was skipped outright while windowFull. - The channel id comes from the head of the channel list at entry, the only channel open there, rather than from DEFAULT_NEXT_CHANNEL, which a build can override. - A lookup that finds nothing is not an EOF: only a channel that is present and drained closes the pipe. - Data arriving behind the peer's EOF, which RFC 4254 section 5.3 forbids, is dropped rather than written to a stdin that is already closed. The write would fail with EBADF and end the session mid-stream. - The short-write retry tests for a -1 return before reading errno, which nothing else sets.
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.
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
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.
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.
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
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.
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
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.
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.
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.
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.
0ab1756 to
b9cfa44
Compare
DoChannelEof()answered a peer's half-close with an EOF of its own. That latchedeofTxd, after which the "Cannot send data after EOF" gates inSendChannelData()andSendChannelExtendedData()refused every later send, so a peer that half-closed could never be replied to.ssh host 'sort' < filereturns nothing on master today: wolfSSHd closes the child's stdin correctly and then cannot send the command's output back. RFC 4254 section 5.3 leaves that reply to the application, and no application could make it -- everySendChannelEof()caller bundled the EOF with the teardown, so there was no way to close one direction and keep reading.The library
DoChannelEof()latcheseofRxdand reportsWS_EOFinstead of echoing.wolfSSH_worker()carries the status out with the channel it belongs to and does not mask it during a rekey, andDoReceiveHandshake()absorbs it in the three accept/connect states that can run with a channel already open: a legal EOF is not a handshake failure (F-1687)wolfSSH_stream_read()andwolfSSH_stream_peek()hand back what is buffered before they report the EOF. TheeofRxdtest moved behind the buffered-data test, so a half-close no longer strands what arrived with it.stream_read()takes the head channel id beforeDoReceive()and keeps waiting when the EOF belongs to another channel, and loops only while the head is unchanged, sinceDoChannelClose()can free itwolfSSH_ChannelSendEof()andwolfSSH_stream_send_eof()are the missing half of the RFC's half-close: they close the sending direction and leave the receiving direction open. Both refuse a NULL session, a channel whose open is unconfirmed, a disconnected session and a packet between KEXINIT and NEWKEYSSendChannelEof()commitseofTxdonce the EOF is in the output buffer, not only once the flush reports success, so the retry an application makes onWS_WANT_WRITEdoes not queue a second EOF behind the first. The one exception is the send failure that discards the buffer (F-8826)SendChannelClose()dropscloseTxdon that same discard. AcloseTxdclaiming a close that never left madewolfSSH_shutdown()'s gate skip the teardown. It is a separate commit from the one above on purpose: that latch widens and this one narrows, and read as one change they read wrongwolfSSH_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 the worker reportsWS_CHANNEL_CLOSED. Removing it locally freed the pointer under the caller and left the peer's close matching nothing (F-8839)DoChannelClose()andwolfSSH_ChannelExit()answer only a channel whose open the peer has confirmed.peerChannelis 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 sessionwolfSSH_SFTP_buffer_read()also stops spending a receive on a peek that has already reported a hang-upThe callers
The last commit is required by the library change, not tidying. All three worker ladders in wolfSSHd and the echoservers end in
else if (rc != WS_WANT_READ) break, and wolfSSHd's reacheskill(childPid, SIGKILL), so landing the library change alone ships green and kills the command on every half-close. wolfSSHd now closes the child's stdin off the channel's own EOF state; the echoservers hand back the backlog before answering with an EOF of their own; the clients treat a half-close as the graceful case;portfwdrelays it to the local socket withshutdown(SHUT_WR)and is the working demonstration of the new API.Tests
tests/unit.ccovers the half-close end to end, the channel id an EOF is reported against, the rekey case, thechannelEofCbcallback -- which had no test anywhere in the tree -- a handshake that survives an EOF, both latch arms throughFailIoSend, and the unconfirmed-channel refusal on both send-EOF entry points.apps/wolfsshd/test/sshd_stdin_eof_test.shis the end-to-end case against a live daemon; it cannot pass on master. The shared receive mock and packet builders are hoisted ahead of both endpoint regions first, in a relocation-only commit that reads with--color-moved.Merging
This branch is a linear stack. GitHub shows 13 commits and 19 files because the first three are #1211 and #1212; the last ten are this PR. #1212 must land first, since the wolfSSHd changes here are written on top of its drain. #1211 is preferred first as well -- the only code coupling is one adjacency in
src/ssh.c, but landing this first would briefly double the post-disconnect chatter that #1211 removes. Merge order: #1211, #1212, this, then #1148.The four commits behind F-8826, F-8839, the
closeTxdlatch and the unconfirmed-channel teardown fix master today and could each have been a same-day PR; worth knowing if anyone needs a backport.Three things are deliberately left out rather than widening this further:
inputBufferbecausewolfSSH_stream_peek()goes blind onceeofRxdis set, and it lands ahead of this branch, so the API cannot come from herewolfSSH_worker()'s changed return contractexamples/client, which is a feature rather than a fixCross-PR: #1188's unlanded half rewrites
echoserver'sssh_worker()with hunks that bracket this one's insertion point. They auto-merge today, but whoever lands second should confirm the EOF arm survived -- if it is lost the SIGKILL comes back and ships green.The review threads below predate a re-cut of the branch and are stale anchors, not open questions. Each one carried its resolution and each fix is in the commit that now owns it; the two Fenrir comments on
wolfSSH_stream_send_eof()are the unconfirmed-channel commit.Issue: F-1687, F-8826, F-8839