From ca90ef2d21a4554b59de001c829c900d1123197b Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Tue, 25 Aug 2026 08:42:47 +0900 Subject: [PATCH] ssh.c: report the byte count from a channel read that defers its credit - wolfSSH_stream_read() advances inputBuffer->idx before _UpdateChannelWindow() and returns the byte count, recording a non-success adjust result in ssh->error and logging anything other than WS_WANT_WRITE. - _ChannelRead() returns the bytes copied, records the adjust result the same way, and clears a stale WS_WANT_WRITE when the adjust goes out with the output buffer drained. - The wolfSSH_stream_read() block comment and a new note above its wolfssh/ssh.h declaration state that wolfSSH_get_error() carries the window-adjust status on a successful read. - tests/unit.c adds test_stream_read_deferredWindowAdjust() and test_ChannelIdRead_deferredWindowAdjust(), each reading a full window through an IO send that reports WS_CBIO_ERR_WANT_WRITE and then through one that fails, checking the byte count, the payload, ssh->error, the window credit and the drained input buffer. - The wolfSSH_ChannelIdRead() test ends on a credit that sends cleanly, asserting ssh->error returns to WS_SUCCESS and no credit stays owed. --- src/ssh.c | 42 ++++++++--- tests/unit.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 3 + 3 files changed, 234 insertions(+), 9 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index 1f3f7cc49..f02070008 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1215,9 +1215,9 @@ static int _ChannelReadExt(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz); * the SSH connection. This function handles low level operations in addition to * the read, such as window adjustment and high water checking. * - * In non blocking mode use the function wolfSSH_get_error(ssh) to check for - * WS_WANT_READ / WS_WANT_WRITE after a fail case was hit with - * wolfSSH_stream_read(). + * In non blocking mode check wolfSSH_get_error(ssh) after the read: it holds + * WS_WANT_READ / WS_WANT_WRITE for a fail case, and for a success the status + * of a window adjust that could not be sent. * * Returns the number of bytes read on success, negative values on fail */ @@ -1288,11 +1288,17 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) ret = WS_BUFFER_E; else { WMEMCPY(buf, inputBuffer->buffer + inputBuffer->idx, n); + inputBuffer->idx += n; ret = _UpdateChannelWindow(ssh->channelList); - if (ret == WS_SUCCESS) { - inputBuffer->idx += n; - ret = n; + if (ret != WS_SUCCESS) { + ssh->error = ret; + if (ret != WS_WANT_WRITE) { + WLOG(WS_LOG_ERROR, + "wolfSSH_stream_read: window adjust send failed " + "(%d); read still succeeded", ret); + } } + ret = n; } } @@ -3800,24 +3806,42 @@ static int _UpdateChannelWindow(WOLFSSH_CHANNEL* channel) } +/* Drains buffered channel data and credits the window for the bytes taken. + * Always reports the bytes copied */ static int _ChannelRead(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz) { WOLFSSH_BUFFER* inputBuffer; + WOLFSSH* ssh; int updateResult = WS_SUCCESS; + int savedError; if (channel == NULL || buf == NULL || bufSz == 0) return WS_BAD_ARGUMENT; + ssh = channel->ssh; inputBuffer = &channel->inputBuffer; bufSz = min(bufSz, inputBuffer->length - inputBuffer->idx); WMEMCPY(buf, inputBuffer->buffer + inputBuffer->idx, bufSz); inputBuffer->idx += bufSz; + savedError = ssh->error; updateResult = _UpdateChannelWindow(channel); - if (updateResult == WS_SUCCESS) - updateResult = bufSz; + if (updateResult == WS_SUCCESS) { + if (savedError == WS_WANT_WRITE && ssh->outputBuffer.length == 0) + ssh->error = WS_SUCCESS; + } + else { + /* SendPacket() sets ssh->error only for WS_WANT_WRITE, so hard + * failures must be recorded here or they stay hidden. */ + ssh->error = updateResult; + if (updateResult != WS_WANT_WRITE) { + WLOG(WS_LOG_ERROR, + "_ChannelRead: window adjust send failed (%d); read still " + "succeeded", updateResult); + } + } - return updateResult; + return (int)bufSz; } diff --git a/tests/unit.c b/tests/unit.c index e1eff261c..0656f57e5 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -6063,6 +6063,192 @@ static int test_SendChannelData_zeroPeerMaxPacket(void) return result; } +#ifndef NO_WOLFSSH_SERVER + +/* wolfSSH_stream_read() counterpart of test_ChannelExtDataCreditWantWrite(): + * a deferred credit must not cost the caller the bytes already consumed, and + * an adjust that fails outright must still surface on wolfSSH_get_error(). */ +static int test_stream_read_deferredWindowAdjust(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte in[64]; + byte out[64]; + word32 i; + + for (i = 0; i < (word32)sizeof(in); i++) { + in[i] = (byte)i; + } + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -6980; + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -6981; goto done; } + /* Allow MSGID_CHANNEL_WINDOW_ADJUST on this bare session. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + /* A window the size of the payload, so draining it in one read leaves + * windowSz at zero and _UpdateChannelWindow() has to credit. */ + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + (word32)sizeof(in), DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -6982; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -6983; + goto done; + } + ch->openConfirmed = 1; + + if (wolfSSH_TestChannelPutData(ch, in, (word32)sizeof(in)) != WS_SUCCESS) { + result = -6984; goto done; + } + if (ch->windowSz != 0) { result = -6985; goto done; } + + /* Every byte is reported, and they are the bytes that were put. */ + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret != (int)sizeof(in)) { result = -6990; goto done; } + if (WMEMCMP(out, in, sizeof(in)) != 0) { result = -6991; goto done; } + + /* The deferral is observable, the window is credited locally, and the + * bytes are consumed rather than left for a re-read. */ + if (ssh->error != WS_WANT_WRITE) { result = -6992; goto done; } + if (ch->windowSz != (word32)sizeof(in)) { result = -6993; goto done; } + if (ch->inputBuffer.length - ch->inputBuffer.idx != 0) { + result = -6994; goto done; + } + + /* A peer that reset rather than blocked. wolfSSH_SendPacket() records only + * WS_WANT_WRITE, so the read path has to record a hard failure itself. */ + wolfSSH_SetIOSend(ctx, FailIoSend); + + if (wolfSSH_TestChannelPutData(ch, in, (word32)sizeof(in)) != WS_SUCCESS) { + result = -6995; goto done; + } + if (ch->windowSz != 0) { result = -6996; goto done; } + + ret = wolfSSH_stream_read(ssh, out, (word32)sizeof(out)); + if (ret != (int)sizeof(in)) { result = -6997; goto done; } + if (ssh->error != WS_SOCKET_ERROR_E) { result = -6998; goto done; } + /* The send discarded what it bundled, so the credit stays owed. */ + if (ch->pendingWindowAdjust != (word32)sizeof(in)) { + result = -6999; goto done; + } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* wolfSSH_ChannelIdRead() counterpart of + * test_stream_read_deferredWindowAdjust(): callers break out on a non-positive + * read, and this entry point has to retire the owed-flush status itself. */ +static int test_ChannelIdRead_deferredWindowAdjust(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte in[64]; + byte out[64]; + word32 i; + + for (i = 0; i < (word32)sizeof(in); i++) { + in[i] = (byte)i; + } + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -7010; + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -7011; goto done; } + /* Allow MSGID_CHANNEL_WINDOW_ADJUST on this bare session. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + /* A window the size of the payload, so draining it in one read leaves + * windowSz at zero and _UpdateChannelWindow() has to credit. */ + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + (word32)sizeof(in), DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -7012; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -7013; + goto done; + } + ch->openConfirmed = 1; + + if (wolfSSH_TestChannelPutData(ch, in, (word32)sizeof(in)) != WS_SUCCESS) { + result = -7014; goto done; + } + if (ch->windowSz != 0) { result = -7015; goto done; } + + /* Unlike wolfSSH_stream_read(), this entry point does not clear the error, + * so seed it: the assert below has to prove the read recorded it. */ + ssh->error = WS_SUCCESS; + + /* Every byte is reported, and they are the bytes that were put. */ + ret = wolfSSH_ChannelIdRead(ssh, ch->channel, out, (word32)sizeof(out)); + if (ret != (int)sizeof(in)) { result = -7016; goto done; } + if (WMEMCMP(out, in, sizeof(in)) != 0) { result = -7017; goto done; } + + /* The deferral is observable, the window is credited locally, and the + * bytes are consumed rather than left for a re-read. */ + if (ssh->error != WS_WANT_WRITE) { result = -7018; goto done; } + if (ch->windowSz != (word32)sizeof(in)) { result = -7019; goto done; } + if (ch->inputBuffer.length - ch->inputBuffer.idx != 0) { + result = -7020; goto done; + } + + /* A peer that reset rather than blocked. wolfSSH_SendPacket() records only + * WS_WANT_WRITE, so the read path has to record a hard failure itself. */ + wolfSSH_SetIOSend(ctx, FailIoSend); + + if (wolfSSH_TestChannelPutData(ch, in, (word32)sizeof(in)) != WS_SUCCESS) { + result = -7021; goto done; + } + if (ch->windowSz != 0) { result = -7022; goto done; } + + ret = wolfSSH_ChannelIdRead(ssh, ch->channel, out, (word32)sizeof(out)); + if (ret != (int)sizeof(in)) { result = -7023; goto done; } + if (ssh->error != WS_SOCKET_ERROR_E) { result = -7024; goto done; } + /* The send discarded what it bundled, so the credit stays owed. */ + if (ch->pendingWindowAdjust != (word32)sizeof(in)) { + result = -7025; goto done; + } + + /* This entry point never resets ssh->error, so a credit that does go out + * has to retire the owed-flush status itself. */ + wolfSSH_SetIOSend(ctx, DiscardIoSend); + ssh->error = WS_WANT_WRITE; + + if (wolfSSH_TestChannelPutData(ch, in, (word32)sizeof(in)) != WS_SUCCESS) { + result = -7026; goto done; + } + + ret = wolfSSH_ChannelIdRead(ssh, ch->channel, out, (word32)sizeof(out)); + if (ret != (int)sizeof(in)) { result = -7027; goto done; } + if (ssh->outputBuffer.length != 0) { result = -7028; goto done; } + if (ssh->error != WS_SUCCESS) { result = -7029; goto done; } + /* Both the parked credit and the new one reached the peer. */ + if (ch->pendingWindowAdjust != 0) { result = -7030; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +#endif /* NO_WOLFSSH_SERVER */ + /* BuildNameList() returns a C string. On an empty id list it must still * terminate the buffer: SendKexInit() measures the result with WSTRLEN * through AlgoListSz() and copies that many bytes into the KEXINIT. */ @@ -16400,6 +16586,18 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; +#ifndef NO_WOLFSSH_SERVER + unitResult = test_stream_read_deferredWindowAdjust(); + printf("stream_read_deferredWindowAdjust: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_ChannelIdRead_deferredWindowAdjust(); + printf("ChannelIdRead_deferredWindowAdjust: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif /* NO_WOLFSSH_SERVER */ + unitResult = test_BuildNameList_emptySrc(); printf("BuildNameList_emptySrc: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index dfb45ab60..a46833ef2 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -559,6 +559,9 @@ WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); +/* Returns the bytes read. On a non-blocking socket a read can succeed with its + * window adjust to the peer still unsent: check wolfSSH_get_error(), and on + * WS_WANT_WRITE flush with wolfSSH_worker() or the channel stalls. */ WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_exit(WOLFSSH* ssh, int status);