Fix window handling for extended data and add a test#936
Fix window handling for extended data and add a test#936yosuke-wolfssl wants to merge 2 commits intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts SSH extended-data (stderr) flow-control handling so the receive window is decremented when SSH_MSG_CHANNEL_EXTENDED_DATA is processed and replenished later when the application consumes the buffered extended data. It also adds an API-level test to exercise the extended-data window behavior end-to-end.
Changes:
- Update
DoChannelExtendedData()to decrementchannel->windowSzafter buffering extended data instead of immediately sendingCHANNEL_WINDOW_ADJUST. - Update
wolfSSH_extended_data_read()to sendCHANNEL_WINDOW_ADJUSTand restore the channel window when the application reads extended data. - Add a new threaded client/server test validating the window decrement/restore behavior for extended data.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tests/api.c |
Adds a new threaded test that sends/receives extended data and asserts window decrement/restore behavior. |
src/ssh.c |
Sends CHANNEL_WINDOW_ADJUST from wolfSSH_extended_data_read() and updates local window accounting. |
src/internal.c |
Decrements the channel receive window when extended data is buffered, deferring window adjust. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret = WS_INVALID_CHANID; | ||
| else if (dataSz > channel->windowSz) { | ||
| WLOG(WS_LOG_ERROR, "Internal state error, too much data"); | ||
| ret = WS_RECV_OVERFLOW_E; |
There was a problem hiding this comment.
The new dataSz > channel->windowSz check returns WS_RECV_OVERFLOW_E, while the analogous check for regular data in ChannelPutData() treats this as an internal/protocol-fatal condition (WS_FATAL_ERROR). Since this path logs "Internal state error", consider aligning the error code with ChannelPutData() to keep error semantics consistent for window violations.
| ret = WS_RECV_OVERFLOW_E; | |
| ret = WS_FATAL_ERROR; |
| * Verifies DoChannelExtendedData decrements the channel window and | ||
| * wolfSSH_extended_data_read restores it via SendChannelWindowAdjust. */ | ||
| #if !defined(SINGLE_THREADED) && !defined(NO_WOLFSSH_CLIENT) && \ | ||
| !defined(NO_FILESYSTEM) |
There was a problem hiding this comment.
test_wolfSSH_extended_data() starts a server endpoint (wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER) + wolfSSH_accept), but the compile-time guard only checks !NO_WOLFSSH_CLIENT. This test should also be excluded when NO_WOLFSSH_SERVER is defined (otherwise client-only builds may fail to compile/link or will fail at runtime).
| !defined(NO_FILESYSTEM) | |
| !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM) |
| SendChannelWindowAdjust(ssh, ssh->channelList->channel, bufSz); | ||
| ssh->channelList->windowSz += bufSz; |
There was a problem hiding this comment.
wolfSSH_extended_data_read() assumes ssh->channelList is the channel that produced the pending extended data. In multi-channel scenarios, WS_EXTDATA is tagged with ssh->lastRxId (set in DoChannelExtendedData), but channelList may refer to a different channel; sending WINDOW_ADJUST on the wrong channel can corrupt flow control. Prefer looking up the channel by lastRxId (or storing the extended-data channelId alongside extDataBuffer) and adjusting that specific channel.
| SendChannelWindowAdjust(ssh, ssh->channelList->channel, bufSz); | |
| ssh->channelList->windowSz += bufSz; | |
| if (ssh->channelList->channel == ssh->lastRxId) { | |
| SendChannelWindowAdjust(ssh, ssh->channelList->channel, bufSz); | |
| ssh->channelList->windowSz += bufSz; | |
| } | |
| else { | |
| WLOG(WS_LOG_ERROR, | |
| "Extended data channel mismatch: pending data is for channel %u," | |
| " current channelList head is %u", | |
| ssh->lastRxId, ssh->channelList->channel); | |
| } |
| if (bufSz > 0 && ssh->channelList != NULL) { | ||
| SendChannelWindowAdjust(ssh, ssh->channelList->channel, bufSz); | ||
| ssh->channelList->windowSz += bufSz; | ||
| } |
There was a problem hiding this comment.
wolfSSH_extended_data_read() sends a CHANNEL_WINDOW_ADJUST but ignores the return code. In non-blocking mode SendChannelWindowAdjust() can return WS_WANT_WRITE/WS_WANT_READ (or other errors); currently this function will still return bufSz and unconditionally increment windowSz, which can desync local window accounting and hides I/O errors from the caller. Capture the send result, only increment windowSz when the adjust message was successfully queued/sent, and propagate/set ssh->error on failure (similar to _ChannelRead/_UpdateChannelWindow).
| ret = PutBuffer(&ssh->extDataBuffer, buf + begin, dataSz); | ||
| #ifdef DEBUG_WOLFSSH | ||
| DumpOctetString(buf + begin, dataSz); | ||
| #endif | ||
| if (ret == WS_SUCCESS) { | ||
| ret = SendChannelWindowAdjust(ssh, channel->channel, dataSz); | ||
| channel->windowSz -= dataSz; | ||
| } |
There was a problem hiding this comment.
DoChannelExtendedData() now decrements channel->windowSz after PutBuffer(), but PutBuffer() resets the extended-data buffer (discarding any previously unread extDataBuffer contents). If another SSH_MSG_CHANNEL_EXTENDED_DATA arrives before the application drains the previous data, the older bytes will be dropped while the window remains decremented, potentially stalling the peer (no corresponding window adjust will ever be sent for the discarded bytes). Consider appending to extDataBuffer (or rejecting new ext-data while unread data remains, or restoring the window for any bytes you overwrite) to keep window accounting consistent.
This PR fixes DoChannelExtendedData() so that it deducts dataSz from channel->windowSz after successful PutBuffer and this never calls SendChannelWindowAdjust() immediately.
SendChannelWindowAdjust() would be deferred into wolfSSH_extended_data_read(). This is a same pattern with regular data handling in wolfSSH_stream_read() now.
Also, this PR adds a test to exercise the windows handling for extended data.