ssh, internal: always flush the worker's output

- wolfSSH_worker() calls wolfSSH_SendPacket() whenever
  ssh->outputBuffer holds bytes and the session is not
  disconnected, in place of doing so only for WS_SUCCESS,
  WS_WANT_READ, WS_CHAN_RXD or WS_EOF. ssh->error keeps the
  receive's code when the receive failed, and the close's when a
  WS_CHANNEL_CLOSED pass hard-failed its flush; a function-scope
  sendRet also masks the WS_REKEYING report. Drops the second
  DoReceive(), its WS_WINDOW_FULL case, the WOLFSSH_TEST_BLOCK
  fork, and the separate WS_CHANNEL_CLOSED flush.
- BundlePacket() resets ssh->outputBuffer.length to
  ssh->packetStartIdx when the framing fails, and
  wolfSSH_shutdown() reports WS_WANT_WRITE when its read for the
  peer's close leaves output queued.
- SendPacketFlush() records its code in ssh->error on every
  transport failure path; wolfSSH_SendPacket() says so and what a
  later write to that field owes it.
- wolfssh/ssh.h drops WS_WINDOW_FULL from wolfSSH_worker() and
  names the two exceptions to a status surviving a failed flush:
  a hard-failed WS_CHANNEL_CLOSED, and WS_REKEYING. Comments in
  ssh.c, unit.c, the echoserver, the Espressif copy and portfwd
  name the channel's own state or the failing call instead of
  restating a contract.
- Sixteen unit tests and the extended TestWorkerReportsDisconnect
  cover what ret and ssh->error hold after a receive, send,
  buffer, callback or framing failure, against channel data,
  extended data, a half-close, a rekey, a close, and a teardown
  read. The #ifndef WOLFSSH_TEST_BLOCK guards around
  TestWorkerReadsWhenSendWouldBlock go with the send-first fork.

Author: yosuke-wolfssl

examples/echoserver/echoserver.c

@@ -1016,12 +1016,9 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                 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
-                 * 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. */
+                 * its EOF, since the library no longer answers for us. Off
+                 * the channel's own state, not the once-only WS_EOF status.
+                 * Echo mode only; a shell child on a pty still produces. */
                 if (!eofAnswered && echoOnly) {
                     WOLFSSH_CHANNEL* eofChannel;
 

examples/portfwd/portfwd.c

@@ -781,10 +781,9 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args)
 
             /* 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. */
+             * state, not the once-only WS_EOF status. Only the channel appFd
+             * is wired to: half-closing the wrong socket truncates a live
+             * transfer. */
             if (appFdSet && fwdChannel != NULL && !appFdHalfClosed
                     && wolfSSH_ChannelGetEof(fwdChannel)) {
                 int drained;

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

@@ -1000,12 +1000,9 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                 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
-                 * 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. */
+                 * its EOF, since the library no longer answers for us. Off
+                 * the channel's own state, not the once-only WS_EOF status.
+                 * Echo mode only; a shell child on a pty still produces. */
                 if (!eofAnswered && echoOnly) {
                     WOLFSSH_CHANNEL* eofChannel;
 

src/internal.c

@@ -5142,6 +5142,7 @@ static int SendPacketFlush(WOLFSSH* ssh)
 
     if (ssh->ctx->ioSendCb == NULL) {
         WLOG(WS_LOG_DEBUG, "Your IO Send callback is null, please set");
+        ssh->error = WS_SOCKET_ERROR_E;
         return WS_SOCKET_ERROR_E;
     }
 
@@ -5152,6 +5153,7 @@ static int SendPacketFlush(WOLFSSH* ssh)
         if (ssh->outputBuffer.length > ssh->outputBuffer.bufferSz ||
                 ssh->outputBuffer.length < ssh->outputBuffer.idx) {
             WLOG(WS_LOG_ERROR, "Bad buffer state");
+            ssh->error = WS_BUFFER_E;
             return WS_BUFFER_E;
         }
 
@@ -5190,11 +5192,13 @@ static int SendPacketFlush(WOLFSSH* ssh)
                     ssh->outputBuffer.plainSz = 0;
                     ShrinkBuffer(&ssh->outputBuffer, 1);
             }
+            ssh->error = WS_SOCKET_ERROR_E;
             return WS_SOCKET_ERROR_E;
         }
 
         if ((word32)sent > ssh->outputBuffer.length) {
             WLOG(WS_LOG_DEBUG, "wolfSSH_SendPacket() out of bounds read");
+            ssh->error = WS_SEND_OOB_READ_E;
             return WS_SEND_OOB_READ_E;
         }
 
@@ -5219,7 +5223,9 @@ static int SendPacketFlush(WOLFSSH* ssh)
 }
 
 
-/* returns WS_SUCCESS on success */
+/* returns WS_SUCCESS on success. Transport failures record their code in
+ * ssh->error, so a later write to that field on the same pass has to be
+ * conditional on this having succeeded, or it hides the dead transport. */
 int wolfSSH_SendPacket(WOLFSSH* ssh)
 {
     int ret;
@@ -14364,6 +14370,10 @@ static int BundlePacket(WOLFSSH* ssh)
     }
     else {
         WLOG(WS_LOG_DEBUG, "BP: failed to encrypt buffer");
+        if (ssh != NULL) {
+            /* Drop the aborted packet */
+            ssh->outputBuffer.length = ssh->packetStartIdx;
+        }
     }
 
     return ret;

src/ssh.c

@@ -1252,6 +1252,9 @@ int wolfSSH_shutdown(WOLFSSH* ssh)
             /* received response */
             ret = WS_SUCCESS;
         }
+        /* A reply queued during that read has not gone out yet. */
+        if (ret == WS_SUCCESS && wolfSSH_OutputPending(ssh))
+            ret = WS_WANT_WRITE;
     }
 
     if (ssh != NULL && ssh->channelList == NULL) {
@@ -3734,6 +3737,7 @@ const char* wolfSSH_GetSessionCommand(const WOLFSSH* ssh)
 int wolfSSH_worker(WOLFSSH* ssh, word32* channelId)
 {
     int ret = WS_SUCCESS;
+    int sendRet = WS_SUCCESS;
 
     WLOG(WS_LOG_DEBUG, "Entering wolfSSH_worker()");
 
@@ -3750,59 +3754,30 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId)
         return WS_FATAL_ERROR;
     }
 
-#ifdef WOLFSSH_TEST_BLOCK
-    /* In forced non-blocking test mode, keep legacy ordering (send before
-     * receive) to match the harness expectations and avoid synthetic spins. */
-    if (ret == WS_SUCCESS) {
-        if (ssh->outputBuffer.length != 0)
-            ret = wolfSSH_SendPacket(ssh);
-    }
-    if (ret == WS_SUCCESS)
-        ret = DoReceive(ssh);
-#else
     /* Always service inbound data first so window updates can unblock sends. */
     if (ret == WS_SUCCESS) {
         ret = DoReceive(ssh);
     }
 
-    /* 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
-            || 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. 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
-                    || recv2 == WS_EOF)
-                ret = sendRet;
-            else
-                ret = recv2;
-        }
-        else {
-            /* Preserve meaningful receive status when send succeeded. */
-            if (sendRet != WS_SUCCESS)
+    /* Flush queued output whatever DoReceive() made of the socket, since an
+     * idle receive reports WS_FATAL_ERROR. !ssh->disconnected gates it. */
+    if (ssh != NULL && !ssh->disconnected && ssh->outputBuffer.length != 0) {
+        int rxErr = ssh->error;
+
+        sendRet = wolfSSH_SendPacket(ssh);
+        if (sendRet != WS_SUCCESS) {
+            if (ret == WS_SUCCESS) {
                 ret = sendRet;
-            /* else leave ret as prior receive result (SUCCESS/WANT_READ/CHAN_RXD). */
+            }
+            else if ((ret == WS_CHANNEL_CLOSED && sendRet != WS_WANT_WRITE)
+                    || (ret == WS_FATAL_ERROR && rxErr != WS_WANT_READ)) {
+                /* A failed receive outranks the flush, and so does a close
+                 * whose flush hard-failed: callers route teardown on it.
+                 * Every other status keeps the code the send set. */
+                ssh->error = rxErr;
+            }
         }
     }
-#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. */
@@ -3812,12 +3787,10 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId)
             *channelId = ssh->lastRxId;
         }
 
-        /* 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) {
+        /* Report the rekey, unless it would hide a once-only WS_EXTDATA
+         * or WS_EOF, or the error from a flush that failed. */
+        if (ssh->isKeying && ret != WS_EXTDATA && ret != WS_EOF
+                && sendRet == WS_SUCCESS) {
             ssh->error = WS_REKEYING;
             return WS_REKEYING;
         }
@@ -4255,8 +4228,9 @@ static int _ChannelRead(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz)
         }
     }
     else {
-        /* SendPacket() records only WS_WANT_WRITE, so a hard failure has to
-         * be recorded here; rewriting WS_WANT_WRITE is deliberate. */
+        /* The adjust can fail before it reaches the transport, so the code
+         * is recorded here; the log skips a WS_WANT_WRITE, which only asks
+         * for a retry. */
         ssh->error = updateResult;
         if (updateResult != WS_WANT_WRITE) {
             WLOG(WS_LOG_ERROR,
@@ -4316,8 +4290,9 @@ static int _ChannelReadExt(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz)
                 ssh->error = savedError;
         }
         else {
-            /* SendPacket() sets ssh->error only for WS_WANT_WRITE, so hard
-             * failures must be recorded here or they stay hidden. */
+            /* The adjust can fail before it reaches the transport, so the
+             * code is recorded here; the log skips a WS_WANT_WRITE, which
+             * only asks for a retry. */
             ssh->error = adjustResult;
             if (adjustResult != WS_WANT_WRITE) {
                 WLOG(WS_LOG_ERROR,

tests/regress.c

@@ -8277,11 +8277,19 @@ static void TestWorkerReportsDisconnect(void)
     wolfSSH_SetIOReadCtx(ssh, &io);
     wolfSSH_SetIOWriteCtx(ssh, &io);
 
+    /* Queued output, so the flush on this pass has something to push. */
+    ssh->outputBuffer.length = 1;
+    ssh->outputBuffer.idx = 0;
+    ssh->outputBuffer.buffer[0] = 0;
+
     AssertIntEQ(wolfSSH_worker(ssh, NULL), WS_FATAL_ERROR);
     AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT);
     AssertTrue(ssh->disconnected);
     AssertTrue(ssh->isKeying != 0);
-    io.outSz = 0;
+
+    /* The queued byte stays put: the session ended on this very pass. */
+    AssertIntEQ(io.outSz, 0);
+    AssertTrue(wolfSSH_OutputPending(ssh));
 
     /* The message behind it is still queued, and every further pass reports
      * the disconnect rather than the WS_SUCCESS of a skipped dispatch or the
@@ -8954,7 +8962,6 @@ static void TestPasswordEofNoCrash(void)
  * still needs to service Receive() so window-adjusts can arrive and
  * unblock the flow control. Verify the receive callback is invoked even
  * when the first send attempt would block. */
-#ifndef WOLFSSH_TEST_BLOCK
 static int recvCallCount;
 
 static int WantWriteSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
@@ -8970,7 +8977,6 @@ static int WantReadRecv(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
     return WS_CBIO_ERR_WANT_READ;
 }
 
-#ifndef WOLFSSH_TEST_BLOCK
 static void TestWorkerReadsWhenSendWouldBlock(void)
 {
     WOLFSSH_CTX* ctx;
@@ -9005,8 +9011,6 @@ static void TestWorkerReadsWhenSendWouldBlock(void)
     wolfSSH_free(ssh);
     wolfSSH_CTX_free(ctx);
 }
-#endif /* !WOLFSSH_TEST_BLOCK */
-#endif
 
 
 #ifdef WOLFSSH_SFTP
@@ -13303,9 +13307,7 @@ int main(int argc, char** argv)
     TestClientBuffersIdempotent();
 #endif
     TestPasswordEofNoCrash();
-#ifndef WOLFSSH_TEST_BLOCK
     TestWorkerReadsWhenSendWouldBlock();
-#endif
 
 #ifdef KEXDH_REPLY_REGRESS_KEX_ALGO
     #ifndef WOLFSSH_NO_RSA_SHA2_256