Skip to content

Fix window handling for extended data and add a test#936

Closed
yosuke-wolfssl wants to merge 2 commits intowolfSSL:masterfrom
yosuke-wolfssl:f_864
Closed

Fix window handling for extended data and add a test#936
yosuke-wolfssl wants to merge 2 commits intowolfSSL:masterfrom
yosuke-wolfssl:f_864

Conversation

@yosuke-wolfssl
Copy link
Copy Markdown
Contributor

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.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Apr 20, 2026
Copilot AI review requested due to automatic review settings April 20, 2026 04:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 decrement channel->windowSz after buffering extended data instead of immediately sending CHANNEL_WINDOW_ADJUST.
  • Update wolfSSH_extended_data_read() to send CHANNEL_WINDOW_ADJUST and 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.

Comment thread src/internal.c
ret = WS_INVALID_CHANID;
else if (dataSz > channel->windowSz) {
WLOG(WS_LOG_ERROR, "Internal state error, too much data");
ret = WS_RECV_OVERFLOW_E;
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
ret = WS_RECV_OVERFLOW_E;
ret = WS_FATAL_ERROR;

Copilot uses AI. Check for mistakes.
Comment thread tests/api.c
* 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)
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
!defined(NO_FILESYSTEM)
!defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM)

Copilot uses AI. Check for mistakes.
Comment thread src/ssh.c
Comment on lines +1332 to +1333
SendChannelWindowAdjust(ssh, ssh->channelList->channel, bufSz);
ssh->channelList->windowSz += bufSz;
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
}

Copilot uses AI. Check for mistakes.
Comment thread src/ssh.c
Comment on lines +1331 to +1334
if (bufSz > 0 && ssh->channelList != NULL) {
SendChannelWindowAdjust(ssh, ssh->channelList->channel, bufSz);
ssh->channelList->windowSz += bufSz;
}
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment thread src/internal.c
Comment on lines +9850 to 9856
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;
}
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@yosuke-wolfssl yosuke-wolfssl deleted the f_864 branch April 20, 2026 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants