F8827 win sftp followup - #1241
Open
miyazakh wants to merge 10 commits into
Open
Conversation
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.
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.
apps/wolfssh/common.c declared a CONSOLE_SCREEN_BUFFER_INFO local in ClientSetEcho that nothing ever read; the Windows echo toggling never grew the code that would have used it. Drop the unused declaration. wolfssh/port.h defined WSTRSEP as strsep(), a BSD extension MSVCRT and MinGW do not provide. Add a portable wstrsep() in src/port.c, matching the wstrnstr/wstrncat/wstrdup pattern already used for other missing string functions, and route WSTRSEP through it under USE_WINDOWS_API. tests/regress.c called the two argument POSIX mkdir(path, mode) and setenv()/unsetenv() directly in TestKnownHostsLastEntry. Use the existing WMKDIR macro for the directory creation, and add small TEST_SETENV/TEST_UNSETENV macros backed by _putenv_s() on Windows so the HOME juggling this test does still works there. src/wolfsftp.c had three separate issues in code paths that had never been compiled before this job existed. wolfSSH_SFTP_RecvOpen declared a flagsAndAttrs DWORD that nothing read, since WS_CreateFileA is called with a hardcoded FILE_ATTRIBUTE_NORMAL instead. wolfSSH_SFTP_RecvOpenDir compared a signed loop counter against a sizeof expression while building ssh->driveList, so make the counter word32. wolfSSH_SFTP_Put passed &state->rSz, an int, to ReadFile()'s DWORD* output parameter; read into a local DWORD and copy it into state->rSz afterward, since that field is also assigned from WFREAD() on non-Windows builds. src/wolfterm.c's wolfSSH_DoOSC never used its handle parameter. Mark it with WOLFSSH_UNUSED rather than removing it, since the parameter matches the signature its two call sites already pass and future OSC handling such as window titles is a natural use for it. Verified against a real x86_64-w64-mingw32 cross compiler with a config.h edited to match the sizes and header availability the actual Windows CI run reported (SIZEOF_LONG 4, HAVE_SYS_IOCTL_H undefined, and so on): every file this job compiles builds cleanly under the same -Werror flag set. Also reconfirmed a clean, unmodified Linux build still passes both tests/regress.test and tests/unit.test.
CI's mingw-regress job showed AppendKeyToFile writing a known_hosts entry, then TestAppendKeyToFile reading it back and finding a byte mismatch on Windows. The file was opened with WFOPEN(..., "a"), a text mode append. On Windows the C runtime rewrites '\n' to CRLF on write in text mode, so the entry landed on disk with a trailing "\r\n" instead of the "\n" the test wrote and expected back. Open the file in binary mode instead, matching the WriteTextFile test helper a few hundred lines above in tests/regress.c, which already uses "wb" for the same reason. known_hosts is conventionally LF-terminated regardless of platform, so this also matches the format other SSH clients expect from the file, not just the test.
The next mingw-regress failure after the previous fix was
TestKnownHostsLastEntry failing its "ready" assertion. It calls
open("/dev/null", O_RDONLY) to point stdin at EOF while it drives the
known_hosts prompt, but Windows has no /dev/null; the call simply
fails there, so open() returns -1 and every check gated on ready is
skipped.
Add a TEST_NULL_DEVICE macro that resolves to "NUL" under
USE_WINDOWS_API and "/dev/null" otherwise, and use it at both call
sites in this file: TestKnownHostsLastEntry, and the same pattern in
TestPasswordEofNoCrash a bit earlier, which happens to be masked in
CI by its own isatty() guard but would hit the same bug if ever run
against a real terminal on Windows.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds CI coverage for Windows-only SFTP and wolfsshd code paths by introducing a MinGW/MSYS2 regression-test job and a new PowerShell regression test for wolfsshd -D option parsing.
Changes:
- Add an MSYS2 MinGW64 GitHub Actions job to build/run
tests/regress.testandtests/unit.testwithUSE_WINDOWS_APIenabled. - Add a Windows PowerShell regression test to validate
wolfsshd -Dcorrectly honors-fand-pparsing. - Improve Windows portability in tests and string helpers (e.g.,
/dev/nullvsNUL,setenvvs_putenv_s,strsepfallback).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/test.h | Gates MSVC-only warning pragmas to avoid issues under MinGW. |
| wolfssh/port.h | Routes WSTRSEP to a Windows-friendly implementation when USE_WINDOWS_API is set. |
| src/port.c | Implements wstrsep() for Windows builds. |
| tests/regress.c | Adjusts portability for Windows/MinGW execution (null device, env, mkdir). |
| src/wolfterm.c | Silences unused parameter warning in Windows console path. |
| src/wolfsftp.c | Minor Windows-type fixes and correct ReadFile() out-param handling. |
| src/internal.c | Refactors Windows path cleanup loop variable scoping. |
| apps/wolfssh/common.c | Ensures known_hosts appends are LF-stable by using binary append mode. |
| apps/wolfsshd/test/sshd_dash_d_test.ps1 | Adds regression test for wolfsshd -D parsing of -f/-p. |
| .github/workflows/windows-check.yml | Wires new PowerShell test into Windows job and adds MinGW regression job. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - name: Checkout wolfssl | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: wolfssl/wolfssl |
Comment on lines
+18
to
+25
| # (SshdExe also accepts the SSHD_PATH environment variable.) | ||
|
|
||
| param( | ||
| [string]$SshdExe = $env:SSHD_PATH, | ||
| [int]$Port = 22335, | ||
| [int]$ConfPort = 22336 | ||
| ) | ||
|
|
|
|
||
| # -D selects the non-service (foreground) path on Windows. | ||
| $sshd = Start-Process -FilePath $SshdExe ` | ||
| -ArgumentList "-D", "-f", "`"$confFile`"", "-p", "$Port" ` |
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
| #include <arpa/inet.h> | ||
| #ifndef _WIN32 |
wolfSSL-Fenrir-bot
requested changes
Sep 4, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1241
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| ready = (mkdir(homeDir, 0700) == 0) | ||
| && (mkdir(sshDir, 0700) == 0) | ||
| && (setenv("HOME", homeDir, 1) == 0); | ||
| ready = (WMKDIR(NULL, homeDir, 0700) == 0) |
There was a problem hiding this comment.
WMKDIR used in a test compiled without SFTP/SCP/SSHD, where the macro is undefined · API contract violations
WMKDIR is only defined by wolfssh/port.h:569-609 when WOLFSSH_SCP, WOLFSSH_SFTP, or WOLFSSH_SSHD is enabled, but TestKnownHostsLastEntry is gated only on WOLFSSL_BASE64_ENCODE and tests/regress.test is an unconditional check_PROGRAM. Plain ./configure (all three off, as in the os-check matrix) plus make check no longer compiles; the replaced mkdir() call had no such dependency.
Fix: Define a local TEST_MKDIR macro in regress.c (_mkdir(p) under USE_WINDOWS_API, mkdir(p,m) otherwise), as done for TEST_SETENV, and use it at both call sites.
A local run with ./configure --enable-smallstack (no --enable-sftp) showed WMKDIR as an implicit, undeclared function under MinGW. wolfssh/port.h only defines WMKDIR when WOLFSSH_SFTP, WOLFSSH_SCP, or WOLFSSH_SSHD is enabled, but TestKnownHostsLastEntry itself is gated on WOLFSSL_BASE64_ENCODE alone, so it compiles in configurations where none of those three are on and WMKDIR does not exist. Add a small TEST_MKDIR macro next to the existing TEST_SETENV and TEST_UNSETENV ones, backed directly by _mkdir() under USE_WINDOWS_API and mkdir() otherwise, and use it in place of WMKDIR. The Windows branch needs direct.h for _mkdir's declaration; include it next to the existing _WIN32 guard around arpa/inet.h at the top of the file. Verified with the MinGW cross compiler both with WOLFSSH_SFTP defined and without, and confirmed a plain Linux build configured with --enable-smallstack (no --enable-sftp) still passes tests/regress.test and tests/unit.test.
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.