F8827 win sftp followup - #1238
Closed
miyazakh wants to merge 6 commits into
Closed
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1238
No scan targets match the changed files in this PR. Review skipped.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes appear correct and scoped to CI/test coverage improvements, with only a minor comment-accuracy nit identified.
Pull request overview
Adds Windows CI coverage for previously untested Windows-only SFTP paths and for wolfsshd -D argument parsing behavior, closing remaining CI gaps from PR #1173 follow-ups.
Changes:
- Adds a PowerShell regression test that validates
wolfsshd -Don Windows correctly honors-fand-p. - Extends the Windows GitHub Actions workflow with (1) a new step to run the
-Dregression test and (2) a new MSYS2 MinGW64 job to build and runtests/regress.test+tests/unit.testwithUSE_WINDOWS_APIenabled.
File summaries
| File | Description |
|---|---|
| apps/wolfsshd/test/sshd_dash_d_test.ps1 | New Windows PowerShell test to validate wolfsshd -D foreground-mode option parsing (-f/-p). |
| .github/workflows/windows-check.yml | Runs the new PowerShell test in the Windows build job and adds an MSYS2 MinGW64 regression job to execute SFTP regression/unit tests on Windows. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+282
to
+283
| # autoreconf directly rather than ./autogen.sh: the latter forces | ||
| # -Werror on autotools warnings, which the MSYS2 automake can trip on. |
Contributor
Author
There was a problem hiding this comment.
You're right on the facts. autogen.sh does export WARNINGS="all,error" when a .git directory is present (which is the case for a CI checkout), and automake / autoconf honor that environment variable, so warnings become hard errors. Calling autoreconf -ivf directly just skips that env var setup.
The existing comment isn't wrong — "-Werror on autotools warnings" is a shorthand for exactly that WARNINGS=all,error behavior — but naming the actual mechanism makes the rationale clearer for future maintainers. I'll tighten it to:
# Run autoreconf directly instead of ./autogen.sh: for a git checkout
# autogen.sh exports WARNINGS="all,error", turning autotools warnings into
# errors that the MSYS2 automake can trip on.
miyazakh
force-pushed
the
f8827_winSFTP_followup
branch
from
September 3, 2026 17:14
a3218d2 to
a731167
Compare
tests/regress.c holds the only coverage for the Windows SFTP open path, including TestSftpWindowsOpenFlagMatrix, which walks the RecvOpen CREAT/EXCL/TRUNC/APPEND matrix against the CreateFile() disposition table. That test is guarded by USE_WINDOWS_API and ran in no CI job. The MSVC solution in ide/winvs has no regress project, and regress.c does not build with cl because it includes arpa/inet.h and unistd.h, so the disposition fix it locks down could regress unnoticed. Add an MSYS2 MinGW64 job to the Windows workflow. MinGW defines _WIN32, so wolfssh/settings.h turns on USE_WINDOWS_API and the Windows-only branches compile and run. The job builds wolfSSL static, configures wolfSSH with --enable-sftp, and runs tests/regress.test and tests/unit.test. wolfsshd is left out: its autotools path is not MinGW-clean and the MSVC solution already covers it.
The Windows StartSSHD() path rebuilds argv from GetCommandLineW(). A regression there left -D foreground mode walking the raw wide command line, so -f and -p were ignored and the daemon used its built-in defaults. Nothing in CI caught that. Add sshd_dash_d_test.ps1: it starts wolfsshd with -D and a config file at a non-default path whose Port line differs from the -p value, then checks the listener binds the -p port and not the config port. That holds only when -D mode parsed both -f and -p. Run it from the Windows build job next to the existing LoginGraceTime check.
miyazakh
force-pushed
the
f8827_winSFTP_followup
branch
from
September 3, 2026 19:15
a731167 to
e2fb60b
Compare
The mingw-regress job's wolfssh configure step fails with "libwolfssl is required for wolfssh" even though libwolfssl.a is installed at the expected path. AC_CHECK_LIB only reports pass/fail; dump config.log on failure to see the actual link error.
…build config.log from a failing run showed the wolfCrypt_Init AC_CHECK_LIB probe pulling in ssl.c/internal.c/wolfio.c from the static libwolfssl.a, leaving Winsock (socket, send, recv, inet_pton, ...) and cert store (CertOpenSystemStoreA, ...) symbols unresolved. A shared build would defer that resolution to the DLL; the static archive here needs ws2_32 and crypt32 passed explicitly via LIBS.
…inGW The new run showed the ws2_32/crypt32 link fix worked (configure passed) but make then failed: "No rule to make target 'tests/regress.test'." Reproducing the autotools build locally confirmed why: MinGW's EXEEXT is ".exe", so automake's check_PROGRAMS rule names the binaries tests/regress.test.exe and tests/unit.test.exe, not the extension-less names this job was asking make to build and run.
MinGW-w64 does not ship arpa/inet.h; guard tests/regress.c's include the same way apps/wolfsshd/auth.c already does, since htonl/ntohl end up declared via the winsock2.h wolfSSL's headers pull in later in the same translation unit. wolfssh/test.h guarded its MSVC #pragma warning(disable:4996) with USE_WINDOWS_API alone, which is also true for MinGW's gcc; gcc treats the unrecognized pragma as an error under -Werror. Require _MSC_VER too, matching the existing ALIGN16 pragma guard in wolfssh/internal.h. wolfSSH_CleanPath's Windows/Nucleus drive-letter cleanup re-declared `i` in a nested scope, shadowing the function's own `i` used by every other loop in the function. Hoist `j` to the function's declarations (guarded by the same #if so non-Windows/Nucleus builds don't get an unused-variable warning) and drop the now-unnecessary block so the loop reuses the outer `i`. Verified locally: autoreconf + configure + make tests/regress.test tests/unit.test builds clean and both binaries pass on Linux.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Windows SFTP / wolfsshd follow-up: CI coverage for PR #1173
Follow-up to the merged PR #1173. A Skoll review of that PR raised no HIGH or Critical findings. The MEDIUM/INFO items about the code itself were already handled on master by commits
5a1ff71aandea530fe0. The one gap left was test coverage: the Windows-only paths those commits touched run in no CI job.Changes
Run the SFTP regression tests on Windows under MSYS2 (
79e4ec98) — adds amingw-regressjob towindows-check.yml. MinGW defines_WIN32, soUSE_WINDOWS_APIis on andtests/regress.c(includingTestSftpWindowsOpenFlagMatrix, which walks theRecvOpenCREAT/EXCL/TRUNC/APPEND matrix against theCreateFile()disposition table) compiles and runs. The MSVC solution has noregressproject andregress.cdoes not build withcl(arpa/inet.h,unistd.h). The job builds wolfSSL static, configures wolfSSH with--enable-sftp, and runstests/regress.testandtests/unit.test. wolfsshd is left out: its autotools path is not MinGW-clean and the MSVC solution already covers it.Cover wolfsshd
-Doption parsing on Windows in CI (a3218d2c) — addsapps/wolfsshd/test/sshd_dash_d_test.ps1, run from the Windows build job. It startswolfsshd -Dwith a config file at a non-default path whosePortdiffers from the-pvalue, then checks the listener binds the-pport and not the config port. That holds only if-Dforeground mode parsed both-fand-pthrough theGetCommandLineW()argv rebuild.Skoll findings
_convertHelper()NULL leaves NULL in argv,mygetopt()derefs it5a1ff71aRecvWriteignoresbytesWritten, short write unrecoverable in appendea530fe0isAppendset on POSIX but only consumed on Windowsea530fe0(POSIX now usesWWRITE)-Dfix has no testmingw-regressjob +sshd_dash_d_test.ps1ea530fe0(SftpOpenPath/SftpWriteHandle/SftpCloseHandlehelpers)ifbodies drop bracesea530fe0"0123456789abcde"READ|TRUNCwithoutWRITEreports success without truncatingTesting
./configure --enable-sftp+make tests/regress.test tests/unit.test+ run,regress: PASS, unit exit 0.sshd_login_grace_test.ps1) and need a first CI run to confirm.