diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index 8a5a558f1..4f4807c6c 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -325,21 +325,26 @@ static int FlushQueuedSend(WOLFSSH* ssh, wolfSSL_Mutex* lock) wc_LockMutex(lock); } ret = wolfSSH_worker(ssh, NULL); - if (ret == WS_FATAL_ERROR) { - /* the session holds the detail behind a fatal error */ - ret = wolfSSH_get_error(ssh); - } if (lock != NULL) { wc_UnLockMutex(lock); } - } while (ret == WS_WANT_WRITE && WTIME(NULL) < deadline); - - /* The queue is out. Whatever the worker made of the peer's end of the - * conversation is for the reader to sort out. A rekey started on the way - * through is the reader's as well, the send itself went out. */ - if (ret == WS_WANT_READ || ret == WS_CHAN_RXD || ret == WS_EXTDATA - || ret == WS_REKEYING) { - ret = WS_SUCCESS; + + /* Anything outside the receive's own statuses is a failure. */ + if (ret != WS_SUCCESS && ret != WS_WANT_READ && ret != WS_CHAN_RXD + && ret != WS_EXTDATA && ret != WS_REKEYING) { + break; + } + } while (wolfSSH_get_error(ssh) == WS_WANT_WRITE + && WTIME(NULL) < deadline); + + /* Report only whether the queue went out. The deadline can run out + * with the packet still queued. */ + if (ret == WS_SUCCESS || ret == WS_WANT_READ || ret == WS_CHAN_RXD + || ret == WS_EXTDATA || ret == WS_REKEYING) { + if (wolfSSH_get_error(ssh) == WS_WANT_WRITE) + ret = WS_WANT_WRITE; + else + ret = WS_SUCCESS; } return ret; @@ -1353,9 +1358,6 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) if (ret == WS_SUCCESS) { ret = wolfSSH_worker(ssh, NULL); - if (ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } if (ret == WS_WANT_WRITE) { /* The close messages are already out, whatever the drain * still wants to send is a reply to the peer. */ diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index d86ac426f..5217e98c4 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -2618,20 +2618,17 @@ static void* HandleConnection(void* arg) wolfSSH_Log(WS_LOG_INFO, "[SSHD] Attempting to close down connection"); ret = wolfSSH_shutdown(ssh); - /* peer hung up, stop shutdown */ - if (ret == WS_SOCKET_ERROR_E) { + /* peer hung up or the channel is already gone, stop shutdown */ + if (ret == WS_SOCKET_ERROR_E || ret == WS_CHANNEL_CLOSED) { ret = 0; } - error = wolfSSH_get_error(ssh); - if (error != WS_SOCKET_ERROR_E && - (error == WS_WANT_READ || error == WS_WANT_WRITE)) { + if (ret == WS_WANT_READ || ret == WS_WANT_WRITE) { int maxAttempt = 10; /* make 10 attempts max before giving up */ int attempt; for (attempt = 0; attempt < maxAttempt; attempt++) { ret = wolfSSH_worker(ssh, NULL); - error = wolfSSH_get_error(ssh); /* peer successfully closed down gracefully */ if (ret == WS_CHANNEL_CLOSED) { @@ -2645,9 +2642,11 @@ static void* HandleConnection(void* arg) break; } - if (ret == WS_FATAL_ERROR && - (error != WS_WANT_READ && - error != WS_WANT_WRITE)) { + /* Keep draining while the socket blocks or the peer is still + * talking. Anything else is a failure worth giving up on. */ + if (ret != WS_SUCCESS && ret != WS_WANT_READ && + ret != WS_WANT_WRITE && ret != WS_CHAN_RXD && + ret != WS_EXTDATA && ret != WS_REKEYING) { break; } #ifdef _WIN32 diff --git a/examples/client/client.c b/examples/client/client.c index b397075e6..caa947ecf 100644 --- a/examples/client/client.c +++ b/examples/client/client.c @@ -1209,7 +1209,8 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) } ret = wolfSSH_worker(ssh, NULL); if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E && - ret != WS_CHANNEL_CLOSED) { + ret != WS_CHANNEL_CLOSED && + ret != WS_WANT_READ && ret != WS_WANT_WRITE) { ClientFreeBuffers(pubKeyName, privKeyName, NULL); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); @@ -1226,7 +1227,8 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E && - ret != WS_CHANNEL_CLOSED) { + ret != WS_CHANNEL_CLOSED && + ret != WS_WANT_READ && ret != WS_WANT_WRITE) { err_sys("Closing client stream failed"); } diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 2dd03dc8c..ab2141b95 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -1383,7 +1383,7 @@ static int sftp_worker(thread_ctx_t* threadCtx) } do { - if (ret == WS_WANT_WRITE || ret == WS_CHAN_RXD || + if (ret == WS_CHAN_RXD || error == WS_WANT_WRITE || wolfSSH_SFTP_PendingSend(ssh)) { /* Yes, process the SFTP data. */ ret = wolfSSH_SFTP_read(ssh); @@ -1639,14 +1639,12 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) if (error != WS_SOCKET_ERROR_E && error != WS_FATAL_ERROR) { ret = wolfSSH_shutdown(threadCtx->ssh); - /* peer hung up, stop shutdown */ - if (ret == WS_SOCKET_ERROR_E) { + /* peer hung up or the channel is already gone, stop shutdown */ + if (ret == WS_SOCKET_ERROR_E || ret == WS_CHANNEL_CLOSED) { ret = 0; } - error = wolfSSH_get_error(threadCtx->ssh); - if (error != WS_SOCKET_ERROR_E && - (error == WS_WANT_READ || error == WS_WANT_WRITE)) { + if (ret == WS_WANT_READ || ret == WS_WANT_WRITE) { int maxAttempt = 10; /* make 10 attempts max before giving up */ int attempt; @@ -1681,6 +1679,10 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) } } + /* The report below names how the connection ended, and the shutdown + * drain refreshes error only on the paths that enter it. */ + error = wolfSSH_get_error(threadCtx->ssh); + if (threadCtx->fd != -1) { WCLOSESOCKET(threadCtx->fd); threadCtx->fd = -1; diff --git a/examples/scpclient/scpclient.c b/examples/scpclient/scpclient.c index 8b817ad5f..82f7da881 100644 --- a/examples/scpclient/scpclient.c +++ b/examples/scpclient/scpclient.c @@ -326,7 +326,8 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) } else { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret != WS_CHANNEL_CLOSED) { + if (ret != WS_SUCCESS && ret != WS_CHANNEL_CLOSED && + ret != WS_WANT_READ && ret != WS_WANT_WRITE) { WLOG(WS_LOG_DEBUG, "Failed to listen for close messages from the peer."); } diff --git a/examples/sftpclient/sftpclient.c b/examples/sftpclient/sftpclient.c index f78c3306d..807881b76 100644 --- a/examples/sftpclient/sftpclient.c +++ b/examples/sftpclient/sftpclient.c @@ -661,9 +661,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_Get(ssh, pt, to, resume, &myStatusCb); @@ -772,9 +769,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_Put(ssh, pt, to, resume, &myStatusCb); @@ -866,9 +860,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_STAT(ssh, pt, &atrb); @@ -920,9 +911,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_CHMOD(ssh, path, mode); @@ -987,9 +975,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_Open(ssh, path, WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT | @@ -1003,9 +988,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_Close(ssh, handle, handleSz); err = wolfSSH_get_error(ssh); @@ -1060,9 +1042,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_RMDIR(ssh, pt); @@ -1117,9 +1096,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_Remove(ssh, pt); @@ -1210,9 +1186,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } ret = wolfSSH_SFTP_Rename(ssh, pt, to); @@ -1348,9 +1321,6 @@ static int doCmds(func_args* args) do { while (ret == WS_REKEYING || ssh->error == WS_REKEYING) { ret = wolfSSH_worker(ssh, NULL); - if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(ssh); - } } current = wolfSSH_SFTP_LS(ssh, workingDir); @@ -1821,9 +1791,7 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args) ret = 0; } - err = wolfSSH_get_error(ssh); - if (err != WS_SOCKET_ERROR_E && - (err == WS_WANT_READ || err == WS_WANT_WRITE)) { + if (ret == WS_WANT_READ || ret == WS_WANT_WRITE) { int maxAttempt = 10; /* make 10 attempts max before giving up */ int attempt; diff --git a/src/internal.c b/src/internal.c index 41168633a..547edd6ac 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4344,6 +4344,8 @@ int wolfSSH_OutputPending(WOLFSSH* ssh) } +/* Reports what the read did: WS_SUCCESS, WS_WANT_READ when the socket + * would block, or WS_MEMORY_E / WS_SOCKET_ERROR_E. */ static int GetInputData(WOLFSSH* ssh, word32 size) { int in; @@ -4352,11 +4354,6 @@ static int GetInputData(WOLFSSH* ssh, word32 size) * for what is missing in the request. */ word32 haveDataSz; - /* reset want read state before attempting to read */ - if (ssh->error == WS_WANT_READ) { - ssh->error = 0; - } - haveDataSz = ssh->inputBuffer.length - ssh->inputBuffer.idx; if (haveDataSz >= size) { WLOG(WS_LOG_INFO, "GID: have enough already, return early"); @@ -4368,8 +4365,7 @@ static int GetInputData(WOLFSSH* ssh, word32 size) } if (GrowBuffer(&ssh->inputBuffer, size) < 0) { - ssh->error = WS_MEMORY_E; - return WS_FATAL_ERROR; + return WS_MEMORY_E; } /* read data from network */ @@ -4378,13 +4374,11 @@ static int GetInputData(WOLFSSH* ssh, word32 size) ssh->inputBuffer.buffer + ssh->inputBuffer.length, size); if (in == -1) { - ssh->error = WS_SOCKET_ERROR_E; - return WS_FATAL_ERROR; + return WS_SOCKET_ERROR_E; } if (in == WS_WANT_READ) { - ssh->error = WS_WANT_READ; - return WS_FATAL_ERROR; + return WS_WANT_READ; } if (in >= 0) { @@ -4393,8 +4387,7 @@ static int GetInputData(WOLFSSH* ssh, word32 size) } else { /* all other unexpected negative values is a failure case */ - ssh->error = WS_SOCKET_ERROR_E; - return WS_FATAL_ERROR; + return WS_SOCKET_ERROR_E; } } while (size); @@ -7715,7 +7708,6 @@ int ChannelCreditWindow(WOLFSSH* ssh, WOLFSSH_CHANNEL* channel, word32 amount) static int SendPendingChannelWindowAdjust(WOLFSSH* ssh) { WOLFSSH_CHANNEL* cur; - int savedError; int ret = WS_SUCCESS; if (ssh == NULL) @@ -7725,11 +7717,7 @@ static int SendPendingChannelWindowAdjust(WOLFSSH* ssh) if (ssh->isKeying) return WS_SUCCESS; - /* ssh->error is restored only when every channel flushed cleanly, so a - * back-pressured or broken transport is not hidden by this incidental - * flush. Keep going after a failure: the other channels are independent. */ - savedError = ssh->error; - + /* Keep going after a failure: the other channels are independent. */ for (cur = ssh->channelList; cur != NULL; cur = cur->next) { if (cur->pendingWindowAdjust != 0) { int adjustResult = ChannelCreditWindow(ssh, cur, 0); @@ -7739,9 +7727,7 @@ static int SendPendingChannelWindowAdjust(WOLFSSH* ssh) } } - if (ret == WS_SUCCESS) - ssh->error = savedError; - else + if (ret != WS_SUCCESS) ssh->error = ret; return ret; @@ -12898,8 +12884,10 @@ int DoReceive(WOLFSSH* ssh) case PROCESS_INIT: readSz = peerBlockSz; WLOG(WS_LOG_DEBUG, "PR1: size = %u", readSz); - if ((ret = GetInputData(ssh, readSz)) < 0) { - return ret; + ret = GetInputData(ssh, readSz); + if (ret < 0) { + ssh->error = ret; + return WS_FATAL_ERROR; } ssh->processReplyState = PROCESS_PACKET_LENGTH; @@ -12954,8 +12942,10 @@ int DoReceive(WOLFSSH* ssh) readSz = UINT32_SZ + ssh->curSz + peerMacSz; WLOG(WS_LOG_DEBUG, "PR2: size = %u", readSz); if (readSz > 0) { - if ((ret = GetInputData(ssh, readSz)) < 0) { - return ret; + ret = GetInputData(ssh, readSz); + if (ret < 0) { + ssh->error = ret; + return WS_FATAL_ERROR; } if (!aeadMode) { @@ -20375,6 +20365,7 @@ int SendChannelData(WOLFSSH* ssh, word32 channelId, byte* output; word32 idx; int ret = WS_SUCCESS; + int sendRet = WS_SUCCESS; WOLFSSH_CHANNEL* channel = NULL; WLOG(WS_LOG_DEBUG, "Entering SendChannelData()"); @@ -20400,8 +20391,14 @@ int SendChannelData(WOLFSSH* ssh, word32 channelId, } if (ret == WS_SUCCESS) { - if (ssh->outputBuffer.length != 0) + if (ssh->outputBuffer.length != 0) { ret = wolfSSH_SendPacket(ssh); + + /* The payload is not bundled on this path, so mark the owed + * flush: the next call flushes first and returns WS_WANT_WRITE. */ + if (ret == WS_WANT_WRITE) + ssh->outputBuffer.plainSz = dataSz; + } } if (ret == WS_SUCCESS) { @@ -20475,15 +20472,21 @@ int SendChannelData(WOLFSSH* ssh, word32 channelId, /* at this point the data has been loaded into WOLFSSH structure and is * considered consumed */ - if (ret == WS_SUCCESS) + if (ret == WS_SUCCESS) { ret = wolfSSH_SendPacket(ssh); + sendRet = ret; + + /* The byte count is the return value, so a short write reaches the + * caller through wolfSSH_get_error(). */ + if (sendRet != WS_SUCCESS) + ssh->error = sendRet; + if (sendRet == WS_WANT_WRITE) + ssh->outputBuffer.plainSz = dataSz; + } if (ret == WS_SUCCESS || ret == WS_WANT_WRITE) ret = dataSz; - if (ssh && ssh->error == WS_WANT_WRITE) - ssh->outputBuffer.plainSz = dataSz; - WLOG(WS_LOG_DEBUG, "Leaving SendChannelData(), ret = %d", ret); return ret; } @@ -20495,6 +20498,7 @@ int SendChannelExtendedData(WOLFSSH* ssh, word32 channelId, byte* output; word32 idx; int ret = WS_SUCCESS; + int sendRet = WS_SUCCESS; WOLFSSH_CHANNEL* channel = NULL; WLOG(WS_LOG_DEBUG, "Entering SendChannelData()"); @@ -20520,8 +20524,12 @@ int SendChannelExtendedData(WOLFSSH* ssh, word32 channelId, } if (ret == WS_SUCCESS) { - if (ssh->outputBuffer.length != 0) + if (ssh->outputBuffer.length != 0) { ret = wolfSSH_SendPacket(ssh); + + if (ret == WS_WANT_WRITE) + ssh->outputBuffer.plainSz = dataSz; + } } if (ret == WS_SUCCESS) { @@ -20598,15 +20606,19 @@ int SendChannelExtendedData(WOLFSSH* ssh, word32 channelId, /* at this point the data has been loaded into WOLFSSH structure and is * considered consumed */ - if (ret == WS_SUCCESS) + if (ret == WS_SUCCESS) { ret = wolfSSH_SendPacket(ssh); + sendRet = ret; + + if (sendRet != WS_SUCCESS) + ssh->error = sendRet; + if (sendRet == WS_WANT_WRITE) + ssh->outputBuffer.plainSz = dataSz; + } if (ret == WS_SUCCESS || ret == WS_WANT_WRITE) ret = dataSz; - if (ssh && ssh->error == WS_WANT_WRITE) - ssh->outputBuffer.plainSz = dataSz; - WLOG(WS_LOG_DEBUG, "Leaving SendChannelExtendedData(), ret = %d", ret); return ret; } diff --git a/src/ssh.c b/src/ssh.c index c15ccb612..7839c6444 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1178,8 +1178,7 @@ int wolfSSH_shutdown(WOLFSSH* ssh) } /* continue on success and in case where queueing up send packets */ - if (ret == WS_SUCCESS || - (ret != WS_BAD_ARGUMENT && ssh->error == WS_WANT_WRITE)) { + if (ret == WS_SUCCESS || ret == WS_WANT_WRITE) { ret = SendChannelExit(ssh, channel->peerChannel, #if defined(WOLFSSH_TERM) || defined(WOLFSSH_SHELL) ssh->exitStatus); @@ -1189,8 +1188,7 @@ int wolfSSH_shutdown(WOLFSSH* ssh) } /* continue on success and in case where queueing up send packets */ - if (ret == WS_SUCCESS || - (ret != WS_BAD_ARGUMENT && ssh->error == WS_WANT_WRITE)) + if (ret == WS_SUCCESS || ret == WS_WANT_WRITE) ret = SendChannelClose(ssh, channel->peerChannel); } @@ -3583,66 +3581,41 @@ const char* wolfSSH_GetSessionCommand(const WOLFSSH* ssh) int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) { - int ret = WS_SUCCESS; + int ret = WS_BAD_ARGUMENT; + int sendRet = WS_SUCCESS; WLOG(WS_LOG_DEBUG, "Entering wolfSSH_worker()"); - if (ssh == NULL) - ret = WS_BAD_ARGUMENT; - -#ifdef WOLFSSH_TEST_BLOCK - /* In forced non-blocking test mode, keep legacy ordering (send before - * receive) to match the harness expectations and avoid synthetic spins. */ - if (ret == WS_SUCCESS) { - if (ssh->outputBuffer.length != 0) - ret = wolfSSH_SendPacket(ssh); - } - if (ret == WS_SUCCESS) - ret = DoReceive(ssh); -#else - /* Always service inbound data first so window updates can unblock sends. */ - if (ret == WS_SUCCESS) { + if (ssh != NULL) { ret = DoReceive(ssh); - } - /* If receive only wanted read or delivered channel data, still try to - * flush any pending outbound packets. */ - if (ret == WS_SUCCESS || ret == WS_WANT_READ || ret == WS_CHAN_RXD) { - int sendRet = WS_SUCCESS; + /* DoReceive() reports an idle socket and a hard failure the same way: + * WS_FATAL_ERROR with the detail in ssh->error. */ + if (ret == WS_FATAL_ERROR && ssh->error != WS_SUCCESS) + ret = ssh->error; - if (ssh->outputBuffer.length != 0) + /* Record the channel first. */ + if (channelId != NULL && (ret == WS_SUCCESS || ret == WS_CHAN_RXD || + ret == WS_EXTDATA)) { + *channelId = ssh->lastRxId; + } + + /* Send remaining bytes anyway */ + if (ssh->outputBuffer.length != 0) { sendRet = wolfSSH_SendPacket(ssh); - /* If send is back-pressured, immediately try another receive to pick - * up potential window-adjusts and then return the send status. */ - if (sendRet == WS_WANT_WRITE || sendRet == WS_WINDOW_FULL) { - int recv2 = DoReceive(ssh); - if (recv2 == WS_SUCCESS || recv2 == WS_WANT_READ || recv2 == WS_CHAN_RXD) - ret = sendRet; - else - ret = recv2; - } - else { - /* Preserve meaningful receive status when send succeeded. */ - if (sendRet != WS_SUCCESS) + /* Store any failure except for WS_WANT_WRITE */ + if (sendRet != WS_SUCCESS && sendRet != WS_WANT_WRITE) { + ssh->error = sendRet; ret = sendRet; - /* else leave ret as prior receive result (SUCCESS/WANT_READ/CHAN_RXD). */ - } - } -#endif /* WOLFSSH_TEST_BLOCK */ - - /* WS_EXTDATA reports the channel too, so a multi-channel caller can route - * the drain to wolfSSH_ChannelIdReadExt(). */ - if (ret == WS_SUCCESS || ret == WS_CHAN_RXD || ret == WS_EXTDATA) { - if (channelId != NULL) { - *channelId = ssh->lastRxId; + } } - /* WS_EXTDATA is raised once, on arrival; masking it would strand the - * buffered stderr and its window credit. */ - if (ssh->isKeying && ret != WS_EXTDATA) { + /* WS_EXTDATA is not folded into WS_REKEYING. It is reported once, + * when the data arrives */ + if (ssh->isKeying && (ret == WS_SUCCESS || ret == WS_CHAN_RXD)) { ssh->error = WS_REKEYING; - return WS_REKEYING; + ret = WS_REKEYING; } } @@ -4013,9 +3986,7 @@ static int _ChannelRead(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz) { WOLFSSH_BUFFER* inputBuffer; WOLFSSH* ssh; - word32 creditedSz; int updateResult = WS_SUCCESS; - int savedError; if (channel == NULL || buf == NULL || bufSz == 0) return WS_BAD_ARGUMENT; @@ -4033,23 +4004,8 @@ static int _ChannelRead(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz) inputBuffer->idx += bufSz; /* Unguarded by bufSz: also compacts, and carries credit left behind. */ - savedError = ssh->error; - creditedSz = inputBuffer->idx; updateResult = _UpdateChannelWindow(channel); - if (updateResult == WS_SUCCESS) { - /* Clear the old WS_WANT_WRITE only if this read sent an adjust of - * its own and the output buffer is now empty. */ - if (savedError == WS_WANT_WRITE && creditedSz != 0 - && inputBuffer->idx == 0 && ssh->outputBuffer.length == 0) { - ssh->error = WS_SUCCESS; - } - else { - ssh->error = savedError; - } - } - else { - /* SendPacket() records only WS_WANT_WRITE, so a hard failure has to - * be recorded here; rewriting WS_WANT_WRITE is deliberate. */ + if (updateResult != WS_SUCCESS) { ssh->error = updateResult; if (updateResult != WS_WANT_WRITE) { WLOG(WS_LOG_ERROR, @@ -4090,7 +4046,6 @@ static int _ChannelReadExt(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz) if (bufSz > 0) { int adjustResult; - int savedError = ssh->error; /* Credit locally regardless of the send result; ChannelCreditWindow() * owns getting it to the peer. */ @@ -4101,16 +4056,7 @@ static int _ChannelReadExt(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz) ShrinkBuffer(extDataBuffer, 0); adjustResult = ChannelCreditWindow(ssh, channel, bufSz); - if (adjustResult == WS_SUCCESS) { - /* Don't restore an owed-flush status once the buffer has drained. */ - if (savedError == WS_WANT_WRITE && ssh->outputBuffer.length == 0) - ssh->error = WS_SUCCESS; - else - ssh->error = savedError; - } - else { - /* SendPacket() sets ssh->error only for WS_WANT_WRITE, so hard - * failures must be recorded here or they stay hidden. */ + if (adjustResult != WS_SUCCESS) { ssh->error = adjustResult; if (adjustResult != WS_WANT_WRITE) { WLOG(WS_LOG_ERROR, diff --git a/tests/regress.c b/tests/regress.c index 76ff4618a..fe82c995b 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4480,13 +4480,13 @@ static void TestWorkerReadsWhenSendWouldBlock(void) recvCallCount = 0; - /* call worker; expect it to attempt send, notice back-pressure, and have - * invoked recv once. Depending on how DoReceive handles WANT_READ, the - * return may be WANT_WRITE or a fatal error; the important part is that - * recv was exercised. */ + /* The worker attempts the send, notices the back-pressure and reports + * what the receive did. The short write stays in ssh->error. */ ret = wolfSSH_worker(ssh, NULL); - AssertTrue(ret == WS_WANT_WRITE || ret == WS_FATAL_ERROR); + AssertIntEQ(ret, WS_WANT_READ); + AssertIntEQ(wolfSSH_get_error(ssh), WS_WANT_WRITE); + AssertTrue(wolfSSH_OutputPending(ssh)); AssertIntEQ(recvCallCount, 1); wolfSSH_free(ssh); diff --git a/tests/unit.c b/tests/unit.c index 489bc04bf..f9392be35 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -6133,6 +6133,277 @@ static int test_WorkerReportsExtDataChannelKeying(void) return result; } +/* channelId=0, type=1 (stderr), dataSz=10, payload all 0x44. */ +static const byte s_workerExtBlob[] = { + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x0A, + 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44 +}; + +/* Bundles a window adjust into ssh->outputBuffer with the send blocked, + * then leaves the receive idle. */ +static int WorkerParkAdjust(WOLFSSH_CTX* ctx, WOLFSSH* ssh, + WOLFSSH_CHANNEL* channel) +{ + word32 idx = 0; + int ret; + byte out[32]; + + ret = wolfSSH_TestDoChannelExtendedData(ssh, (byte*)s_workerExtBlob, + (word32)sizeof(s_workerExtBlob), + &idx); + if (ret != WS_EXTDATA) + return WS_FATAL_ERROR; + if (channel->windowSz != 118) + return WS_FATAL_ERROR; + + wolfSSH_SetIOSend(ctx, WantWriteIoSend); + + ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out)); + if (ret != 10) + return WS_FATAL_ERROR; + if (ssh->outputBuffer.length == 0) + return WS_FATAL_ERROR; + if (channel->pendingWindowAdjust != 0) + return WS_FATAL_ERROR; + + /* No staged packet, so PacketIoRecv reports want-read. */ + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + + return WS_SUCCESS; +} + +/* A window adjust a short write left in ssh->outputBuffer goes out on a + * later wolfSSH_worker() call. The peer is out of window and sends nothing + * until it arrives, so the receive stays idle. */ +static int test_WorkerFlushesOnIdleReceive(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1530; + wolfSSH_SetIOSend(ctx, CountIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1531; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 64); + if (ch == NULL) { result = -1532; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1533; + goto done; + } + + if (WorkerParkAdjust(ctx, ssh, ch) != WS_SUCCESS) { + result = -1534; + goto done; + } + + /* The socket takes writes again. */ + wolfSSH_SetIOSend(ctx, CountIoSend); + s_extSendCount = 0; + + ret = wolfSSH_worker(ssh, NULL); + + if (s_extSendCount != 1) { result = -1535; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1536; goto done; } + + /* Nothing left queued, so the call reports the receive. */ + if (ret != WS_WANT_READ) { result = -1537; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_READ) { result = -1538; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* wolfSSH_worker() keeps an owed flush in ssh->error while the send is + * short, and its return describes the receive, which stays idle here. */ +static int test_WorkerReportsOwedFlush(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1550; + wolfSSH_SetIOSend(ctx, CountIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1551; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 64); + if (ch == NULL) { result = -1552; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1553; + goto done; + } + + if (WorkerParkAdjust(ctx, ssh, ch) != WS_SUCCESS) { + result = -1554; + goto done; + } + + /* The send still blocks, so the flush stays owed across both calls. */ + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_WANT_READ) { result = -1555; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_WRITE) { result = -1556; goto done; } + if (ssh->outputBuffer.length == 0) { result = -1557; goto done; } + + /* The second call must not retire it. */ + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_WANT_READ) { result = -1558; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_WRITE) { result = -1559; goto done; } + if (ssh->outputBuffer.length == 0) { result = -1560; goto done; } + + /* Bundled credit is owed by the output buffer, not the channel. */ + if (ch->pendingWindowAdjust != 0) { result = -1561; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* Extended data arriving on a call whose flush back-pressures must still be + * routed to the caller. wolfscp.c _RecvScpMessage() and the echoserver switch + * on wolfSSH_get_error(), not on the return value. */ +static int test_WorkerExtDataWithOwedFlush(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + word32 reportedId = 0xFFFFFFFF; + byte pkt[32]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1570; + wolfSSH_SetIOSend(ctx, CountIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1571; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 64); + if (ch == NULL) { result = -1572; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1573; + goto done; + } + + if (WorkerParkAdjust(ctx, ssh, ch) != WS_SUCCESS) { + result = -1574; + goto done; + } + + /* Stderr lands on the same call whose flush is still short. */ + s_recvPkt = pkt; + s_recvPktSz = BuildExtDataStderrPacket(pkt, ch->channel, 0x55); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, &reportedId); + if (ret != WS_EXTDATA) { result = -1575; goto done; } + if (reportedId != ch->channel) { result = -1576; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_WRITE) { result = -1577; goto done; } + if (ssh->outputBuffer.length == 0) { result = -1578; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* A decrypt, MAC or socket failure on the receive must reach the caller. A + * back-pressured flush on the same call reports WS_WANT_WRITE, which reads as + * transient and would have the caller retry a dead session. */ +static int test_WorkerHardRecvErrorOutranksFlush(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte pkt[8]; + + /* packet_length far past MAX_PACKET_SZ, so DoReceive() fails the length + * check with WS_OVERFLOW_E instead of blocking. */ + static const byte badLen[8] = { + 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x00 + }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1580; + wolfSSH_SetIOSend(ctx, CountIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1581; goto done; } + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 64); + if (ch == NULL) { result = -1582; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1583; + goto done; + } + + if (WorkerParkAdjust(ctx, ssh, ch) != WS_SUCCESS) { + result = -1584; + goto done; + } + + WMEMCPY(pkt, badLen, sizeof(pkt)); + s_recvPkt = pkt; + s_recvPktSz = (word32)sizeof(pkt); + s_recvPktOff = 0; + + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_OVERFLOW_E) { result = -1585; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_WRITE) { result = -1586; goto done; } + +done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + /* The documented primary flow: wolfSSH_stream_read() reports WS_EXTDATA when * stderr arrives on the head channel, and the caller drains it with * wolfSSH_extended_data_read() until that returns 0 (wolfssh/ssh.h). */ @@ -6313,7 +6584,7 @@ static int test_ChannelReadExtHardFailureReported(void) * WS_WANT_WRITE. wolfSSH_SendPacket() does not clear ssh->error once it drains * the output buffer, so restoring the saved value unconditionally leaves the * caller polling for writability with nothing left to write. */ -static int test_ChannelReadExtClearsStaleWantWrite(void) +static int test_ChannelReadExtWorkerRetiresWantWrite(void) { WOLFSSH_CTX* ctx = NULL; WOLFSSH* ssh = NULL; @@ -6341,6 +6612,7 @@ static int test_ChannelReadExtClearsStaleWantWrite(void) if (ctx == NULL) return -1450; wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); ssh = wolfSSH_new(ctx); if (ssh == NULL) { result = -1451; goto done; } @@ -6370,16 +6642,27 @@ static int test_ChannelReadExtClearsStaleWantWrite(void) if (ret != 10) { result = -1456; goto done; } if (wolfSSH_get_error(ssh) != WS_WANT_WRITE) { result = -1457; goto done; } - /* Pass 2 succeeds and drains the output buffer completely. */ + /* Pass 2 succeeds and drains the output buffer completely. The read does + * not retire the status itself. */ wolfSSH_SetIOSend(ctx, DiscardIoSend); ret = wolfSSH_extended_data_read(ssh, out, (word32)sizeof(out)); if (ret != 10) { result = -1458; goto done; } if (ssh->outputBuffer.length != 0) { result = -1459; goto done; } - /* Nothing is queued, so no flush may be reported as owed. */ - if (wolfSSH_get_error(ssh) == WS_WANT_WRITE) { result = -1460; goto done; } - if (wolfSSH_get_error(ssh) != WS_SUCCESS) { result = -1461; goto done; } + + /* wolfSSH_worker() is what retires it: it flushes whatever is queued and + * rewrites ssh->error from the receive on every call. Nothing is queued + * here, so the idle receive is all that is left to report. */ + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + ret = wolfSSH_worker(ssh, NULL); + if (ret != WS_WANT_READ) { result = -1460; goto done; } + if (wolfSSH_get_error(ssh) != WS_WANT_READ) { result = -1461; goto done; } done: + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); return result; @@ -6612,6 +6895,7 @@ static int test_ChannelIdRead_deferredWindowAdjust(void) if (ctx == NULL) return -7010; wolfSSH_SetIOSend(ctx, WantWriteIoSend); + wolfSSH_SetIORecv(ctx, PacketIoRecv); ssh = wolfSSH_new(ctx); if (ssh == NULL) { result = -7011; goto done; } @@ -6669,8 +6953,8 @@ static int test_ChannelIdRead_deferredWindowAdjust(void) 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. */ + /* The read sends its credit but does not retire the owed-flush status; + * wolfSSH_worker() does, by flushing and rewriting ssh->error each call. */ wolfSSH_SetIOSend(ctx, DiscardIoSend); ssh->error = WS_WANT_WRITE; @@ -6681,10 +6965,15 @@ static int test_ChannelIdRead_deferredWindowAdjust(void) 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 (wolfSSH_get_error(ssh) != 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; } + s_recvPkt = NULL; + s_recvPktSz = 0; + s_recvPktOff = 0; + (void)wolfSSH_worker(ssh, NULL); + if (wolfSSH_get_error(ssh) == WS_WANT_WRITE) { result = -7029; goto done; } + /* A read with nothing buffered sends no credit, so it has no standing to * retire a WS_WANT_WRITE some other sender is still owed. */ ssh->error = WS_WANT_WRITE; @@ -17531,6 +17820,26 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_WorkerFlushesOnIdleReceive(); + printf("WorkerFlushesOnIdleReceive: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_WorkerReportsOwedFlush(); + printf("WorkerReportsOwedFlush: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_WorkerExtDataWithOwedFlush(); + printf("WorkerExtDataWithOwedFlush: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_WorkerHardRecvErrorOutranksFlush(); + printf("WorkerHardRecvErrorOutranksFlush: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_StreamReadExtDataHeadChannel(); printf("StreamReadExtDataHeadChannel: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); @@ -17546,8 +17855,8 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; - unitResult = test_ChannelReadExtClearsStaleWantWrite(); - printf("ChannelReadExtClearsStaleWantWrite: %s\n", + unitResult = test_ChannelReadExtWorkerRetiresWantWrite(); + printf("ChannelReadExtWorkerRetiresWantWrite: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult;