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. A failed flush reaches the
  return only when the receive reported nothing. 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. Drops the second
  DoReceive(), its WS_WINDOW_FULL case, the WOLFSSH_TEST_BLOCK
  fork, and the separate WS_CHANNEL_CLOSED flush.
- sendRet moves to the top of wolfSSH_worker(), and the
  WS_REKEYING report is skipped when the flush failed.
- SendPacketFlush() records its code in ssh->error on every
  transport failure path, and wolfSSH_SendPacket() says so. The
  comments on _ChannelRead(), _ChannelReadExt() and
  test_ChannelReadExtHardFailureReported() drop the former
  contract.
- The echoserver and Espressif shell loops and the Windows
  wolfsshd shell loop treat a WS_WANT_WRITE from wolfSSH_worker()
  as non-fatal, sftp_worker()'s handshake flush retry waits on
  tcp_select(), and wolfSSH_stream_read()'s note drops the
  receive-succeeded precondition.
- tests cover the flush on an idle receive, the owed flush across
  calls, what ssh->error holds after a receive failure, a hard
  send failure, channel data alongside a failed send with and
  without a rekey, a discarded buffer, an out-of-bounds send and a
  close whose flush hard-failed, and queued output staying unsent
  on the disconnect pass.

Author: yosuke-wolfssl

apps/wolfsshd/wolfsshd.c

@@ -1361,7 +1361,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
                          * peer. Both want fixing where they can be tested. */
                         continue;
                     }
-                    else if (rc != WS_WANT_READ) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) {
                         break;
                     }
                 }

examples/echoserver/echoserver.c

@@ -1205,7 +1205,7 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                          * above, which has already run this pass. */
                         continue;
                     }
-                    else if (rc != WS_WANT_READ) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) {
                         #ifdef SHELL_DEBUG
                             printf("Break:read sshFd returns %d: errno =%x\n",
                                     cnt_r, errno);
@@ -1495,8 +1495,11 @@ static int sftp_worker(thread_ctx_t* threadCtx)
     ret = error = wolfSSH_get_error(ssh);
 
     /* there is an edge case where the last SFTP handshake message sent got a
-     * WANT_WRITE case, keep trying to send it here. */
+     * WANT_WRITE case, keep trying to send it here. tcp_select() watches the
+     * read side only, so this paces the retry, it does not wait to write. */
     while (error == WS_WANT_WRITE) {
+        if (tcp_select(s, TEST_SFTP_TIMEOUT_SHORT) == WS_SELECT_ERROR_READY)
+            break;
         ret = wolfSSH_worker(ssh, NULL);
         error = wolfSSH_get_error(ssh);
     }

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

@@ -1170,7 +1170,7 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                          * above, which has already run this pass. */
                         continue;
                     }
-                    else if (rc != WS_WANT_READ) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE) {
                         #ifdef SHELL_DEBUG
                             printf("Break:read sshFd returns %d: errno =%x\n",
                                     cnt_r, errno);

src/internal.c

@@ -4955,6 +4955,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;
     }
 
@@ -4965,6 +4966,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;
         }
 
@@ -5003,11 +5005,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;
         }
 
@@ -5032,7 +5036,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;

src/ssh.c

@@ -3704,6 +3704,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()");
 
@@ -3720,59 +3721,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. */
@@ -3782,12 +3754,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;
         }
@@ -4225,8 +4195,8 @@ 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 window adjust failed. ssh->error carries the code, and 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,
@@ -4286,8 +4256,8 @@ 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 window adjust failed. ssh->error carries the code, and
+             * 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

@@ -6770,11 +6770,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