Skip to content

tests: fix api.test SFTP read target race #1142

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

Merged
JacobBarthelmeh merged 3 commits into from
Aug 5, 2026
+176 −20
Merged

tests: fix api.test SFTP read target race #1142

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94d0fc4
tests: fix api.test SFTP read target race
ejohnstown Aug 4, 2026
9f2f158
tests: harden SFTP read target staging
ejohnstown Aug 4, 2026
5036653
tests: retry stateful SFTP calls in read staging
ejohnstown Aug 5, 2026
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
196 changes: 176 additions & 20 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2697,6 +2697,85 @@ 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. */
#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;
Expand Down Expand Up @@ -2738,6 +2817,18 @@ 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;
word32 rdChunk;
/* staging chunk, kept small for constrained targets */
byte rdData[512];
int rdTries;
int rdWrote;
int rdErr;
int rdRet;
#ifdef WOLFSSH_TEST_INTERNAL
int err;
int tries;
Expand All @@ -2749,23 +2840,80 @@ static void test_wolfSSH_SFTP_SendReadPacket(void)
char wrName[] = "wolfssh_5574_write.tmp";
#endif

current = wolfSSH_SFTP_LS(ssh, (char*)currentDir);
/* 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". The staging is asserted
* rather than skipped on failure; skipping would drop the read
* coverage below without failing the test. Note this makes a
* writable server working directory a prerequisite of the test. */
WMEMSET(rdData, 'a', sizeof(rdData));
/* best effort, the file is normally absent */
(void)sftp_retry_remove(ssh, rdName);
rdRet = WS_FATAL_ERROR;
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,
NULL, rdHandle, &rdHandleSz);
if (rdRet == WS_SUCCESS) {
break;
}
rdErr = wolfSSH_get_error(ssh);
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)) +
SFTP_MAX_RETRY_TRIES;
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 (!sftp_error_keeps_state(rdErr)) {
break; /* unexpected error */
}
}
rdRet = sftp_retry_close(ssh, rdHandle, rdHandleSz);
AssertIntEQ(rdSz, WOLFSSH_MAX_SFTP_RW);
AssertIntEQ(rdRet, WS_SUCCESS);

current = NULL;
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 (!sftp_error_keeps_state(rdErr) && rdErr != WS_REKEYING) {
break;
}
}
AssertNotNull(current);
tmp = current;
while (tmp != NULL) {
if ((tmp->atrb.sz[0] > 0) &&
(tmp->atrb.flags & WOLFSSH_FILEATRB_PERM) &&
!(tmp->atrb.per & 040000)) {
if (tmp->fName != NULL && WSTRCMP(tmp->fName, rdName) == 0) {
break;
}
tmp = tmp->next;
}
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);
Expand All @@ -2779,13 +2927,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;
Expand All @@ -2794,6 +2941,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 */
Expand All @@ -2803,6 +2951,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
Expand Down Expand Up @@ -2840,12 +2989,17 @@ 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);
/* 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);
(void)sftp_retry_remove(ssh, rdName);

#ifdef WOLFSSH_TEST_INTERNAL
/* Issue 5574: exercise the partial-send resume path for
Expand Down Expand Up @@ -2886,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 */
}
Expand Down
Loading