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]

🟠 [High] New WSCPRINTF macro breaks Windows WSTRING_USER builds
🚫 BLOCK

The PR changes the Windows length probe from WSNPRINTF(NULL, 0, ...) to WSCPRINTF(...), but WSCPRINTF is only defined inside #ifndef WSTRING_USER in wolfssh/port.h. A supported Windows build that defines WSTRING_USER and enables SCP will now compile MakeScpCmd with USE_WINDOWS_API and hit an undefined WSCPRINTF macro. This is introduced by the PR because MakeScpCmd previously only depended on the existing WSNPRINTF wrapper.

Recommendation: Provide a WSCPRINTF fallback outside the #ifndef WSTRING_USER block, or avoid adding a new required string-port macro by using a local Windows size-probe helper that is always available for USE_WINDOWS_API builds. Fix the macro availability before merge so custom string-port Windows builds do not regress.

[yosuke-wolfssl]

Thanks for reviewing.
Confirmed. WSCPRINTF was defined at port.h:595, inside the #ifndef WSTRING_USER block that closes at line 637. Since WSTRING_USER means the integrator supplies every string macro themselves, a WSTRING_USER + USE_WINDOWS_API + WOLFSSH_SCP build would hit an undefined WSCPRINTF — and would have no way to know it needed to define it, because before this PR MakeScpCmd only depended on the already-required WSNPRINTF. A genuine build regression introduced here.

Moved the definition out of the block, behind its own guard so it is still overridable.

[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]

🟠 [High] New WSCPRINTF macro breaks Windows WSTRING_USER builds
🚫 BLOCK

The PR changes the Windows length probe from WSNPRINTF(NULL, 0, ...) to WSCPRINTF(...), but WSCPRINTF is only defined inside #ifndef WSTRING_USER in wolfssh/port.h. A supported Windows build that defines WSTRING_USER and enables SCP will now compile MakeScpCmd with USE_WINDOWS_API and hit an undefined WSCPRINTF macro. This is introduced by the PR because MakeScpCmd previously only depended on the existing WSNPRINTF wrapper.

Recommendation: Provide a WSCPRINTF fallback outside the #ifndef WSTRING_USER block, or avoid adding a new required string-port macro by using a local Windows size-probe helper that is always available for USE_WINDOWS_API builds. Fix the macro availability before merge so custom string-port Windows builds do not regress.

[yosuke-wolfssl]

It's duplicated and fixed on above comment

[aidangarske]

🟡 [Medium] Boundary checks reject one valid maximum-length path
💡 SUGGEST

The new WSTRNCPY guards compare against buffer_size - 1 and then pass buffer_size - 1 as the Windows secure-CRT destination size. That avoids the strncpy_s fastfail, but it also rejects strings that exactly fill the destination with one byte left for the terminator. For example, ctx->dirName is DEFAULT_SCP_FILE_NAME_SZ bytes, so a DEFAULT_SCP_FILE_NAME_SZ - 1 byte path should fit, but the new guard returns WS_BUFFER_E. The same off-by-one behavior exists for absolute autopilot paths in fullpath.

Recommendation: Use the real destination size in the WSTRNCPY call and guard against >= sizeof(destination), then add a boundary test for an input whose length is exactly sizeof(destination) - 1. Adjust the copy sizes so the Windows hardening does not reduce the accepted maximum path length.

[yosuke-wolfssl]

Confirmed, and it's a behavior regression on POSIX too, not only Windows. Both guards compared against size - 1 while the destination is explicitly NUL-terminated, so they rejected a string that previously fit exactly.
Both sites now pass the true destination size to WSTRNCPY and guard on >= sizeof(dest).

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

[yosuke-wolfssl]

Hello @aidangarske ,
Could you please review this again ?