Window flow control for extended data

ejohnstown · ejohnstown · commit f97de5798105 · 2026-06-23T17:39:03.000-07:00
- Charge the channel window on receipt; replenish only when the
  application consumes bytes via wolfSSH_extended_data_read.
- Track extDataChannelId so the read path adjusts the right window.
- Reject extended data larger than the advertised window.
- stderr now shares the channel window with normal data (RFC 4254
  section 5.2 back-pressure).
- Gate wolfSSH_extended_data_read on isKeying: a window adjust is a
  connection-layer message IsMessageAllowed does not gate, so the read
  must not emit one between KEXINIT and NEWKEYS (RFC 4253 section 7.1).
- Fail safe when a second channel sends stderr while another still has
  unread data: drop it and replenish its window instead of leaking the
  displaced channel's window and over-crediting the survivor.
- Document the mandatory-drain contract in the public header.

Issue: F-864
diff --git a/src/internal.c b/src/internal.c
@@ -10211,20 +10211,50 @@ static int DoChannelExtendedData(WOLFSSH* ssh,
             ret = WS_INVALID_CHANID;
         else if (dataSz > channel->maxPacketSz)
             ret = WS_RECV_OVERFLOW_E;
+        else if (dataSz > channel->windowSz)
+            /* Peer sent more than the window we advertised. */
+            ret = WS_RECV_OVERFLOW_E;
         else if (dataTypeCode == CHANNEL_EXTENDED_DATA_STDERR) {
             /* Buffer the stderr data for the application to read. The
              * extended data buffer accumulates unread data instead of
              * overwriting it, so a blob arriving before the application
-             * reads the previous one is no longer silently lost. */
-            ret = PutBuffer(&ssh->extDataBuffer, buf + begin, dataSz);
-            #ifdef DEBUG_WOLFSSH
-            DumpOctetString(buf + begin, dataSz);
-            #endif
-            if (ret == WS_SUCCESS)
+             * reads the previous one is no longer silently lost.
+             *
+             * Extended data consumes channel window the same as ordinary
+             * data (RFC 4254 section 5.2). Charge the window now and only
+             * replenish it once the application drains the data in
+             * wolfSSH_extended_data_read(). That gives stderr real
+             * back-pressure instead of replenishing on receipt, which
+             * previously let the peer keep clobbering the buffer. */
+            int hadPending = ssh->extDataBuffer.idx < ssh->extDataBuffer.length;
+
+            if (hadPending && ssh->extDataChannelId != channel->channel) {
+                /* Buffer and extDataChannelId are shared (single-session
+                 * limitation, see WOLFSSH.extDataBuffer in internal.h), so only
+                 * one channel's stderr buffers at a time. Buffering a second
+                 * would desync windows (displaced channel never credited, this
+                 * one over-credited). Fail safe: drop it and replenish its
+                 * window now so nothing leaks. */
+                WLOG(WS_LOG_ERROR,
+                     "Extended data from channel %u dropped; channel %u still "
+                     "has unread stderr buffered (only one channel's stderr "
+                     "can be buffered at a time)",
+                     channel->channel, ssh->extDataChannelId);
                 ret = SendChannelWindowAdjust(ssh, channel->channel, dataSz);
-            if (ret == WS_SUCCESS) {
-                ssh->lastRxId = channelId;
-                ret = WS_EXTDATA;
+                if (ret == WS_SUCCESS)
+                    ssh->lastRxId = channelId;
+            }
+            else {
+                ret = PutBuffer(&ssh->extDataBuffer, buf + begin, dataSz);
+                #ifdef DEBUG_WOLFSSH
+                DumpOctetString(buf + begin, dataSz);
+                #endif
+                if (ret == WS_SUCCESS) {
+                    channel->windowSz -= dataSz;
+                    ssh->extDataChannelId = channel->channel;
+                    ssh->lastRxId = channelId;
+                    ret = WS_EXTDATA;
+                }
             }
         }
         else {
diff --git a/src/ssh.c b/src/ssh.c
@@ -1404,9 +1404,14 @@ int wolfSSH_extended_data_send(WOLFSSH* ssh, byte* buf, word32 bufSz)
 }
 
 
-/* Reads pending data from extended data buffer. Currently can be used to get
- * STDERR information sent across the channel.
- * Returns the number of bytes read on success */
+/* Reads pending STDERR data from the extended data buffer. Returns bytes read
+ * (>= 0) or a negative WS_ error. See wolfssh/ssh.h for the public contract
+ * (callers MUST drain stderr; WS_REKEYING during key exchange).
+ *
+ * A successful read drains the buffer and sends a window adjust for the bytes
+ * consumed, releasing back-pressure. The read still succeeds if that send
+ * cannot complete: WS_WANT_WRITE is absorbed (adjust stays queued) and a hard
+ * failure is logged, not returned. ssh->error is saved/restored across it. */
 int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz)
 {
     byte*  buf;
@@ -1416,6 +1421,14 @@ int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz)
         return WS_BAD_ARGUMENT;
     }
 
+    /* A read sends a window adjust, which must not go out between KEXINIT and
+     * NEWKEYS. IsMessageAllowed() gates only transport messages on isKeying,
+     * not connection-layer ones, so gate here like wolfSSH_stream_read(). */
+    if (ssh->isKeying) {
+        ssh->error = WS_REKEYING;
+        return WS_REKEYING;
+    }
+
     /* sanity check to make sure idx is not in a bad state */
     if (ssh->extDataBuffer.idx > ssh->extDataBuffer.length) {
         WLOG(WS_LOG_ERROR, "Bad internal state for buffer index");
@@ -1425,6 +1438,48 @@ int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz)
     buf = ssh->extDataBuffer.buffer + ssh->extDataBuffer.idx;
     WMEMCPY(out, buf, bufSz);
     ssh->extDataBuffer.idx += bufSz;
+
+    /* Replenish the channel window for the bytes just consumed. The window
+     * was charged when the extended data arrived (DoChannelExtendedData), so
+     * draining it here is what releases back-pressure to the peer. If the
+     * channel is gone the data is still returned; there is just no window to
+     * adjust. */
+    if (bufSz > 0) {
+        WOLFSSH_CHANNEL* channel;
+
+        channel = ChannelFind(ssh, ssh->extDataChannelId, WS_CHANNEL_ID_SELF);
+        if (channel != NULL) {
+            int adjustResult;
+            int savedError = ssh->error;
+
+            /* Credit the window locally regardless of send result, mirroring
+             * _UpdateChannelWindow(). On WS_WANT_WRITE the adjust is already
+             * bundled into outputBuffer and the peer is credited on a later
+             * flush; on a hard failure the channel is already failing, so a
+             * transient local over-advertisement is moot and crediting avoids
+             * permanently leaking the window for the bytes just consumed.
+             *
+             * The bytes are already in out, so this is a completed read: it
+             * reports the byte count and must not disturb ssh->error. Stomping
+             * ssh->error here would be invisible to callers that only check the
+             * return value and surprising to callers that inspect
+             * wolfSSH_get_error() after a successful read. SendChannelWindow-
+             * Adjust -> wolfSSH_SendPacket sets ssh->error = WS_WANT_WRITE when
+             * the bundled adjust cannot be flushed in non-blocking mode, so
+             * save ssh->error across the send and restore it; a hard send
+             * failure is logged, not returned. */
+            adjustResult = SendChannelWindowAdjust(ssh, channel->channel,
+                                                   bufSz);
+            channel->windowSz += bufSz;
+            ssh->error = savedError;
+            if (adjustResult != WS_SUCCESS && adjustResult != WS_WANT_WRITE)
+                WLOG(WS_LOG_ERROR,
+                     "wolfSSH_extended_data_read: window adjust send failed "
+                     "(%d); window credited locally, read still succeeded",
+                     adjustResult);
+        }
+    }
+
     return bufSz;
 }
 
diff --git a/tests/unit.c b/tests/unit.c
@@ -1769,10 +1769,16 @@ static int test_DoChannelExtendedData_overflow(void)
     return result;
 }
 
-/* Exercises the accumulating extended-data buffer: two stderr blobs that
- * arrive before the application reads must both be preserved (no silent
- * overwrite), and an unknown extended data type must be ignored rather than
- * rejected. */
+/* Exercises the accumulating extended-data buffer and its window
+ * back-pressure: two stderr blobs that arrive before the application reads
+ * must both be preserved (no silent overwrite), the channel window must be
+ * charged on receipt and replenished on read, a partial read followed by an
+ * append must preserve byte ordering and window accounting across the
+ * GrowBuffer compaction, and an unknown extended data type must be ignored
+ * rather than rejected. A final case pins the documented single-session
+ * limitation: stderr arriving on a second channel before the first is drained
+ * is dropped (its window replenished immediately) rather than corrupting the
+ * shared buffer's window accounting. */
 static int test_DoChannelExtendedData_flow(void)
 {
     WOLFSSH_CTX*     ctx = NULL;
@@ -1824,17 +1830,20 @@ static int test_DoChannelExtendedData_flow(void)
         goto done;
     }
 
-    /* First stderr blob: buffered. */
+    /* First stderr blob: buffered, window charged 128 -> 118. */
     idx = 0;
     ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)blob1,
                                             (word32)sizeof(blob1), &idx);
     if (ret != WS_EXTDATA) { result = -604; goto done; }
+    if (ch->windowSz != 118) { result = -605; goto done; }
 
-    /* Second stderr blob before any read: must accumulate, not overwrite. */
+    /* Second stderr blob before any read: must accumulate, not overwrite.
+     * Window charged 118 -> 108. */
     idx = 0;
     ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)blob2,
                                             (word32)sizeof(blob2), &idx);
     if (ret != WS_EXTDATA) { result = -606; goto done; }
+    if (ch->windowSz != 108) { result = -607; goto done; }
 
     /* Read everything: both blobs present and in order (no data loss). */
     ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
@@ -1846,20 +1855,178 @@ static int test_DoChannelExtendedData_flow(void)
         for (i = 10; i < 20; i++)
             if (out[i] != 0x22) { result = -610; goto done; }
     }
+    /* Draining replenishes the window: 108 + 20 -> 128. */
+    if (ch->windowSz != 128) { result = -611; goto done; }
 
     /* Buffer drained; a further read returns 0. */
     ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
     if (ret != 0) { result = -612; goto done; }
 
+    /* Partial-read-then-append: read part of one blob (leaving idx > 0), then
+     * receive another blob. The append forces GrowBuffer to compact a non-zero
+     * idx in place (WMEMMOVE) before adding the new data, and the window must
+     * track a read that is split across the receipt of more data. */
+    idx = 0;
+    ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)blob1,
+                                            (word32)sizeof(blob1), &idx);
+    if (ret != WS_EXTDATA) { result = -630; goto done; }
+    if (ch->windowSz != 118) { result = -631; goto done; }
+
+    /* Read 4 of the 10 buffered bytes: window 118 + 4 -> 122, idx left at 4. */
+    ret = wolfSSH_extended_data_read(ssh, out, 4);
+    if (ret != 4) { result = -632; goto done; }
+    {
+        int i;
+        for (i = 0; i < 4; i++)
+            if (out[i] != 0x11) { result = -633; goto done; }
+    }
+    if (ch->windowSz != 122) { result = -634; goto done; }
+
+    /* Append blob2 with 6 unread bytes still pending (idx=4, length=10): the
+     * 6 remaining 0x11 bytes must be preserved ahead of the new 0x22 bytes.
+     * Window charged 122 -> 112. */
+    idx = 0;
+    ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)blob2,
+                                            (word32)sizeof(blob2), &idx);
+    if (ret != WS_EXTDATA) { result = -635; goto done; }
+    if (ch->windowSz != 112) { result = -636; goto done; }
+
+    /* Read the rest: 6 leftover 0x11 followed by 10 0x22, in order. Window
+     * 112 + 16 -> 128. */
+    ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+    if (ret != 16) { result = -637; goto done; }
+    {
+        int i;
+        for (i = 0; i < 6; i++)
+            if (out[i] != 0x11) { result = -638; goto done; }
+        for (i = 6; i < 16; i++)
+            if (out[i] != 0x22) { result = -639; goto done; }
+    }
+    if (ch->windowSz != 128) { result = -640; goto done; }
+
+    /* Drained again. */
+    ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+    if (ret != 0) { result = -641; goto done; }
+
     /* Unknown extended data type: ignored (consumed), not rejected. Nothing
-     * is buffered for the application. */
+     * is buffered and the window is left intact (replenished on receipt). */
     idx = 0;
     ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)unknownBlob,
                                             (word32)sizeof(unknownBlob), &idx);
     if (ret != WS_SUCCESS) { result = -613; goto done; }
+    if (ch->windowSz != 128) { result = -614; goto done; }
     ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
     if (ret != 0) { result = -615; goto done; }
 
+    /* Window-overflow branch: draw the window down below a blob that is
+     * still <= maxPacketSz, then confirm the next blob is rejected by the
+     * "dataSz > windowSz" branch and not the maxPacketSz branch. dataSz=60
+     * stays under maxPacketSz=64 so the maxPacketSz check cannot fire first. */
+    {
+        byte big[12 + 60];
+        int  i;
+
+        WMEMSET(big, 0, sizeof(big));
+        big[7]  = 0x01;        /* type = stderr */
+        big[11] = 0x3C;        /* dataSz = 60 */
+        for (i = 12; i < (int)sizeof(big); i++)
+            big[i] = 0x44;
+
+        /* window 128 -> 68 */
+        idx = 0;
+        ret = wolfSSH_TestDoChannelExtendedData(ssh, big,
+                                                (word32)sizeof(big), &idx);
+        if (ret != WS_EXTDATA) { result = -616; goto done; }
+        if (ch->windowSz != 68) { result = -617; goto done; }
+
+        /* window 68 -> 8 */
+        idx = 0;
+        ret = wolfSSH_TestDoChannelExtendedData(ssh, big,
+                                                (word32)sizeof(big), &idx);
+        if (ret != WS_EXTDATA) { result = -618; goto done; }
+        if (ch->windowSz != 8) { result = -619; goto done; }
+
+        /* dataSz=60 <= maxPacketSz but > windowSz=8: window-overflow reject,
+         * window left unchanged. */
+        idx = 0;
+        ret = wolfSSH_TestDoChannelExtendedData(ssh, big,
+                                                (word32)sizeof(big), &idx);
+        if (ret != WS_RECV_OVERFLOW_E) { result = -620; goto done; }
+        if (ch->windowSz != 8) { result = -621; goto done; }
+    }
+
+    /* Cross-channel fail safe: the extData buffer and extDataChannelId are
+     * shared across channels, so only one channel's stderr can be buffered at a
+     * time. Stderr arriving on a second channel before the first is drained is
+     * dropped (its window replenished immediately) instead of corrupting the
+     * window accounting. This pins the documented single-session limitation.
+     * First drain the 120 bytes still pending on channel 0 from the overflow
+     * block (8 + 120 -> 128 on read) to reach a clean state. */
+    {
+        WOLFSSH_CHANNEL* chB = NULL;
+        /* channelId=1, type=1 (stderr), dataSz=10, payload all 0x22 */
+        static const byte blobB[] = {
+            0x00, 0x00, 0x00, 0x01,
+            0x00, 0x00, 0x00, 0x01,
+            0x00, 0x00, 0x00, 0x0A,
+            0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22
+        };
+
+        ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+        if (ret != 32) { result = -650; goto done; }
+        ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+        if (ret != 32) { result = -651; goto done; }
+        ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+        if (ret != 32) { result = -652; goto done; }
+        ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+        if (ret != 24) { result = -653; goto done; }
+        ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+        if (ret != 0) { result = -654; goto done; }
+        if (ch->windowSz != 128) { result = -655; goto done; }
+
+        /* Second channel, id 1, same window/packet sizes as channel 0. */
+        chB = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 64);
+        if (chB == NULL) { result = -656; goto done; }
+        if (ChannelAppend(ssh, chB) != WS_SUCCESS) {
+            ChannelDelete(chB, ssh->ctx->heap);
+            result = -657;
+            goto done;
+        }
+
+        /* Channel 0 buffers stderr first: charged 128 -> 118, extDataChannelId
+         * becomes 0, no warning (nothing was pending). */
+        idx = 0;
+        ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)blob1,
+                                                (word32)sizeof(blob1), &idx);
+        if (ret != WS_EXTDATA) { result = -658; goto done; }
+        if (ch->windowSz != 118) { result = -659; goto done; }
+
+        /* Channel 1 sends stderr while channel 0 still has unread data. The
+         * shared single-session buffer can only hold one channel's stderr, so
+         * this blob is dropped (fail safe) rather than corrupting the window
+         * accounting. Its window is replenished immediately (SendChannelWindow-
+         * Adjust returns WS_SUCCESS), so chB stays at 128, the data is not
+         * buffered, and extDataChannelId is left at channel 0. */
+        idx = 0;
+        ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)blobB,
+                                                (word32)sizeof(blobB), &idx);
+        if (ret != WS_SUCCESS) { result = -660; goto done; }
+        if (chB->windowSz != 128) { result = -661; goto done; }
+
+        /* One read drains only channel 0's buffered 10 bytes (channel 1's were
+         * dropped, not buffered) and credits them back to channel 0:
+         * 118 + 10 -> 128. No window is leaked and no channel is over-credited. */
+        ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out));
+        if (ret != 10) { result = -662; goto done; }
+        {
+            int i;
+            for (i = 0; i < 10; i++)
+                if (out[i] != 0x11) { result = -663; goto done; }
+        }
+        if (ch->windowSz != 128) { result = -665; goto done; }
+        if (chB->windowSz != 128) { result = -666; goto done; }
+    }
+
 done:
     wolfSSH_free(ssh);
     wolfSSH_CTX_free(ctx);
diff --git a/wolfssh/internal.h b/wolfssh/internal.h
@@ -874,7 +874,22 @@ struct WOLFSSH {
 
     WOLFSSH_BUFFER inputBuffer;
     WOLFSSH_BUFFER outputBuffer;
-    WOLFSSH_BUFFER extDataBuffer; /* extended data ready to be read */
+    WOLFSSH_BUFFER extDataBuffer; /* extended data ready to be read. Shared
+                                   * across channels, so stderr buffering and
+                                   * its window back-pressure are effectively
+                                   * single-channel: only extDataChannelId is
+                                   * replenished on read. */
+    word32 extDataChannelId;      /* self channel id whose window the buffered
+                                   * extended data was charged against; its
+                                   * window is replenished as the app reads.
+                                   * If a second channel buffers stderr before
+                                   * the app drains the buffer, this is
+                                   * overwritten: the first channel's window is
+                                   * never credited, and on read this channel is
+                                   * over-credited by the first channel's bytes
+                                   * (the whole shared buffer is credited here).
+                                   * Buffered stderr is intended for the
+                                   * single-session case. */
     WC_RNG* rng;
 
     byte h[WC_MAX_DIGEST_SIZE];
diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h
