From 9b4875adcac0145272ff6b8c76e7cdeeade86aa8 Mon Sep 17 00:00:00 2001 From: ouki Date: Sun, 11 Oct 2026 15:53:34 +0900 Subject: [PATCH 1/3] Fix SendPacketType to not advance state when negotiation fails SendPacketType() set the state to SFTP_SEND_PACKET even when wolfSSH_SFTP_negotiate() returned a retry or an error. No request buffer was built in that case, so an empty buffer was sent and the caller could wait for a response to a request that was never sent. Move to SFTP_SEND_PACKET only when the request has been built, and return the negotiation result otherwise. Keep the state for retryable errors. Add the wolfSSH_TestSftpSendPacketType() test hook and an API test that fails without this change. F-13904 --- src/wolfsftp.c | 18 +++++++++++++++++- tests/api.c | 28 ++++++++++++++++++++++++++++ wolfssh/wolfsftp.h | 2 ++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 699d1e886..76a740521 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -711,6 +711,15 @@ int wolfSSH_TestSftpRecvSizeCheck(int sz) { return SFTP_CheckRecvSz(sz); } + + +/* Test hook: exposes the static SendPacketType() so the unit tests can + * check that no request is sent when SFTP negotiation did not succeed. */ +int wolfSSH_TestSftpSendPacketType(WOLFSSH* ssh, byte type, byte* buf, + word32 bufSz) +{ + return SendPacketType(ssh, type, buf, bufSz); +} #endif @@ -6905,8 +6914,15 @@ int SendPacketType(WOLFSSH* ssh, byte type, byte* buf, word32 bufSz) /* reset state for sending data */ wolfSSH_SFTP_buffer_rewind(&state->buffer); + state->state = SFTP_SEND_PACKET; + } + else { + if(!NoticeError(ssh)){ + wolfSSH_SFTP_ClearState(ssh, STATE_ID_SEND); + } + return ret; } - state->state = SFTP_SEND_PACKET; + FALL_THROUGH; case SFTP_SEND_PACKET: diff --git a/tests/api.c b/tests/api.c index 5be9c300d..dab36b9dd 100644 --- a/tests/api.c +++ b/tests/api.c @@ -6267,6 +6267,32 @@ static void test_wolfSSH_SFTP_SaveOfst(void) } +/* SendPacketType must not send a request when negotiation did not succeed */ +static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) +{ +#ifdef WOLFSSH_TEST_INTERNAL + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte payload[4] = { 0x00, 0x01, 0x02, 0x03 }; + int ret; + + AssertNotNull(ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL)); + AssertNotNull(ssh = wolfSSH_new(ctx)); + + ret = wolfSSH_TestSftpSendPacketType(ssh, WOLFSSH_FTP_REALPATH, + payload, (word32)sizeof(payload)); + + /* The SFTP connection was not established, so negotiation cannot succeed. + * SendPacketType() must report that failure instead of returning WS_SUCCESS */ + AssertIntNE(ret, WS_SUCCESS); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +#endif /* WOLFSSH_TEST_INTERNAL */ +} + + + #if !defined(NO_FILESYSTEM) && !defined(WOLFSSH_USER_FILESYSTEM) && \ !defined(WOLFSSH_ZEPHYR) @@ -6690,6 +6716,7 @@ static void test_wolfSSH_SFTP_StartPathNotConfined(void) { ; } static void test_wolfSSH_SFTP_SetConfinePath(void) { ; } static void test_wolfSSH_SFTP_SetDefaultPath(void) { ; } static void test_wolfSSH_SFTP_SaveOfst(void) { ; } +static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) { ; } static void test_wolfSSH_SFTP_PutResume(void) { ; } static void test_wolfSSH_SFTP_GetResume(void) { ; } #endif /* WOLFSSH_SFTP && !NO_WOLFSSH_CLIENT && !SINGLE_THREADED */ @@ -8823,6 +8850,7 @@ int wolfSSH_ApiTest(int argc, char** argv) test_wolfSSH_SFTP_SetConfinePath(); test_wolfSSH_SFTP_SetDefaultPath(); test_wolfSSH_SFTP_SaveOfst(); + test_wolfSSH_SFTP_SendPacketType_negotiate(); test_wolfSSH_SFTP_PutResume(); test_wolfSSH_SFTP_GetResume(); diff --git a/wolfssh/wolfsftp.h b/wolfssh/wolfsftp.h index 9b3c9aefd..fe0a7a08a 100644 --- a/wolfssh/wolfsftp.h +++ b/wolfssh/wolfsftp.h @@ -360,6 +360,8 @@ WOLFSSH_LOCAL void wolfSSH_SFTP_ShowSizes(void); WS_SFTP_FILEATRB* atr, word32* idx); WOLFSSH_API int wolfSSH_TestSftpSendCap(WOLFSSH* ssh, word32 cap); WOLFSSH_API int wolfSSH_TestSftpStallPending(WOLFSSH* ssh, word32 count); + WOLFSSH_API int wolfSSH_TestSftpSendPacketType(WOLFSSH* ssh, byte type, + byte* buf, word32 bufSz); #if !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM) WOLFSSH_API int wolfSSH_SFTP_TestRecvStateInit(WOLFSSH* ssh); WOLFSSH_API const byte* wolfSSH_SFTP_TestRecvReply(WOLFSSH* ssh, From ae1ac5d571edd78aa1bf0eca4d80211ba4d0b29a Mon Sep 17 00:00:00 2001 From: ouki Date: Sun, 11 Oct 2026 15:53:34 +0900 Subject: [PATCH 2/3] Add test for retryable SFTP negotiation on non-blocking socket --- src/wolfsftp.c | 3 +- tests/api.c | 171 ++++++++++++++++++++++++++++++++++++++++++++- wolfssh/wolfsftp.h | 2 +- 3 files changed, 172 insertions(+), 4 deletions(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 76a740521..42768e82b 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -6917,12 +6917,11 @@ int SendPacketType(WOLFSSH* ssh, byte type, byte* buf, word32 bufSz) state->state = SFTP_SEND_PACKET; } else { - if(!NoticeError(ssh)){ + if(!NoticeError(ssh)) { wolfSSH_SFTP_ClearState(ssh, STATE_ID_SEND); } return ret; } - FALL_THROUGH; case SFTP_SEND_PACKET: diff --git a/tests/api.c b/tests/api.c index dab36b9dd..5ca7da85e 100644 --- a/tests/api.c +++ b/tests/api.c @@ -6283,7 +6283,8 @@ static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) payload, (word32)sizeof(payload)); /* The SFTP connection was not established, so negotiation cannot succeed. - * SendPacketType() must report that failure instead of returning WS_SUCCESS */ + * SendPacketType() must report that failure instead of returning + * WS_SUCCESS */ AssertIntNE(ret, WS_SUCCESS); wolfSSH_free(ssh); @@ -6291,6 +6292,172 @@ static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) #endif /* WOLFSSH_TEST_INTERNAL */ } +/* Same as sftp_client_connect(), but stops before wolfSSH_SFTP_connect() and + * leaves the socket non-blocking. The first SFTP request then has to drive + * the SSH handshake and the SFTP negotiation itself. */ +static void wolfSSH_SFTP_connect_nonblock(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) +{ + SOCKET_T sockFd = WOLFSSH_SOCKET_INVALID; + SOCKADDR_IN_T clientAddr; + socklen_t clientAddrSz = sizeof(clientAddr); + int ret; + char* host = (char*)wolfSshIp; + const char* username = "jill"; + const char* password = "upthehill"; + + if (ctx == NULL || ssh == NULL) { + return; + } + + *ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (*ctx == NULL) { + return; + } + + wolfSSH_CTX_SetPublicKeyCheck(*ctx, AcceptAnyServerHostKey); + wolfSSH_SetUserAuth(*ctx, sftpUserAuth); + *ssh = wolfSSH_new(*ctx); + if (*ssh == NULL) { + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + return; + } + + build_addr(&clientAddr, host, port); + tcp_socket(&sockFd, ((struct sockaddr_in *)&clientAddr)->sin_family); + if (sockFd < 0) { + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + return; + } + + ret = connect(sockFd, (const struct sockaddr *)&clientAddr, clientAddrSz); + if (ret != 0) { + WCLOSESOCKET(sockFd); + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + return; + } + + wolfSSH_SetUserAuthCtx(*ssh, (void*)password); + ret = wolfSSH_SetUsername(*ssh, username); + if (ret == WS_SUCCESS) + ret = wolfSSH_set_fd(*ssh, (int)sockFd); + + if (ret != WS_SUCCESS) { + WCLOSESOCKET(sockFd); + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + return; + } + + /* no wolfSSH_connect() or wolfSSH_SFTP_connect() here; the caller's first + * SFTP request negotiates over the non-blocking socket */ + tcp_set_nonblocking(&sockFd); +} + +/* Upper bound on non-blocking retry iterations. Each wait is at most 1s, so a + * regression fails the AssertIntLE below instead of hanging CI. */ +#define SFTP_MAX_NEGOTIATION_TRIES 100 + +/* The retryable half of the SendPacketType() negotiation fix. Without + * wolfSSH_SFTP_connect(), the first request on a non-blocking socket must + * negotiate first. Negotiation returns WS_WANT_READ/WS_WANT_WRITE, the send + * state is kept, and a later call finishes negotiating and actually sends the + * request. Before the fix the first call failed with WS_BUFFER_E instead. */ +static void test_wolfSSH_SFTP_SendPacketType_negotiation_non_block(void) +{ + func_args ser; + tcp_ready ready; + int argsCount; + WS_SOCKET_T clientFd; + const char* args[10]; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WS_SFTPNAME* name = NULL; + THREAD_TYPE serThread; + int sawPartial = 0; + int err; + int tries = 0; + + /* one-shot echoserver on a free port */ + WMEMSET(&ser, 0, sizeof(func_args)); + argsCount = 0; + args[argsCount++] = "."; + args[argsCount++] = "-1"; + args[argsCount++] = "-p"; + args[argsCount++] = "0"; + ser.argv = (char**)args; + ser.argc = argsCount; + ser.signal = &ready; + InitTcpReady(ser.signal); + ThreadStart(echoserver_test, (void*)&ser, &serThread); + WaitTcpReady(&ready); + + wolfSSH_SFTP_connect_nonblock(&ctx, &ssh, ready.port); + AssertNotNull(ctx); + AssertNotNull(ssh); + clientFd = wolfSSH_get_fd(ssh); + + /* RealPath(".") goes through　SendPacketType(). Retry it until + * the negotiation completes and the server answers with a name. */ + do { + name = wolfSSH_SFTP_RealPath(ssh, (char*)"."); + err = wolfSSH_get_error(ssh); + if (name == NULL && (err == WS_WANT_READ || err == WS_WANT_WRITE)) { + sawPartial = 1; + tcp_select(clientFd, 1); + } + tries++; + } while (name == NULL && (err == WS_WANT_READ || err == WS_WANT_WRITE) && + tries <= SFTP_MAX_NEGOTIATION_TRIES); + + AssertIntLE(tries, SFTP_MAX_NEGOTIATION_TRIES); + AssertIntEQ(sawPartial, 1); + AssertNotNull(name); + AssertNotNull(name->fName); + wolfSSH_SFTPNAME_list_free(name); + + /* shut down over the non-blocking socket, retrying the same way */ + tries = 0; + do { + argsCount = wolfSSH_shutdown(ssh); + err = wolfSSH_get_error(ssh); + if (argsCount != WS_SUCCESS && (err == WS_WANT_READ || + err == WS_WANT_WRITE || err == WS_REKEYING)) { + tcp_select(clientFd, 1); + } + tries++; + } while (argsCount != WS_SUCCESS && (err == WS_WANT_READ || + err == WS_WANT_WRITE || err == WS_REKEYING) && + tries <= SFTP_MAX_NEGOTIATION_TRIES); + + AssertIntLE(tries, SFTP_MAX_NEGOTIATION_TRIES); + argsCount = AbsorbBenignReset(ssh, argsCount); + +#if DEFAULT_HIGHWATER_MARK < 8000 + if (argsCount == WS_REKEYING) { + argsCount = WS_SUCCESS; + } +#endif + AssertIntEQ(argsCount, WS_SUCCESS); + + WCLOSESOCKET(clientFd); + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +#ifdef WOLFSSH_ZEPHYR + k_sleep(Z_TIMEOUT_TICKS(100)); +#endif + ThreadJoin(serThread); + FreeTcpReady(&ready); +} + #if !defined(NO_FILESYSTEM) && !defined(WOLFSSH_USER_FILESYSTEM) && \ @@ -6717,6 +6884,7 @@ static void test_wolfSSH_SFTP_SetConfinePath(void) { ; } static void test_wolfSSH_SFTP_SetDefaultPath(void) { ; } static void test_wolfSSH_SFTP_SaveOfst(void) { ; } static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) { ; } +static void test_wolfSSH_SFTP_SendPacketType_negotiation_non_block(void) { ; } static void test_wolfSSH_SFTP_PutResume(void) { ; } static void test_wolfSSH_SFTP_GetResume(void) { ; } #endif /* WOLFSSH_SFTP && !NO_WOLFSSH_CLIENT && !SINGLE_THREADED */ @@ -8851,6 +9019,7 @@ int wolfSSH_ApiTest(int argc, char** argv) test_wolfSSH_SFTP_SetDefaultPath(); test_wolfSSH_SFTP_SaveOfst(); test_wolfSSH_SFTP_SendPacketType_negotiate(); + test_wolfSSH_SFTP_SendPacketType_negotiation_non_block(); test_wolfSSH_SFTP_PutResume(); test_wolfSSH_SFTP_GetResume(); diff --git a/wolfssh/wolfsftp.h b/wolfssh/wolfsftp.h index fe0a7a08a..ec3a377ab 100644 --- a/wolfssh/wolfsftp.h +++ b/wolfssh/wolfsftp.h @@ -361,7 +361,7 @@ WOLFSSH_LOCAL void wolfSSH_SFTP_ShowSizes(void); WOLFSSH_API int wolfSSH_TestSftpSendCap(WOLFSSH* ssh, word32 cap); WOLFSSH_API int wolfSSH_TestSftpStallPending(WOLFSSH* ssh, word32 count); WOLFSSH_API int wolfSSH_TestSftpSendPacketType(WOLFSSH* ssh, byte type, - byte* buf, word32 bufSz); + byte* buf, word32 bufSz); #if !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM) WOLFSSH_API int wolfSSH_SFTP_TestRecvStateInit(WOLFSSH* ssh); WOLFSSH_API const byte* wolfSSH_SFTP_TestRecvReply(WOLFSSH* ssh, From 1f23d0f1160c581fddb5ebd0c9c5c07c4d7372a6 Mon Sep 17 00:00:00 2001 From: ouki Date: Sun, 11 Oct 2026 15:53:34 +0900 Subject: [PATCH 3/3] Address review comments --- src/wolfsftp.c | 2 +- tests/api.c | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 42768e82b..cccf00b06 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -6917,7 +6917,7 @@ int SendPacketType(WOLFSSH* ssh, byte type, byte* buf, word32 bufSz) state->state = SFTP_SEND_PACKET; } else { - if(!NoticeError(ssh)) { + if (!NoticeError(ssh)) { wolfSSH_SFTP_ClearState(ssh, STATE_ID_SEND); } return ret; diff --git a/tests/api.c b/tests/api.c index 5ca7da85e..41b10a0fd 100644 --- a/tests/api.c +++ b/tests/api.c @@ -6283,7 +6283,7 @@ static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) payload, (word32)sizeof(payload)); /* The SFTP connection was not established, so negotiation cannot succeed. - * SendPacketType() must report that failure instead of returning + * SendPacketType() must report that failure instead of returning * WS_SUCCESS */ AssertIntNE(ret, WS_SUCCESS); @@ -6295,7 +6295,8 @@ static void test_wolfSSH_SFTP_SendPacketType_negotiate(void) /* Same as sftp_client_connect(), but stops before wolfSSH_SFTP_connect() and * leaves the socket non-blocking. The first SFTP request then has to drive * the SSH handshake and the SFTP negotiation itself. */ -static void wolfSSH_SFTP_connect_nonblock(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) +static void sftp_client_connect_nonblock(WOLFSSH_CTX** ctx, WOLFSSH** ssh, + int port) { SOCKET_T sockFd = WOLFSSH_SOCKET_INVALID; SOCKADDR_IN_T clientAddr; @@ -6370,7 +6371,8 @@ static void wolfSSH_SFTP_connect_nonblock(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int * wolfSSH_SFTP_connect(), the first request on a non-blocking socket must * negotiate first. Negotiation returns WS_WANT_READ/WS_WANT_WRITE, the send * state is kept, and a later call finishes negotiating and actually sends the - * request. Before the fix the first call failed with WS_BUFFER_E instead. */ + * request. Before the fix the empty request was "sent" and the client + * waited for a reply that never came, until the retry bound tripped. */ static void test_wolfSSH_SFTP_SendPacketType_negotiation_non_block(void) { func_args ser; @@ -6400,12 +6402,12 @@ static void test_wolfSSH_SFTP_SendPacketType_negotiation_non_block(void) ThreadStart(echoserver_test, (void*)&ser, &serThread); WaitTcpReady(&ready); - wolfSSH_SFTP_connect_nonblock(&ctx, &ssh, ready.port); + sftp_client_connect_nonblock(&ctx, &ssh, ready.port); AssertNotNull(ctx); AssertNotNull(ssh); clientFd = wolfSSH_get_fd(ssh); - /* RealPath(".") goes through　SendPacketType(). Retry it until + /* RealPath(".") goes through SendPacketType(). Retry it until * the negotiation completes and the server answers with a name. */ do { name = wolfSSH_SFTP_RealPath(ssh, (char*)".");