Fix the EOF retry loop and the drain guards

Findings from the second review round on the EOF and close changes.

- wolfSSH_stream_read() held inputBuffer across the new EOF retry. When
  DoChannelClose() retired the head inside DoReceive() and the next head
  already had its EOF latched and drained, the override forced WS_EOF
  while lastRxId still named the channel that had gone, so the retry
  looped forever on freed memory. Both messages that get there are
  peer-controlled. Capture the head's id and only loop while it is
  unchanged.
- The EOF drain guards read a peek of WS_REKEYING as a drained channel.
  wolfSSH_stream_peek() reports it before it looks at the buffer, and
  wolfSSH_worker() no longer masks WS_EOF during a rekey, so this is
  reachable. Distinguish it in wolfsshd and both echoservers.
- wolfsshd closed the child's stdin without checking that the buffered
  channel data had been handed over. The read above it is skipped while
  windowFull, so a later read would write to a closed pipe.
- DoChannelClose() reported WS_WANT_WRITE and left lastRxId unset when
  the flush was back-pressured, costing the caller the graceful-close
  signal for a channel that is already closed and already bundled. The
  pending flush belongs to the output buffer, not the channel.
- test_SendEofApi's idempotence check could not fail: DiscardIoSend
  flushes everything, so the output buffer length was 0 either way.
  Count what reaches the transport instead.

Add coverage for the retry loop and the deferred flush; both fail
without the fix.

Author: ejohnstown

apps/wolfsshd/wolfsshd.c

@@ -934,9 +934,15 @@ static int SFTP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
                     continue;
                 }
 
-                /* Drain what is buffered before leaving on the EOF. */
-                if (error == WS_EOF && wolfSSH_stream_peek(ssh, NULL, 1) <= 0) {
-                    break;
+                /* Drain what is buffered before leaving on the EOF. A rekey
+                 * is not a drained channel: peek reports WS_REKEYING before
+                 * it ever looks at the buffer. */
+                if (error == WS_EOF) {
+                    int peekRet = wolfSSH_stream_peek(ssh, NULL, 1);
+
+                    if (peekRet != WS_REKEYING && peekRet <= 0) {
+                        break;
+                    }
                 }
                 if (ret != WS_SUCCESS && ret != WS_CHAN_RXD
                         && ret != WS_EOF) {
@@ -1967,13 +1973,24 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
                 }
             }
 
-            /* Peer done sending: close the child's stdin. */
+            /* Peer done sending: close the child's stdin, but only once
+             * what it already sent has been handed over. The read above is
+             * skipped while windowFull, so data can still be buffered here;
+             * closing early drops it and the next write lands on fd -1.
+             * peek reports the head channel, which is the shell channel, and
+             * reports WS_REKEYING without looking at the buffer at all. */
             if (stdinPipe[1] != -1 && (!ptyReq || forcedCmd)) {
+                int peekRet = wolfSSH_stream_peek(ssh, NULL, 1);
                 WOLFSSH_CHANNEL* current;
 
                 current = wolfSSH_ChannelFind(ssh, shellChannelId,
                     WS_CHANNEL_ID_SELF);
-                if (current != NULL && wolfSSH_ChannelGetEof(current)) {
+                /* A channel that is gone reports EOF: wolfSSH_ChannelGetEof()
+                 * returns 1 for NULL by design, and DoChannelClose() can have
+                 * retired it already. Testing for NULL here would leave the
+                 * child waiting on a stdin that never closes. */
+                if (wolfSSH_ChannelGetEof(current)
+                        && peekRet != WS_REKEYING && peekRet <= 0) {
                     /* SSH is done, close stdin pipe to child process */
                     close(stdinPipe[1]);
                     stdinPipe[1] = -1;

examples/echoserver/echoserver.c

@@ -1096,6 +1096,14 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                         continue;
                     }
                     else if (rc == WS_EOF) {
+                        /* The peer has finished sending. Do NOT answer with an
+                         * EOF of our own here: that latches eofTxd and
+                         * SendChannelData() then rejects every later send, so
+                         * the child's remaining output would be dropped. That
+                         * auto-echo is the bug this half-close support exists
+                         * to remove. An application sends its own EOF with
+                         * wolfSSH_ChannelSendEof() once it has finished
+                         * producing output for the channel. */
                         continue;
                     }
                     else if (rc != WS_WANT_READ) {
@@ -1438,8 +1446,15 @@ static int sftp_worker(thread_ctx_t* threadCtx)
             }
 
             /* Drain what is buffered before leaving on the EOF. */
-            if (error == WS_EOF && wolfSSH_stream_peek(ssh, NULL, 1) <= 0) {
-                break;
+            if (error == WS_EOF) {
+                /* 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 && ret != WS_EOF) {
             #ifdef WOLFSSH_TEST_BLOCK
@@ -1477,8 +1492,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) {
@@ -1648,6 +1665,15 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs)
             ret = 0;
         }
 
+        /* The channel was already retired by the peer's close, which is a
+         * completed shutdown rather than a failure. Without this any session
+         * whose channel is gone before the shutdown call leaves ret at
+         * WS_CHANNEL_CLOSED, and the non-zero return sets quit, taking the
+         * whole 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)) {

ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c

@@ -1083,6 +1083,14 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                         continue;
                     }
                     else if (rc == WS_EOF) {
+                        /* The peer has finished sending. Do NOT answer with an
+                         * EOF of our own here: that latches eofTxd and
+                         * SendChannelData() then rejects every later send, so
+                         * the child's remaining output would be dropped. That
+                         * auto-echo is the bug this half-close support exists
+                         * to remove. An application sends its own EOF with
+                         * wolfSSH_ChannelSendEof() once it has finished
+                         * producing output for the channel. */
                         continue;
                     }
                     else if (rc != WS_WANT_READ) {
@@ -1409,8 +1417,15 @@ static int sftp_worker(thread_ctx_t* threadCtx)
             }
 
             /* Drain what is buffered before leaving on the EOF. */
-            if (error == WS_EOF && wolfSSH_stream_peek(ssh, NULL, 1) <= 0) {
-                break;
+            if (error == WS_EOF) {
+                /* 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 && ret != WS_EOF) {
                 if (ret == WS_WANT_WRITE) {
@@ -1435,8 +1450,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) {
@@ -1578,6 +1595,15 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs)
             ret = 0;
         }
 
+        /* The channel was already retired by the peer's close, which is a
+         * completed shutdown rather than a failure. Without this any session
+         * whose channel is gone before the shutdown call leaves ret at
+         * WS_CHANNEL_CLOSED, and the non-zero return sets quit, taking the
+         * whole 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)) {

src/internal.c

@@ -11331,11 +11331,14 @@ static int DoChannelClose(WOLFSSH* ssh,
 
         if (removeRet != WS_SUCCESS)
             ret = removeRet;
-    }
-
-    if (ret == WS_SUCCESS) {
-        ret = WS_CHANNEL_CLOSED;
-        ssh->lastRxId = channelId;
+        else {
+            /* The channel is closed and named either way. A back-pressured
+             * flush is a property of the output buffer, not of the channel,
+             * so reporting WS_WANT_WRITE here would cost the caller its
+             * graceful-close signal and leave it to time the teardown out. */
+            ret = WS_CHANNEL_CLOSED;
+            ssh->lastRxId = channelId;
+        }
     }
 
     WLOG(WS_LOG_DEBUG, "Leaving DoChannelClose(), ret = %d", ret);

src/ssh.c

@@ -1230,13 +1230,16 @@ 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()");
 
     if (ssh == NULL || buf == NULL || bufSz == 0 || ssh->channelList == NULL)
         return WS_BAD_ARGUMENT;
 
     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
@@ -1269,12 +1272,15 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz)
                            - ssh->channelList->inputBuffer.idx == 0))
                 ret = WS_EOF;
             if (ret == WS_EOF && ssh->channelList != NULL
-                    && ssh->lastRxId != ssh->channelList->channel) {
+                    && ssh->channelList->channel == headId
+                    && ssh->lastRxId != headId) {
                 /* An EOF on another channel is not this read's EOF: the head
                  * is still open and still has nothing buffered. Reporting it
                  * would be indistinguishable from the head's own EOF, so keep
                  * waiting. Multi-channel apps see it on the channel EOF
-                 * callback or as WS_EOF from wolfSSH_worker(). */
+                 * callback or as WS_EOF from wolfSSH_worker(). Only loop while
+                 * the head is unchanged: DoChannelClose() can free it, and
+                 * inputBuffer still points into it. */
                 continue;
             }
             if (ret == WS_EXTDATA &&