From 94d0fc482eee83be7c710e5e955505096bf3c3e1 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 4 Aug 2026 15:30:46 -0700 Subject: [PATCH 1/3] tests: fix api.test SFTP read target race - Opening the listing's first entry raced with tests/testsuite.test, which creates and removes files in the same directory under "make -j check"; the entry could be gone before the open. - Stage the read target over SFTP, select it from the listing by name, and remove it afterward. - Check the listing with AssertNotNull; a missing entry used to skip the whole test body. --- tests/api.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/tests/api.c b/tests/api.c index 1edada867..94d206f51 100644 --- a/tests/api.c +++ b/tests/api.c @@ -2738,6 +2738,15 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) int outSz = 18; int rxSz; const word32 ofst[2] = {0}; + char rdName[] = "wolfssh_5574_read.tmp"; + byte rdHandle[WOLFSSH_MAX_HANDLE]; + word32 rdHandleSz; + word32 rdOfst[2] = {0, 0}; + word32 rdSz = 0; + byte rdData[4096]; + int rdTries; + int rdWrote; + int rdErr; #ifdef WOLFSSH_TEST_INTERNAL int err; int tries; @@ -2749,16 +2758,46 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) char wrName[] = "wolfssh_5574_write.tmp"; #endif + /* Stage the file to read. Opening the listing's first entry raced + * with tests/testsuite.test, which creates and removes files in the + * same directory under "make -j check". Skipped if create is + * denied. */ + rdHandleSz = WOLFSSH_MAX_HANDLE; + WMEMSET(rdData, 'a', sizeof(rdData)); + if (wolfSSH_SFTP_Open(ssh, rdName, + WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT | WOLFSSH_FXF_TRUNC, + NULL, rdHandle, &rdHandleSz) == WS_SUCCESS) { + for (rdTries = 0; + rdTries < 1000 && rdSz < WOLFSSH_MAX_SFTP_RW; + rdTries++) { + rdOfst[0] = rdSz; + rdWrote = wolfSSH_SFTP_SendWritePacket(ssh, rdHandle, + rdHandleSz, rdOfst, rdData, (word32)sizeof(rdData)); + if (rdWrote > 0) { + rdSz += (word32)rdWrote; + continue; + } + rdErr = wolfSSH_get_error(ssh); + if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && + rdErr != WS_REKEYING) { + break; /* unexpected error */ + } + } + wolfSSH_SFTP_Close(ssh, rdHandle, rdHandleSz); + AssertIntEQ(rdSz, WOLFSSH_MAX_SFTP_RW); + } + current = wolfSSH_SFTP_LS(ssh, (char*)currentDir); tmp = current; while (tmp != NULL) { - if ((tmp->atrb.sz[0] > 0) && - (tmp->atrb.flags & WOLFSSH_FILEATRB_PERM) && - !(tmp->atrb.per & 040000)) { + if (WSTRCMP(tmp->fName, rdName) == 0) { break; } tmp = tmp->next; } + if (rdSz > 0) { + AssertNotNull(tmp); + } if (tmp != NULL) { /* Allocate buffer large enough for maximum read size */ @@ -2847,6 +2886,10 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) } wolfSSH_SFTPNAME_list_free(current); + if (rdSz > 0) { + wolfSSH_SFTP_Remove(ssh, rdName); + } + #ifdef WOLFSSH_TEST_INTERNAL /* Issue 5574: exercise the partial-send resume path for * wolfSSH_SFTP_SendWritePacket. STATE_SEND_WRITE_SEND_HEADER must not From 9f2f1585e1eae46735688b538dd9bf28360fde9e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 4 Aug 2026 15:56:33 -0700 Subject: [PATCH 2/3] tests: harden SFTP read target staging - Clamp the staging write chunk to the bytes remaining and derive the try cap from the chunk count, so the loop lands exactly on WOLFSSH_MAX_SFTP_RW for any value of the macro. - Retry the staging Open and the LS on WS_WANT_READ, WS_WANT_WRITE, and WS_REKEYING, matching the rekey tolerance used elsewhere in the function. - Assert the staging Open, the LS, and the listing match rather than keying the read block on them, so neither a failed create nor a connection stuck in WANT/REKEYING until the try cap can drop the read coverage without failing the test. - Remove any stale file before the staging Open, so the reads never target a file staged by a prior aborted run. - Size the read buffer from WOLFSSH_MAX_SFTP_RW, the amount staged and the largest amount read, instead of the listed file size, and drop the listed-size guard on the 18 byte read. - Guard the listing name compare against a NULL fName. - Verify every read returns the staged fill byte, not just a valid length; the check is a macro so a failure reports the calling read's line. --- tests/api.c | 131 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 86 insertions(+), 45 deletions(-) diff --git a/tests/api.c b/tests/api.c index 94d206f51..208a5c235 100644 --- a/tests/api.c +++ b/tests/api.c @@ -2697,6 +2697,20 @@ static void sftp_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) } +/* The staged read target is filled with 'a'; every read starts at offset 0, + * so all returned bytes must be 'a'. A macro so a failure reports the + * calling read's line. */ +#define SFTP_CHECK_READ_PAYLOAD(out, rxSz) do { \ + int _i; \ + for (_i = 0; _i < (rxSz); _i++) { \ + if ((out)[_i] != 'a') { \ + break; \ + } \ + } \ + AssertIntEQ(_i, (rxSz)); \ +} while (0) + + static void test_wolfSSH_SFTP_SendReadPacket(void) { func_args ser; @@ -2743,10 +2757,12 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) word32 rdHandleSz; word32 rdOfst[2] = {0, 0}; word32 rdSz = 0; + word32 rdChunk; byte rdData[4096]; int rdTries; int rdWrote; int rdErr; + int rdRet; #ifdef WOLFSSH_TEST_INTERNAL int err; int tries; @@ -2760,51 +2776,77 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) /* Stage the file to read. Opening the listing's first entry raced * with tests/testsuite.test, which creates and removes files in the - * same directory under "make -j check". Skipped if create is - * denied. */ - rdHandleSz = WOLFSSH_MAX_HANDLE; + * same directory under "make -j check". The staging is asserted + * rather than skipped on failure; skipping would drop the read + * coverage below without failing the test. */ WMEMSET(rdData, 'a', sizeof(rdData)); - if (wolfSSH_SFTP_Open(ssh, rdName, - WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT | WOLFSSH_FXF_TRUNC, - NULL, rdHandle, &rdHandleSz) == WS_SUCCESS) { - for (rdTries = 0; - rdTries < 1000 && rdSz < WOLFSSH_MAX_SFTP_RW; - rdTries++) { - rdOfst[0] = rdSz; - rdWrote = wolfSSH_SFTP_SendWritePacket(ssh, rdHandle, - rdHandleSz, rdOfst, rdData, (word32)sizeof(rdData)); - if (rdWrote > 0) { - rdSz += (word32)rdWrote; - continue; - } - rdErr = wolfSSH_get_error(ssh); - if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && - rdErr != WS_REKEYING) { - break; /* unexpected error */ - } + wolfSSH_SFTP_Remove(ssh, rdName); /* clear any stale file */ + rdRet = WS_FATAL_ERROR; + for (rdTries = 0; rdTries < 1000; rdTries++) { + rdHandleSz = WOLFSSH_MAX_HANDLE; + rdRet = wolfSSH_SFTP_Open(ssh, rdName, + WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT | WOLFSSH_FXF_TRUNC, + NULL, rdHandle, &rdHandleSz); + if (rdRet == WS_SUCCESS) { + break; + } + rdErr = wolfSSH_get_error(ssh); + if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && + rdErr != WS_REKEYING) { + break; /* create failed */ + } + } + AssertIntEQ(rdRet, WS_SUCCESS); + + /* one try per full chunk plus slack for transient errors */ + rdTries = (int)(WOLFSSH_MAX_SFTP_RW / sizeof(rdData)) + 1000; + for (; rdTries > 0 && rdSz < WOLFSSH_MAX_SFTP_RW; rdTries--) { + rdChunk = (word32)sizeof(rdData); + if (rdChunk > WOLFSSH_MAX_SFTP_RW - rdSz) { + rdChunk = WOLFSSH_MAX_SFTP_RW - rdSz; + } + rdOfst[0] = rdSz; + rdWrote = wolfSSH_SFTP_SendWritePacket(ssh, rdHandle, + rdHandleSz, rdOfst, rdData, rdChunk); + if (rdWrote > 0) { + rdSz += (word32)rdWrote; + continue; + } + rdErr = wolfSSH_get_error(ssh); + if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && + rdErr != WS_REKEYING) { + break; /* unexpected error */ } - wolfSSH_SFTP_Close(ssh, rdHandle, rdHandleSz); - AssertIntEQ(rdSz, WOLFSSH_MAX_SFTP_RW); } + wolfSSH_SFTP_Close(ssh, rdHandle, rdHandleSz); + AssertIntEQ(rdSz, WOLFSSH_MAX_SFTP_RW); - current = wolfSSH_SFTP_LS(ssh, (char*)currentDir); + current = NULL; + for (rdTries = 0; rdTries < 1000; rdTries++) { + current = wolfSSH_SFTP_LS(ssh, (char*)currentDir); + if (current != NULL) { + break; + } + rdErr = wolfSSH_get_error(ssh); + if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && + rdErr != WS_REKEYING) { + break; + } + } + AssertNotNull(current); tmp = current; while (tmp != NULL) { - if (WSTRCMP(tmp->fName, rdName) == 0) { + if (tmp->fName != NULL && WSTRCMP(tmp->fName, rdName) == 0) { break; } tmp = tmp->next; } - if (rdSz > 0) { - AssertNotNull(tmp); - } + AssertNotNull(tmp); - if (tmp != NULL) { - /* Allocate buffer large enough for maximum read size */ - word32 allocSz = tmp->atrb.sz[0]; - if (allocSz < WOLFSSH_MAX_SFTP_RW) - allocSz = WOLFSSH_MAX_SFTP_RW; - out = (byte*)malloc(allocSz); + { + /* The staging above wrote WOLFSSH_MAX_SFTP_RW bytes, and no read + * below asks for more, so that is the buffer size needed. */ + out = (byte*)malloc(WOLFSSH_MAX_SFTP_RW); AssertNotNull(out); AssertIntEQ(wolfSSH_SFTP_Open(ssh, tmp->fName, WOLFSSH_FXF_READ, NULL, handle, &handleSz), WS_SUCCESS); @@ -2818,13 +2860,12 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) */ /* read 18 bytes */ - if (tmp->atrb.sz[0] >= 18) { - outSz = 18; - rxSz = wolfSSH_SFTP_SendReadPacket(ssh, handle, handleSz, - ofst, out, outSz); - AssertIntGT(rxSz, 0); - AssertIntLE(rxSz, outSz); - } + outSz = 18; + rxSz = wolfSSH_SFTP_SendReadPacket(ssh, handle, handleSz, + ofst, out, outSz); + AssertIntGT(rxSz, 0); + AssertIntLE(rxSz, outSz); + SFTP_CHECK_READ_PAYLOAD(out, rxSz); /* partial read */ outSz = WOLFSSH_MAX_SFTP_RW / 2; @@ -2833,6 +2874,7 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) if (wolfSSH_get_error(ssh) != WS_REKEYING) { AssertIntGT(rxSz, 0); AssertIntLE(rxSz, outSz); + SFTP_CHECK_READ_PAYLOAD(out, rxSz); } /* read all */ @@ -2842,6 +2884,7 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) if (wolfSSH_get_error(ssh) != WS_REKEYING) { AssertIntGT(rxSz, 0); AssertIntLE(rxSz, outSz); + SFTP_CHECK_READ_PAYLOAD(out, rxSz); } #ifdef WOLFSSH_TEST_INTERNAL @@ -2879,16 +2922,14 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) AssertIntLT(tries, 1000); AssertIntGT(rxSz, 0); AssertIntLE(rxSz, outSz); + SFTP_CHECK_READ_PAYLOAD(out, rxSz); #endif /* WOLFSSH_TEST_INTERNAL */ free(out); wolfSSH_SFTP_Close(ssh, handle, handleSz); } wolfSSH_SFTPNAME_list_free(current); - - if (rdSz > 0) { - wolfSSH_SFTP_Remove(ssh, rdName); - } + wolfSSH_SFTP_Remove(ssh, rdName); #ifdef WOLFSSH_TEST_INTERNAL /* Issue 5574: exercise the partial-send resume path for From 50366537788ee4b83c9a6b1f287f4c672f8609a2 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 4 Aug 2026 22:10:19 -0700 Subject: [PATCH 3/3] tests: retry stateful SFTP calls in read staging - add sftp_retry_remove()/sftp_retry_close() so Close and Remove are driven to completion instead of abandoned mid-operation - split the retry predicate: Open and SendWritePacket keep state only on WS_WANT_READ/WS_WANT_WRITE, LS adds WS_REKEYING, Close and Remove gate on NoticeError() - assert the staging close; keep the post-read cleanup best effort, as the reads are skipped rather than completed on WS_REKEYING - route the WOLFSSH_TEST_INTERNAL write cleanup through the helpers - shrink the staging chunk to 512 bytes --- tests/api.c | 106 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 89 insertions(+), 17 deletions(-) diff --git a/tests/api.c b/tests/api.c index 208a5c235..6e3359e98 100644 --- a/tests/api.c +++ b/tests/api.c @@ -2697,6 +2697,71 @@ static void sftp_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) } +/* Upper bound on retry iterations for the SFTP helpers below. */ +#define SFTP_MAX_RETRY_TRIES 1000 + +/* Which errors are safe to retry depends on the callee: re-calling one that + * has already torn down its state reissues a request that may be partly on + * the wire, which is the desync these loops exist to avoid. + * + * wolfSSH_SFTP_Open() and wolfSSH_SFTP_SendWritePacket() keep their state + * only for these two (src/wolfsftp.c STATE_OPEN_SEND, STATE_OPEN_GETHANDLE, + * STATE_SEND_WRITE_READ_STATUS). wolfSSH_SFTP_LS() also keeps it on + * WS_REKEYING. */ +static int sftp_error_keeps_state(int err) +{ + return err == WS_WANT_READ || err == WS_WANT_WRITE; +} + + +/* wolfSSH_SFTP_Remove() and wolfSSH_SFTP_Close() gate on NoticeError() in + * src/wolfsftp.c instead, which is broader. Mirror it exactly; the two must + * not drift apart. */ +static int sftp_error_is_notice(int err) +{ + return sftp_error_keeps_state(err) || err == WS_CHAN_RXD || + err == WS_WINDOW_FULL || err == WS_REKEYING; +} + + +/* Drive an SFTP operation to completion. Abandoning one part way leaves its + * response pending on the stream, which the next operation then reads as its + * own header. Both helpers stop on a terminal error so a real failure is + * still reported to the caller. */ +static int sftp_retry_remove(WOLFSSH* ssh, char* name) +{ + int ret = WS_FATAL_ERROR; + int tries; + + for (tries = 0; tries < SFTP_MAX_RETRY_TRIES; tries++) { + ret = wolfSSH_SFTP_Remove(ssh, name); + if (ret == WS_SUCCESS || + !sftp_error_is_notice(wolfSSH_get_error(ssh))) { + break; + } + } + + return ret; +} + + +static int sftp_retry_close(WOLFSSH* ssh, byte* handle, word32 handleSz) +{ + int ret = WS_FATAL_ERROR; + int tries; + + for (tries = 0; tries < SFTP_MAX_RETRY_TRIES; tries++) { + ret = wolfSSH_SFTP_Close(ssh, handle, handleSz); + if (ret == WS_SUCCESS || + !sftp_error_is_notice(wolfSSH_get_error(ssh))) { + break; + } + } + + return ret; +} + + /* The staged read target is filled with 'a'; every read starts at offset 0, * so all returned bytes must be 'a'. A macro so a failure reports the * calling read's line. */ @@ -2758,7 +2823,8 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) word32 rdOfst[2] = {0, 0}; word32 rdSz = 0; word32 rdChunk; - byte rdData[4096]; + /* staging chunk, kept small for constrained targets */ + byte rdData[512]; int rdTries; int rdWrote; int rdErr; @@ -2778,11 +2844,13 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) * with tests/testsuite.test, which creates and removes files in the * same directory under "make -j check". The staging is asserted * rather than skipped on failure; skipping would drop the read - * coverage below without failing the test. */ + * coverage below without failing the test. Note this makes a + * writable server working directory a prerequisite of the test. */ WMEMSET(rdData, 'a', sizeof(rdData)); - wolfSSH_SFTP_Remove(ssh, rdName); /* clear any stale file */ + /* best effort, the file is normally absent */ + (void)sftp_retry_remove(ssh, rdName); rdRet = WS_FATAL_ERROR; - for (rdTries = 0; rdTries < 1000; rdTries++) { + for (rdTries = 0; rdTries < SFTP_MAX_RETRY_TRIES; rdTries++) { rdHandleSz = WOLFSSH_MAX_HANDLE; rdRet = wolfSSH_SFTP_Open(ssh, rdName, WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT | WOLFSSH_FXF_TRUNC, @@ -2791,15 +2859,15 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) break; } rdErr = wolfSSH_get_error(ssh); - if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && - rdErr != WS_REKEYING) { + if (!sftp_error_keeps_state(rdErr)) { break; /* create failed */ } } AssertIntEQ(rdRet, WS_SUCCESS); /* one try per full chunk plus slack for transient errors */ - rdTries = (int)(WOLFSSH_MAX_SFTP_RW / sizeof(rdData)) + 1000; + rdTries = (int)(WOLFSSH_MAX_SFTP_RW / sizeof(rdData)) + + SFTP_MAX_RETRY_TRIES; for (; rdTries > 0 && rdSz < WOLFSSH_MAX_SFTP_RW; rdTries--) { rdChunk = (word32)sizeof(rdData); if (rdChunk > WOLFSSH_MAX_SFTP_RW - rdSz) { @@ -2813,23 +2881,22 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) continue; } rdErr = wolfSSH_get_error(ssh); - if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && - rdErr != WS_REKEYING) { + if (!sftp_error_keeps_state(rdErr)) { break; /* unexpected error */ } } - wolfSSH_SFTP_Close(ssh, rdHandle, rdHandleSz); + rdRet = sftp_retry_close(ssh, rdHandle, rdHandleSz); AssertIntEQ(rdSz, WOLFSSH_MAX_SFTP_RW); + AssertIntEQ(rdRet, WS_SUCCESS); current = NULL; - for (rdTries = 0; rdTries < 1000; rdTries++) { + for (rdTries = 0; rdTries < SFTP_MAX_RETRY_TRIES; rdTries++) { current = wolfSSH_SFTP_LS(ssh, (char*)currentDir); if (current != NULL) { break; } rdErr = wolfSSH_get_error(ssh); - if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE && - rdErr != WS_REKEYING) { + if (!sftp_error_keeps_state(rdErr) && rdErr != WS_REKEYING) { break; } } @@ -2926,10 +2993,13 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) #endif /* WOLFSSH_TEST_INTERNAL */ free(out); - wolfSSH_SFTP_Close(ssh, handle, handleSz); + /* Results ignored: the reads above are skipped rather than driven + * to completion on WS_REKEYING, so the stream state here is not + * known to be clean and a failure would not mean a real fault. */ + (void)sftp_retry_close(ssh, handle, handleSz); } wolfSSH_SFTPNAME_list_free(current); - wolfSSH_SFTP_Remove(ssh, rdName); + (void)sftp_retry_remove(ssh, rdName); #ifdef WOLFSSH_TEST_INTERNAL /* Issue 5574: exercise the partial-send resume path for @@ -2970,8 +3040,10 @@ static void test_wolfSSH_SFTP_SendReadPacket(void) AssertIntLT(tries, 1000); AssertIntEQ(wrRet, (int)sizeof(wrData)); - wolfSSH_SFTP_Close(ssh, whandle, whandleSz); - wolfSSH_SFTP_Remove(ssh, wrName); + /* last SFTP calls before the rekey drain and shutdown assert, so + * do not leave a response pending here */ + (void)sftp_retry_close(ssh, whandle, whandleSz); + (void)sftp_retry_remove(ssh, wrName); } #endif /* WOLFSSH_TEST_INTERNAL */ }