From e74fd7147378656982fa7fba49047aadf3b271f0 Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Thu, 9 Apr 2026 11:54:58 -0700 Subject: [PATCH 1/3] Fix underflow in sftp example with empty args --- examples/sftpclient/sftpclient.c | 16 ++++++++-------- tests/sftp.c | 10 ++++++++++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/examples/sftpclient/sftpclient.c b/examples/sftpclient/sftpclient.c index be4a3687b..800b0ea85 100644 --- a/examples/sftpclient/sftpclient.c +++ b/examples/sftpclient/sftpclient.c @@ -472,7 +472,7 @@ static int doCmds(func_args* args) pt += sizeof("mkdir"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; if (pt[0] != '/') { int maxSz = (int)WSTRLEN(workingDir) + sz + 2; f = (char*)WMALLOC(maxSz, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -522,7 +522,7 @@ static int doCmds(func_args* args) pt += sizeof("get"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; /* search for space delimiter */ to = pt; @@ -633,7 +633,7 @@ static int doCmds(func_args* args) pt += sizeof("put"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; to = pt; for (i = 0; i < sz; i++) { @@ -739,7 +739,7 @@ static int doCmds(func_args* args) pt += sizeof("cd"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; if (pt[0] != '/') { int maxSz = (int)WSTRLEN(workingDir) + sz + 2; f = (char*)WMALLOC(maxSz, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -803,7 +803,7 @@ static int doCmds(func_args* args) pt += sizeof("chmod"); sz = (word32)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; /* advance pointer to first location of non space character */ for (idx = 0; idx < sz && pt[0] == ' '; idx++, pt++); @@ -885,7 +885,7 @@ static int doCmds(func_args* args) pt += sizeof("rmdir"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; if (pt[0] != '/') { int maxSz = (int)WSTRLEN(workingDir) + sz + 2; f = (char*)WMALLOC(maxSz, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -942,7 +942,7 @@ static int doCmds(func_args* args) pt += sizeof("rm"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; if (pt[0] != '/') { int maxSz = (int)WSTRLEN(workingDir) + sz + 2; f = (char*)WMALLOC(maxSz, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -997,7 +997,7 @@ static int doCmds(func_args* args) pt += sizeof("rename"); sz = (int)WSTRLEN(pt); - if (pt[sz - 1] == '\n') pt[sz - 1] = '\0'; + if (sz > 0 && pt[sz - 1] == '\n') pt[sz - 1] = '\0'; /* search for space delimiter */ to = pt; diff --git a/tests/sftp.c b/tests/sftp.c index 1a9c74c4d..29d648d16 100644 --- a/tests/sftp.c +++ b/tests/sftp.c @@ -68,6 +68,16 @@ static const char* cmds[] = { "chmod 600 test-get-2", "rm test-get-2", "ls -s", + /* empty arg tests: trigger pt[sz-1] underflow if sz == 0 */ + "mkdir", + "cd", + "ls", + "chmod", + "rmdir", + "rm", + "rename", + "get", + "put", "exit" }; static int commandIdx = 0; From d854f035704d1cf5f863e6675dd96990f02c0da3 Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Thu, 9 Apr 2026 12:06:16 -0700 Subject: [PATCH 2/3] Move sftp test cases into a table for stronger linkage to expected behavior --- tests/sftp.c | 231 +++++++++++++++++++++++++++++---------------------- 1 file changed, 130 insertions(+), 101 deletions(-) diff --git a/tests/sftp.c b/tests/sftp.c index 29d648d16..559a58bee 100644 --- a/tests/sftp.c +++ b/tests/sftp.c @@ -43,118 +43,139 @@ #include "examples/echoserver/echoserver.h" #include "examples/sftpclient/sftpclient.h" -static const char* cmds[] = { - "mkdir a", - "cd a", - "pwd", - "ls", -#ifdef WOLFSSH_ZEPHYR - "put " CONFIG_WOLFSSH_SFTP_DEFAULT_DIR "/configure.ac", -#else - "put configure.ac", -#endif - "ls", -#ifdef WOLFSSH_ZEPHYR - "get configure.ac " CONFIG_WOLFSSH_SFTP_DEFAULT_DIR "/test-get", -#else - "get configure.ac test-get", -#endif - "rm configure.ac", - "cd ../", - "ls", - "rename test-get test-get-2", - "rmdir a", - "ls", - "chmod 600 test-get-2", - "rm test-get-2", - "ls -s", - /* empty arg tests: trigger pt[sz-1] underflow if sz == 0 */ - "mkdir", - "cd", - "ls", - "chmod", - "rmdir", - "rm", - "rename", - "get", - "put", - "exit" -}; -static int commandIdx = 0; +/* + * Each test command is paired with an optional check function that + * validates the output it produces. This eliminates the fragile + * index-based coupling between the command array and the validator. + */ +typedef int (*SftpTestCheck)(void); + +typedef struct { + const char* cmd; + SftpTestCheck check; /* validates output from THIS cmd, or NULL */ +} SftpTestCmd; + +/* Test buffer */ static char inBuf[1024] = {0}; -/* return 0 on success */ -static int Expected(int command) +/* check that pwd output ends in /a */ +static int checkPwdInA(void) { int i; + int len; - switch (command) { - case 3: /* pwd */ - /* working directory should not contain a '.' at the end */ - for (i = 0; i < (int)sizeof(inBuf); i++) { - if (inBuf[i] == '\n') { - inBuf[i] = '\0'; - break; - } - } - - if (inBuf[WSTRLEN(inBuf) - 2] != '/') { - printf("unexpected pwd of %s\n looking for '/'\n", inBuf); - return -1; - } - if (inBuf[WSTRLEN(inBuf) - 1] != 'a') { - printf("unexpected pwd of %s\n looking for 'a'\n", inBuf); - return -1; - } + for (i = 0; i < (int)sizeof(inBuf); i++) { + if (inBuf[i] == '\n') { + inBuf[i] = '\0'; break; + } + } - case 4: - { + len = (int)WSTRLEN(inBuf); + if (len < 2) { + printf("pwd output too short: %s\n", inBuf); + return -1; + } + if (inBuf[len - 2] != '/') { + printf("unexpected pwd of %s, looking for '/'\n", inBuf); + return -1; + } + if (inBuf[len - 1] != 'a') { + printf("unexpected pwd of %s, looking for 'a'\n", inBuf); + return -1; + } + return 0; +} + +/* check that ls of empty dir shows only . and .. */ +static int checkLsEmpty(void) +{ #ifdef WOLFSSH_ZEPHYR - /* No . and .. in zephyr fs API */ - char expt1[] = "wolfSSH sftp> "; - char expt2[] = "wolfSSH sftp> "; + /* No . and .. in zephyr fs API */ + char expt1[] = "wolfSSH sftp> "; + char expt2[] = "wolfSSH sftp> "; #else - char expt1[] = ".\n..\nwolfSSH sftp> "; - char expt2[] = "..\n.\nwolfSSH sftp> "; + char expt1[] = ".\n..\nwolfSSH sftp> "; + char expt2[] = "..\n.\nwolfSSH sftp> "; #endif - if (WMEMCMP(expt1, inBuf, sizeof(expt1)) != 0 && - WMEMCMP(expt2, inBuf, sizeof(expt2)) != 0) { - printf("unexpected ls\n"); - printf("\texpected \n%s\n\tor\n%s\n\tbut got\n%s\n", expt1, - expt2, inBuf); - return -1; - } - else - return 0; - - } - - case 6: - if (WSTRNSTR(inBuf, "configure.ac", sizeof(inBuf)) == NULL) { - fprintf(stderr, "configure.ac not found in %s\n", inBuf); - return 1; - } - else { - return 0; - } + if (WMEMCMP(expt1, inBuf, sizeof(expt1)) != 0 && + WMEMCMP(expt2, inBuf, sizeof(expt2)) != 0) { + printf("unexpected ls\n"); + printf("\texpected \n%s\n\tor\n%s\n\tbut got\n%s\n", + expt1, expt2, inBuf); + return -1; + } + return 0; +} - case 10: - return (WSTRNSTR(inBuf, "test-get", sizeof(inBuf)) == NULL); +/* check that ls output contains a specific file */ +static int checkLsHasConfigureAc(void) +{ + if (WSTRNSTR(inBuf, "configure.ac", sizeof(inBuf)) == NULL) { + fprintf(stderr, "configure.ac not found in %s\n", inBuf); + return 1; + } + return 0; +} - case 13: - return (WSTRNSTR(inBuf, "test-get-2", sizeof(inBuf)) == NULL); +static int checkLsHasTestGet(void) +{ + return (WSTRNSTR(inBuf, "test-get", + sizeof(inBuf)) == NULL) ? 1 : 0; +} - case 16: - return (WSTRNSTR(inBuf, "size in bytes", sizeof(inBuf)) == NULL); +static int checkLsHasTestGet2(void) +{ + return (WSTRNSTR(inBuf, "test-get-2", + sizeof(inBuf)) == NULL) ? 1 : 0; +} - default: - break; - } - WMEMSET(inBuf, 0, sizeof(inBuf)); - return 0; +static int checkLsSize(void) +{ + return (WSTRNSTR(inBuf, "size in bytes", + sizeof(inBuf)) == NULL) ? 1 : 0; } +static const SftpTestCmd cmds[] = { + { "mkdir a", NULL }, + { "cd a", NULL }, + { "pwd", checkPwdInA }, + { "ls", checkLsEmpty }, +#ifdef WOLFSSH_ZEPHYR + { "put " CONFIG_WOLFSSH_SFTP_DEFAULT_DIR "/configure.ac", NULL }, +#else + { "put configure.ac", NULL }, +#endif + { "ls", checkLsHasConfigureAc }, +#ifdef WOLFSSH_ZEPHYR + { "get configure.ac " + CONFIG_WOLFSSH_SFTP_DEFAULT_DIR "/test-get", NULL }, +#else + { "get configure.ac test-get", NULL }, +#endif + { "rm configure.ac", NULL }, + { "cd ../", NULL }, + { "ls", checkLsHasTestGet }, + { "rename test-get test-get-2", NULL }, + { "rmdir a", NULL }, + { "ls", checkLsHasTestGet2 }, + { "chmod 600 test-get-2", NULL }, + { "rm test-get-2", NULL }, + { "ls -s", checkLsSize }, + /* empty arg tests: must not underflow on pt[sz-1] */ + { "mkdir", NULL }, + { "cd", NULL }, + { "ls", NULL }, + { "chmod", NULL }, + { "rmdir", NULL }, + { "rm", NULL }, + { "rename", NULL }, + { "get", NULL }, + { "put", NULL }, + { "exit", NULL }, +}; +static int commandIdx = 0; + static int commandCb(const char* in, char* out, int outSz) { @@ -167,18 +188,26 @@ static int commandCb(const char* in, char* out, int outSz) /* get command input */ if (out) { - int sz = (int)WSTRLEN(cmds[commandIdx]); + int sz = (int)WSTRLEN(cmds[commandIdx].cmd); if (outSz < sz) { ret = -1; } else { - WMEMCPY(out, cmds[commandIdx], sz); + WMEMCPY(out, cmds[commandIdx].cmd, sz); } - if (Expected(commandIdx) != 0) { - fprintf(stderr, "Failed on command index %d\n", commandIdx); - exit(1); /* abort out */ + /* validate output from the previous command */ + if (commandIdx > 0 && + cmds[commandIdx - 1].check != NULL) { + if (cmds[commandIdx - 1].check() != 0) { + fprintf(stderr, + "Check failed for \"%s\" (index %d)\n", + cmds[commandIdx - 1].cmd, + commandIdx - 1); + exit(1); + } } + WMEMSET(inBuf, 0, sizeof(inBuf)); commandIdx++; } return ret; From 94b82715a4c4887ffc73f9789e90bcd7fad22eb6 Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Thu, 9 Apr 2026 14:41:19 -0700 Subject: [PATCH 3/3] Cleanup sftp test artifacts at start of test --- tests/sftp.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/sftp.c b/tests/sftp.c index 559a58bee..0066c1776 100644 --- a/tests/sftp.c +++ b/tests/sftp.c @@ -137,6 +137,20 @@ static int checkLsSize(void) } static const SftpTestCmd cmds[] = { + /* If a prior run was interrupted, files and directories + * created during the test may still exist in the working + * directory, causing mkdir to fail and ls checks to see + * unexpected entries. Remove them here before starting. + * These run as SFTP commands rather than local syscalls + * so they are portable across all platforms (Windows, + * Zephyr, POSIX). Failures are silently ignored since + * the files may not exist. */ + { "rm a/configure.ac", NULL }, + { "rmdir a", NULL }, + { "rm test-get", NULL }, + { "rm test-get-2", NULL }, + + /* --- test sequence starts here --- */ { "mkdir a", NULL }, { "cd a", NULL }, { "pwd", checkPwdInA },