fix(File): getExecutablePath skips directories shadowing executables …#5211
fix(File): getExecutablePath skips directories shadowing executables …#5211
Conversation
…on PATH #5210 A directory in CWD with the same name as a PATH executable (e.g. a source checkout named crun/ shadowing /usr/bin/crun) caused getExecutablePathImpl() to return empty instead of finding the real executable. The root cause was findInPath() returning the first existing entry (including directories) with no way to continue searching. Replaced the findInPath() call in the bare-name branch with an inline CWD + PATH search requiring S_ISREG. Added testGetExecutablePathDirectoryShadow (FileTest) and testPathResolution (ProcessRunnerTest).
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where a directory in the current working directory with the same name as a PATH executable would incorrectly shadow the real executable, causing getExecutablePath() to return empty. The fix replaces the findInPath() call with an inline search that explicitly checks for regular executable files using S_ISREG.
Changes:
- Modified
File_UNIX.cppto implement inline CWD + PATH search with explicit regular file checking - Added
testGetExecutablePathDirectoryShadowtest inFileTestto verify directory shadowing is prevented - Added
testPathResolutiontest inProcessRunnerTestto verify end-to-end process execution with directory shadowing
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Foundation/src/File_UNIX.cpp | Replaces findInPath() with inline search requiring S_ISREG to skip directories |
| Foundation/testsuite/src/FileTest.h | Declares new test method testGetExecutablePathDirectoryShadow |
| Foundation/testsuite/src/FileTest.cpp | Implements test verifying directory shadowing doesn't prevent executable resolution |
| Foundation/testsuite/src/ProcessRunnerTest.h | Declares new test method testPathResolution |
| Foundation/testsuite/src/ProcessRunnerTest.cpp | Implements end-to-end test with ProcessRunner verifying directory shadowing fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matejk
left a comment
There was a problem hiding this comment.
Review Notes
1. Code duplication with canExecuteImpl()
The isExecutableFile lambda in getExecutablePathImpl() (File_UNIX.cpp:115-126) duplicates the exact logic of canExecuteImpl() (lines 205-219). If the permission-check logic ever changes (e.g., adding supplementary group support), it would need to be updated in two places. Consider extracting a shared static helper or calling canExecuteImpl() from the lambda to avoid divergence.
2. Root user check — only checks S_IXUSR
For root (geteuid() == 0), the code only checks S_IXUSR. The kernel allows root to execute a file if any execute bit is set (S_IXUSR | S_IXGRP | S_IXOTH). This means a file with mode ---x--x--x owned by another user would be rejected by this code but accepted by the kernel. This is a pre-existing pattern (same in canExecuteImpl), but worth noting.
3. PATH cache bypass — consider using getPathDirectories()
File.cpp already has a thread-safe cached PATH tokenizer getPathDirectories() (File.cpp:47-76) that avoids re-reading and re-tokenizing PATH on every call. This fix duplicates that logic and re-parses PATH from scratch each time.
Since File_UNIX.cpp is #included from File.cpp (line 34), getPathDirectories() could be made accessible by moving its definition (and the anonymous namespace) above the platform #include. This would eliminate the duplicated PATH parsing and provide the same caching benefit that findInPath() already enjoys.
#5211) * fix(File): getExecutablePath skips directories shadowing executables on PATH #5210 A directory in CWD with the same name as a PATH executable (e.g. a source checkout named crun/ shadowing /usr/bin/crun) caused getExecutablePathImpl() to return empty instead of finding the real executable. The root cause was findInPath() returning the first existing entry (including directories) with no way to continue searching. Replaced the findInPath() call in the bare-name branch with an inline CWD + PATH search requiring S_ISREG. Added testGetExecutablePathDirectoryShadow (FileTest) and testPathResolution (ProcessRunnerTest). * fix(ProcessRunnerTest): wrong cmdDir * fix(File): code duplication; root can execute any x bit #5210
…on PATH #5210
A directory in CWD with the same name as a PATH executable (e.g. a source checkout named crun/ shadowing /usr/bin/crun) caused getExecutablePathImpl() to return empty instead of finding the real executable. The root cause was findInPath() returning the first existing entry (including directories) with no way to continue searching. Replaced the findInPath() call in the bare-name branch with an inline CWD + PATH search requiring S_ISREG.
Added testGetExecutablePathDirectoryShadow (FileTest) and testPathResolution (ProcessRunnerTest).