Skip to content

ssh.c: don't fail a channel read whose window credit is deferred #1192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
+234 −9
Open

ssh.c: don't fail a channel read whose window credit is deferred #1192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions src/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
}


Expand Down
198 changes: 198 additions & 0 deletions tests/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Comment thread
yosuke-wolfssl marked this conversation as resolved.
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; }
Comment thread
yosuke-wolfssl marked this conversation as resolved.
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. */
Expand Down Expand Up @@ -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"));
Expand Down
3 changes: 3 additions & 0 deletions wolfssh/ssh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading