Don't resolve a directory as an executable - #358
Merged
iCharlesHu merged 4 commits intoJul 29, 2026
Merged
Conversation
`Executable.name(_:)` resolution accepted any path that `access(_, X_OK)` succeeded on. On a directory the execute bit means "searchable", not "runnable", so a directory whose name matched the executable was returned as a resolved executable path. A `PATH` entry containing a `docker/` subdirectory, for example, would shadow the real `docker` binary and the returned path could then only fail at spawn time. Add `Configuration.executableAccessible(_:)`, which requires a candidate to be a regular file in addition to being executable, and use it for every candidate in `resolveExecutablePath(withPathValue:)`.
`GetFileAttributesW` succeeds for a directory and `SearchPathW` matches directories as well as files, so the Windows resolver had the same false positive as the Unix one: a `tool.exe` *directory* earlier on `PATH` shadowed the real `tool.exe`, and `resolveExecutablePath` handed back a path that can never be run. Verified on Windows 11 arm64: SearchPathW name="tool.exe" ext=nil -> E:\probe\shadow\tool.exe (a directory) CreateProcessW lpApplicationName=<that directory> -> 5 (ERROR_ACCESS_DENIED) Add `Configuration.executableAccessible(_:)` (exists and is not a directory) and `Configuration.isDirectory(_:)`, replacing the unused private `pathAccessible(_:)`, then: - `resolveExecutablePath(withPathValue:)` rejects a `SearchPathW` match that is a directory. `SearchPathW` cannot be resumed past a match, so it falls back to walking `possibleExecutablePaths(withPathValue:)`, which replicates the same search order, and takes the first real file. Behavior is unchanged whenever `SearchPathW` matches a file. - the spawn-time candidate loop, which is only used when argument 0 is overridden, treats ERROR_ACCESS_DENIED on a directory as a miss and tries the next candidate. Genuine permission failures on a real file still throw. `CreateProcessW`'s own search, used by the fast path, already skips directories, so the fast path needs no change: CWD=<dir containing a tool.exe directory>, PATH=<dir with real tool.exe> CreateProcessW(nil, "tool.exe") -> created -> E:\probe\real\tool.exe
The Unix check requires a regular file while the Windows one requires "not a directory", which raises the question of how each treats symlinks. Measured on both platforms; both are already correct, so this adds tests rather than changing either check. Unix uses `stat`, which follows symlinks, so a symlink is classified by its target: a link to an executable resolves, a link to a directory is rejected, and a dangling link fails the `access` check. realexe access(X_OK)= 0 stat S_ISREG=1 S_ISDIR=0 link-to-exe access(X_OK)= 0 stat S_ISREG=1 S_ISDIR=0 lstat S_ISLNK=1 link-to-dir access(X_OK)= 0 stat S_ISREG=0 S_ISDIR=1 lstat S_ISLNK=1 dangling access(X_OK)=-1 stat failed Requiring a regular file rather than merely a non-directory also matches what `execve` itself accepts. A FIFO with the execute bit set passes `access(_, X_OK)` and is not a directory, yet cannot be executed: myfifo: access(X_OK)=0 S_ISREG=0 S_ISFIFO=1 posix_spawn -> 13 (EACCES) On Windows `GetFileAttributesW` reports reparse points by what they point at, so the existing directory bit test is sufficient there: link-to-exe -> REPARSE_POINT CreateProcessW -> created link-to-dir -> DIRECTORY|REPARSE_POINT CreateProcessW -> 5 (ERROR_ACCESS_DENIED) junction-to-dir -> DIRECTORY|REPARSE_POINT Add tests for each case. On Unix, the non-regular-file and symlink-to-directory tests fail before the fix; the symlink-to-executable test passes either way and guards against switching `stat` for `lstat`. The Windows test needs SeCreateSymbolicLinkPrivilege, so it is gated on a `requiresSymbolicLinkPrivilege` condition trait.
Android CI fails the test at the setup step: `mkfifo` in the temporary
directory returns -1, so the fixture can never be built there.
✘ testNameResolutionSkipsNonRegularFile
Expectation failed: fifo._fileSystemPath.withCString { mkfifo($0, 0o755) } == 0
→ -1
Gate the test on a `requiresFIFOCreation` condition trait that probes
`mkfifo` in the temporary directory, mirroring the
`requiresSymbolicLinkPrivilege` trait the Windows symlink test uses.
Testing the capability rather than checking for `os(Android)` also covers
sandboxes elsewhere that deny FIFO creation.
Also report `errno` when the `mkfifo` inside the test fails, so a future
failure at that line says why.
Verified that the trait skips (rather than fails) when the probe fails,
and that the test target still cross-compiles for
aarch64-unknown-linux-android24.
iCharlesHu
approved these changes
Jul 29, 2026
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.
Problem
Executable resolution accepted candidates that can never be executed, on both platforms, because the "can I run this?" check was really an "does this exist / is it searchable?" check.
Unix.
access(_, X_OK)returns 0 for any directory the process can traverse — on a directory the execute bit means "searchable", not "runnable". Probed on macOS:Windows.
GetFileAttributesWsucceeds for a directory, andSearchPathWmatches directories as well as files. Probed on Windows 11 arm64, withshadow\containing directories namedtoolandtool.exeandreal\containing a realtool.exe:Consequences, all reachable today:
PATHentry containing adocker/(ortool.exe\) subdirectory shadows the real binary:resolveExecutablePath(in:)returns the directory instead of continuing the search..path(...)producesExecutable "docker" is not found or cannot be executed., pointing at the executable name rather than at the directory that actually matched.ERROR_ACCESS_DENIEDfrom a directory candidate, which is not in its retry set, so it throwsspawnFailedimmediately instead of trying the real executable later in the list.Two things that are not broken, and are therefore untouched here:
execvefails withEACCES, which is in their retry set.CreateProcessW's own search, which the Windows fast path relies on, already skips directories. Verified:Change
Each platform gets an
executableAccessible(_:)helper that answers the question the resolver actually needs, and the resolvers use it.Unix (
Subprocess+Unix.swift) —Configuration.executableAccessible(_:)requires a candidate to be a regular file (S_IFREG) in addition to being executable, and both candidate checks inresolveExecutablePath(withPathValue:)use it.possibleExecutablePaths(withPathValue:), the spawn loops, and.path(_:)are untouched.mode_t(S_IFMT)rather than a bare&:st_modeismode_t(UInt16on Darwin,UInt32on Linux) whileS_IFMT/S_IFREGare octal integer macros that import asInt32on glibc and musl. Confirmed against the musl 1.2.5 SDK headers (typedef unsigned mode_t;,#define S_IFMT 0170000).The two checks differ deliberately — regular-file on Unix, not-a-directory on Windows — and both handle symlinks correctly as a result:
stat, which follows symlinks, so a link is classified by its target: a link to an executable resolves, a link to a directory is rejected, and a dangling link fails theaccesscheck. Usinglstatwould break the first of those.execveaccepts, so it is not gratuitously stricter than reality. A FIFO with the execute bit set satisfiesaccess(_, X_OK)and is not a directory, yetposix_spawnrejects it withEACCES— "not a directory" would let it through.S_IFREGequivalent, andGetFileAttributesWalready reports reparse points by what they point at:REPARSE_POINTalone for a file symlink,DIRECTORY|REPARSE_POINTfor a directory symlink or a junction. The directory-bit test therefore covers all three, and file symlinks keep resolving and running.Windows (
Subprocess+Windows.swift) —Configuration.executableAccessible(_:)(exists and is not a directory) andConfiguration.isDirectory(_:)replace the unused privatepathAccessible(_:), which checked existence only. Then:resolveExecutablePath(withPathValue:)rejects aSearchPathWmatch that is a directory.SearchPathWstops at its first match and cannot be resumed, so on that outcome it falls back to walkingpossibleExecutablePaths(withPathValue:)— which already replicatesCreateProcessW's documented search order — and takes the first candidate that is a real file. Behavior is bit-for-bit unchanged wheneverSearchPathWmatches a file, including the error it reports when it matches nothing. One consequence worth calling out:SearchPathWis given thePATHvalue aslpPath, so it searches only those directories, whereaspossibleExecutablePathsalso covers the app directory and the current directory. In the directory-match case only, resolution can therefore land on a candidateSearchPathWwould not have considered. That matches what the spawn path already does on Windows, and it is the search order this file documents;Executable.name(_:)searches the current working directory beforePATH— decide and document the contract #357 covers whether that order is the one we want.ERROR_ACCESS_DENIEDas a miss only when the candidate is a directory, and tries the next candidate. Genuine permission failures on a real file still throwspawnFailedas before.Tests
In
SubprocessUnixTestsandSubprocessWindowsTests, allPATH-based with no dependency on the process's working directory:PATH, is skipped in favor of the real executabletestNameResolutionSkipsDirectoryInPathEntry(eager +Subprocess.run)testNameResolutionSkipsDirectoryInPathEntrytestNameThatIsADirectoryPathIsNotResolvedtestNameThatIsADirectoryPathIsNotResolvedtestNameResolutionSkipsSymlinkToDirectory,testNameResolutionFollowsSymlinkToExecutabletestNameResolutionClassifiesSymlinksByTargettestNameResolutionSkipsNonExecutableRegularFile,testNameResolutionSkipsNonRegularFiletestSpawnSkipsDirectoryCandidateWithArgumentZeroOverrideEvery test above fails before this change and passes after, except
testNameResolutionSkipsNonExecutableRegularFileandtestNameResolutionFollowsSymlinkToExecutable, which pass in both states by design: the first guards against theaccesscheck being replaced by hand-rolled mode-bit logic, the second againststatbeing swapped forlstat.Two tests are gated on a capability probe rather than on a platform check, so they skip cleanly where the environment forbids the setup:
SeCreateSymbolicLinkPrivilege, which an unelevated process only holds with Developer Mode enabled →requiresSymbolicLinkPrivilege. It did run and pass on the machine used here.mkfifoto succeed in the temporary directory, which it does not on Android (CI reportedmkfifo(...) → -1at the fixture setup) →requiresFIFOCreation. Probing the capability also covers sandboxes elsewhere that deny FIFO creation. I confirmed the trait reports the test as skipped rather than failed when the probe fails, and that the test target still cross-compiles foraarch64-unknown-linux-android24.On Windows, pre-fix, the failures are exactly as diagnosed:
Full suites: 160/160 on macOS 26 arm64, 139/139 on Windows 11 arm64 (Swift 6.3.2).
swift format lint --strictclean.Not addressed here
Whether
Executable.name(_:)should consult the current working directory at all is a separate, larger question — the spawn path searches it ahead ofPATHon both platforms, which contradicts the documentedPATH-only contract, while the eager API's behavior differs per platform. That is tracked in #357 with measurements from both platforms and the options; this PR deliberately settles none of it, and none of its tests assert anything about the working directory.