Run threaded api-test SFTP/SCP tests on Windows

[yosuke-wolfssl]

Summary

Enables the threaded echoserver+client SFTP/SCP integration tests in api-test on Windows CI (MSVC + AddressSanitizer). These tests were previously stubbed out on Windows because the readiness handshake was POSIX-only, so a whole class of SFTP/SCP coverage never ran there. Running them surfaced several Windows-only bugs in wolfSSH, which this PR also fixes.

Net change is small (7 files, +105/−32) — most of the work was diagnosis.

What changed

Test harness / CI

• windows-check.yml — fix the user_settings.h #if 0→#if 1 flip so SSHD/SFTP/SCP/CERTS/X509 are actually compiled and tested (they were silently stubbing to { ; }).
• wolfssh/test.h — implement the Win32 readiness handshake for tcp_ready (a CRITICAL_SECTION + manual-reset event, with Init/Free/Wait/SignalTcpReady Win32 branches). WaitForSingleObject is bounded by a 30s timeout so a stuck server can't hang CI. This lets the threaded tests run instead of being skipped.
• tests/api.c — drop the !USE_WINDOWS_API guards that stubbed the threaded SFTP/SCP tests; pair InitTcpReady/FreeTcpReady; use an ephemeral port (-p 0) on Windows like POSIX, so concurrent CI runs don't collide on a fixed port.

wolfSSH Windows fixes uncovered by the tests

• MakeScpCmd (src/wolfscp.c) — the length-probe used WSNPRINTF(NULL, 0, ...), but on Windows that maps to _snprintf_s, which rejects the NULL/0 form and aborts via __fastfail. Use _scprintf through a new Windows-only WSCPRINTF wrapper (wolfssh/port.h) for the length measurement.
• WSTRNCPY call-site hardening — on Windows WSTRNCPY(s1,s2,n) maps to strncpy_s((s1),(n),(s2),(n)), which __fastfails when strlen(src) >= n. Audited every src/ and example call site; fixed the unguarded ones:
  • ScpCheckForRename — pass the real buffer size (sizeof(buf)) rather than a copy count.
  • ScpPushDir — add an upstream length guard returning WS_BUFFER_E.
  • echoserver.c / sftpclient.c — guard the user/server-supplied string copies (skip or return on over-long input).
• SFTP rekey-test shutdown flake (tests/api.c) — on Windows the post-transfer close races the client's graceful wolfSSH_shutdown; Winsock raises an RST that the client sees on the recv path. There, wolfSSH follows its convention of returning the generic WS_ERROR while stashing the specific code in ssh->error. The test only absorbed the direct WS_SOCKET_ERROR_E return (the send-path manifestation), so the recv-path reset slipped through ~30–40% of runs. The fix absorbs the benign reset via wolfSSH_get_error(ssh) == WS_SOCKET_ERROR_E, gated on argsCount == WS_ERROR so a genuine error with a different ssh->error still fails — no masking.

Verification

• Windows (winvs Release + MSVC AddressSanitizer, run from repo root): api-test + unit-test pass 15/15 (was ~30–40% flaky at the SFTP-rekey shutdown).
• POSIX (macOS): api-test + testsuite clean under -fsanitize=address,undefined.

Notes

• No change to the WSTRNCPY macro mapping — fixes are at the call sites only.
• Win32 sync primitives (CreateEvent, EnterCriticalSection, WaitForSingleObject) are used directly, consistent with existing USE_WINDOWS_API code in the repo.

Supersedes #1030 (which this branch already contains in full; #1058 also un-stubs the threaded tests that #1030 had excluded).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1058

Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1

Low (1)

Confinement test shutdown check not hardened for Windows peer-gone path

File: tests/api.c:2506
Function: test_wolfSSH_SFTP_Confinement
Category: Incorrect error handling

This PR enables test_wolfSSH_SFTP_Confinement on Windows but its shutdown check only converts WS_SOCKET_ERROR_E, not the generic WS_ERROR with wolfSSH_get_error()==WS_SOCKET_ERROR_E that the recv path returns on Windows. Every sibling Windows-enabled test (1710, 1990, 2130, 3673) was hardened for this; this one fails AssertIntEQ(ret, WS_SUCCESS) on a benign peer reset.

Recommendation: Accept ret == WS_ERROR && wolfSSH_get_error(ssh) == WS_SOCKET_ERROR_E here, matching the sibling tests.

Referenced code: tests/api.c:2506-2509 (4 lines)

---

This review was generated automatically by Fenrir. Findings are non-blocking.

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

• [High] New WSCPRINTF macro breaks Windows WSTRING_USER builds — src/wolfscp.c:1908
• [Medium] Boundary checks reject one valid maximum-length path — src/wolfscp.c:2594; examples/sftpclient/sftpclient.c:1467

Review generated by Skoll.

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

• [High] New WSCPRINTF macro breaks Windows WSTRING_USER builds — src/wolfscp.c:1908
• [Medium] Boundary checks reject one valid maximum-length path — src/wolfscp.c:2594; examples/sftpclient/sftpclient.c:1467

Review generated by Skoll.

Superseded by corrected review (findings now attached as inline comments).

[yosuke-wolfssl]

Hello @aidangarske ,
Could you please review this again ?

[aidangarske]

Skoll Multi-Scan Review

Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 8 total — 8 posted, 0 skipped
8 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] [review] AbsorbBenignReset widens shutdown tolerance beyond peer-reset — tests/api.c:1508-1517
• [Medium] [review] No test coverage for the new input-validation reject paths — src/wolfscp.c:2675-2677
• [Low] [review+review-security+bugs] myStatusCb early return permanently suppresses all progress output for over-long file names — examples/sftpclient/sftpclient.c:180-190
• [Low] [review+review-security+bugs] WaitTcpReady Win32 path discards WaitForSingleObject result, turning a server-startup failure into a 30s stall plus a misleading connect failure — wolfssh/test.h:920-924
• [Low] [review+bugs] CRITICAL_SECTION in tcp_ready is never acquired by any reader — wolfssh/test.h:829-831
• [Low] [review] MakeScpCmd now repeats the "scp -%c %s" format string three times — src/wolfscp.c:1911-1923
• [Low] [review] Leftover WOLFSSH_MAX_FILENAME memsets inconsistent with new sizeof(currentFile) — examples/sftpclient/sftpclient.c:198
• [Low] [review] Over-long entry guard copy-pasted three times with a non-identifying message — examples/echoserver/echoserver.c:2263-2267

Review generated by Skoll

[aidangarske]

🟠 [Medium] AbsorbBenignReset widens shutdown tolerance beyond peer-reset · Test Quality

Verified against the tree. The helper absorbs ret == WS_ERROR && wolfSSH_get_error(ssh) == WS_SOCKET_ERROR_E in addition to the direct WS_SOCKET_ERROR_E return the old code handled. Confirmed that WS_ERROR and WS_FATAL_ERROR are the same value (-1001, wolfssh/error.h:42-43), so the ret == WS_ERROR gate matches every generic wolfSSH failure return, not just the recv path. WS_SOCKET_ERROR_E (-1009) is wolfSSH's generic socket-layer failure code — it is not specific to "peer sent RST". So a server that dies mid-shutdown for a genuine reason (rather than the benign post-transfer race this targets) now also reports WS_SUCCESS across all five call sites. This is a deliberate and reasonable trade (the old code already absorbed the send-path form, and the PR documents the ~30-40% flake it fixes), but the tests can no longer distinguish a benign reset from a real socket failure at shutdown, and the widening applies uniformly to SendReadPacket / PartialSend / rekey / Confinement / KeyboardInteractive.

Fix: Consider gating on the underlying WSAGetLastError() == WSAECONNRESET / errno == ECONNRESET so a genuine socket failure at shutdown still fails the test. If that proves too brittle across the platforms api-test runs on, keep the current form but note the limitation in the helper comment.

[aidangarske]

🟠 [Medium] No test coverage for the new input-validation reject paths · Test Coverage

Verified against the tree. The PR adds four new bounds rejects — ScpPushDir -> WS_BUFFER_E (wolfscp.c:2676), doAutopilot -> WS_BUFFER_E (sftpclient.c:1471), the three echoserver LoadPasswdList/LoadKeyboardList/LoadPubKeyList skip-and-continue guards (echoserver.c:2263, 2297, 2338), and the myStatusCb name guard — and none of them gain a test. The stated verification is that the pre-existing Windows suite stops crashing, which exercises only the under-limit path in every case; the new over-limit branches are entirely uncovered on both POSIX and Windows. ScpPushDir is the one worth covering: it is a genuine behavior change (was silently truncating at DEFAULT_SCP_FILE_NAME_SZ - 1 and returning WS_SUCCESS, now rejects at >= 1024 — confirmed dirName[DEFAULT_SCP_FILE_NAME_SZ] at wolfscp.h:95 and the 1024 define at wolfscp.h:47), and its three callers map the failure to three different outcomes — WS_INVALID_PATH_E/WS_FATAL_ERROR (wolfscp.c:1383), WS_SCP_ABORT (wolfscp.c:2965), and a bare log (wolfscp.c:3243). Confirmed ScpPushDir is forward-declared static at wolfscp.c:56 (the definition at line 2667 omits static, but the prior internal-linkage declaration is visible, so linkage stays internal) — it cannot be unit-tested directly, and coverage would need an integration case driving a >1023-char base path through wolfSSH_SCP_to.

Fix: At minimum cover the ScpPushDir over-long-path reject, since it changes library behavior for all three callers. The example-program guards (echoserver/sftpclient) are lower value and can reasonably be left uncovered.

[aidangarske]

🔵 [Low] myStatusCb early return permanently suppresses all progress output for over-long file names · Logic

All three scan modes independently confirmed this against the tree. The new length guard sits AFTER the state reset, so when nameSz >= sizeof(currentFile) (257, since currentFile is static char currentFile[WOLFSSH_MAX_FILENAME + 1] at sftpclient.c:104 and WOLFSSH_MAX_FILENAME is 256 per wolfssh/ssh.h:572) the callback has already zeroed currentFile and reset startTime/lastOutputTime/lastPrintedBytes before it bails. Because currentFile is left empty, the next call's WSTRNCMP(currentFile, name, WSTRLEN(name)) still mismatches, so every subsequent call re-enters the reset branch and returns again — progress reporting is permanently suppressed for the entire transfer, and the WSNPRINTF/WFPUTS print at lines 192-207 is never reached. Previously on POSIX WSTRNCPY(currentFile, name, WOLFSSH_MAX_FILENAME) truncated the name into the buffer and fell through to print, so this is a regression on POSIX. On Windows the old WSTRNCPY expanded to strncpy_s(currentFile, 256, name, 256), a constraint violation that __fastfails for strlen(name) >= 256 — the guard genuinely fixes that crash, so the direction is right; the issue is that it trades a crash for silence rather than clamping. The guard itself is buffer-correct: nameSz == 256 copies 257 bytes and fits exactly, no off-by-one. Triggering input is ordinary: statusCb receives the caller-supplied local path from wolfSSH_SFTP_Get/wolfSSH_SFTP_Put, and POSIX PATH_MAX is 4096. Severity views differed — the review mode rated this Medium/SUGGEST, review-security and bugs rated it Low; the…

Fix: Clamp nameSz to sizeof(currentFile) - 1 and fall through instead of returning. This keeps the strncpy_s-safe property the PR is after, populates currentFile so the name-change branch stops re-triggering, and preserves the pre-existing truncate-and-print behavior on POSIX. It does not address the pre-existing dead timeout for long names — to fix that too, compare over the stored length (e.g. WSTRNCMP(currentFile, name, WSTRLEN(currentFile))) or track nameSz in a static alongside currentFile.

[aidangarske]

🔵 [Low] WaitTcpReady Win32 path discards WaitForSingleObject result, turning a server-startup failure into a 30s stall plus a… · Error Handling

All three scan modes independently confirmed this against the tree (TCP_READY_TIMEOUT_MS is 30000 at wolfssh/test.h:860). The Win32 branch calls WaitForSingleObject(ready->readyEvent, TCP_READY_TIMEOUT_MS) and discards the DWORD result, and unlike the POSIX branch directly above (test.h:905-913, while (!ready->ready) pthread_cond_wait(...)) it never inspects the ready->ready predicate — WAIT_OBJECT_0, WAIT_TIMEOUT and WAIT_FAILED are indistinguishable to the caller. Triggering condition: SignalTcpReady is invoked from exactly one site (echoserver.c:3469, immediately after tcp_listen()), so every ES_ERROR path before that line expands to WOLFSSL_RETURN_FROM_THREAD(0) and exits the server thread WITHOUT signalling the manual-reset event created at test.h:875. Reachable examples on the Windows api-test build (ide/winvs/api-test/api-test.vcxproj defines NO_MAIN_DRIVER project-wide, so the whole Win32 handshake is live): -p with a NULL myoptarg, a host-key load failure, or -R under NO_FILESYSTEM. In all of those WaitTcpReady stalls the full 30s then returns exactly as if the server were up; ready.port is still 0 (set in InitTcpReady, test.h:866), the client connects to 127.0.0.1:0, connect() fails, and AssertNotNull(ctx) from sftp_client_connect(&ctx, &ssh, 0) aborts. The in-code claim that the caller "fails fast" is only half true: the run aborts rather than hangs, but only after burning 30s per affected test (up to six call sites in tests/api.c -> ~3 minutes), and the operator sees ctx => NULL — a connect failure pointing at the…

Fix: Check for WAIT_OBJECT_0 and the ready->ready predicate, and call err_sys() otherwise. err_sys is WS_NORETURN/exit() on Windows (test.h:299-315) and is the established convention in this header — it already handles CreateEvent failure the same way at test.h:876-879 — so this keeps the fail-fast intent, makes the cause visible at the point it happens, avoids the racy ready.port read, and realigns the Win32 branch with the POSIX predicate loop. If aborting inside the harness is undesirable, at minimum print the wait result and drop the 'fails fast' wording from the comment, since the observed behaviour is a 30s stall followed by an unrelated-looking assertion.

[aidangarske]

🔵 [Low] CRITICAL_SECTION in tcp_ready is never acquired by any reader · Dead Code

Verified against the tree; the bugs mode independently noted the same thing as a related observation on its WaitTcpReady finding ("ready->ready is declared as the predicate (test.h:823) and is written under cs by the signalling side, but is never read by WaitTcpReady, so the predicate the CRITICAL_SECTION protects has no reader on this path"). The new Win32 tcp_ready carries both a CRITICAL_SECTION cs and a manual-reset readyEvent, but only the writer (SignalTcpReady in echoserver.c:2861-2865) ever enters the CS. WaitTcpReady waits solely on the event (test.h:924) and never enters cs nor reads the ready->ready predicate; callers read ready.port directly with no lock. SetEvent/WaitForSingleObject are already full barriers, so they alone order the port write ahead of the reader's load — the CS protects nothing and adds InitializeCriticalSection/DeleteCriticalSection lifecycle (test.h:874-877, 896) to every call site. This is a faithful transliteration of the POSIX mutex+cond shape, but a Win32 event needs no companion lock for a one-shot signal. Note the event handle still needs FreeTcpReady, so dropping cs does not remove the Init/Free pairing — it just removes dead state. Also flagging the asymmetry: the struct members are guarded by #elif defined(USE_WINDOWS_API) alone while every function that touches them additionally requires NO_MAIN_DRIVER && !SINGLE_THREADED. This is harmless (the members are simply unused in those builds) and mirrors the pre-existing POSIX asymmetry, but tightening the struct guard to match would prevent a…

Fix: Drop cs and the EnterCriticalSection/LeaveCriticalSection pair in SignalTcpReady; SetEvent already publishes the port write. If the CS is kept deliberately, have WaitTcpReady actually take it and check ready->ready so the lock has a reader — which would also fix the predicate gap in the WaitTcpReady finding above.

[aidangarske]

🔵 [Low] MakeScpCmd now repeats the "scp -%c %s" format string three times · Maintainability

Verified against the tree. Adding the Windows WSCPRINTF branch brings the format-string count in this one function to three: the Windows length probe, the POSIX length probe, and the actual write. The probe result sizes the WMALLOC, so any future divergence between a probe copy and the write copy silently becomes either a heap overflow or a truncated command. The two probes are also mutually exclusive at compile time, so no build ever cross-checks them against each other. The current logic is correct — _scprintf returns chars-excluding-NUL (-1 on error, caught by sz <= 0), and the subsequent _snprintf_s(cmd, sz, _TRUNCATE, ...) writes at most sz-1 chars plus NUL, which fits exactly — but the duplication is the fragile part.

Fix: Hoist the format into a file-local #define so the probe and the write cannot drift. Use a macro rather than a const char* local — MSVC emits C4774 for non-literal format strings passed to the CRT _s/_scprintf family.

[aidangarske]

🔵 [Low] Over-long entry guard copy-pasted three times with a non-identifying message · Maintainability

Verified against the tree — the same five-line guard appears verbatim at lines 2263, 2297 and 2338. The bound is correct: names[256], guard rejects len >= 255, and WSTRNCPY(names, strList->str, sizeof names - 1) maps to strncpy_s(names, 255, src, 255), which __fastfails exactly when strlen(src) >= 255, so the guard covers precisely the crashing set. Two small polish points: the message "Ignoring over-long entry\n" does not say which entry, unlike the adjacent fprintf(stderr, "Ignoring password: %s\n", names) a few lines down; and the guard's sizeof names - 1 must stay in lockstep with the WSTRNCPY count argument or the protection silently lapses. (The skipped entries also no longer bump count, but every caller at lines 3204-3249 discards the return value, so that is a non-issue.)

Fix: Print a truncated prefix of the offending entry so the operator can identify it. Optionally factor the guard into a small CopyListEntry(names, sizeof names, strList->str) helper shared by the three loaders.

[aidangarske]

🔵 [Low] Leftover WOLFSSH_MAX_FILENAME memsets inconsistent with new sizeof(currentFile) · Consistency

Verified against the tree. Line 185 was updated to WMEMSET(currentFile, 0, sizeof(currentFile)), but lines 198, 674 and 785 still use WMEMSET(currentFile, 0, WOLFSSH_MAX_FILENAME). currentFile is declared char currentFile[WOLFSSH_MAX_FILENAME + 1] (line 102), so those three leave the final byte untouched. This is harmless today — after the new WMEMCPY(currentFile, name, nameSz + 1) with nameSz == 256, byte [256] is the NUL, so a 256-byte memset still yields an empty string — but the mismatch is precisely the off-by-one class this PR set out to eliminate, and it now reads as an oversight next to the updated line 185.

Fix: Switch lines 198, 674 and 785 to sizeof(currentFile) for consistency with line 185.

---

Note: Referenced line (examples/sftpclient/sftpclient.c:198) is outside the diff hunk. Comment anchored to nearest changed region.