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 and
  what a later write to that field owes it.
- wolfssh/ssh.h states the rule on wolfSSH_worker(): a status
  survives in the return while wolfSSH_get_error() holds the
  flush's code, except that a WS_CHANNEL_CLOSED whose flush failed
  hard keeps the close. It drops WS_WINDOW_FULL, which the worker
  can no longer return. The echoserver, Espressif and portfwd
  comments now name the channel's own state rather than restating
  the worker's contract, and four comments in ssh.c and unit.c
  drop the former one.
- The #ifndef WOLFSSH_TEST_BLOCK guards around
  TestWorkerReadsWhenSendWouldBlock go. The send-first fork they
  worked around is gone, so it runs in every configuration.
- 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 rekey the flush did not disturb, a discarded
  buffer, an out-of-bounds send, a missing send callback, a bad
  buffer state and a close whose flush hard-failed, and queued
  output staying unsent on the disconnect pass.

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

@@ -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

@@ -3707,6 +3707,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()");
 
@@ -3723,59 +3724,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. */
@@ -3785,12 +3757,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;
         }
@@ -4228,8 +4198,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,
@@ -4289,8 +4260,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

@@ -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
@@ -7447,7 +7455,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)
@@ -7463,7 +7470,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;
@@ -7498,8 +7504,6 @@ static void TestWorkerReadsWhenSendWouldBlock(void)
     wolfSSH_free(ssh);
     wolfSSH_CTX_free(ctx);
 }
-#endif /* !WOLFSSH_TEST_BLOCK */
-#endif
 
 
 #ifdef WOLFSSH_SFTP
@@ -11192,9 +11196,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