PAL: Skip directories when looking for executables for CreateProcess by hoyosjs · Pull Request #126888 · dotnet/runtime

hoyosjs · 2026-04-14T17:51:53Z

This pull request improves the behavior of CreateProcessW on Unix-like systems to more closely match Windows by ensuring directories in the PATH are skipped when searching for executables. It also adds new tests to validate this behavior.

Mirror the fix from dotnet/diagnostics#5803: add fileExistsAndIsNotDirectory helper to process.cpp that uses stat() to verify a PATH candidate is a file, not a directory. Update getPath to use this helper in all three search locations (app dir, current dir, $PATH elements) so that directories with the same name as the target executable are skipped rather than treated as a match, matching Windows CreateProcessW behavior. Add threading/CreateProcessW/test3 PAL test that verifies: - (negative) CreateProcessW returns ERROR_FILE_NOT_FOUND when only a directory with the target name exists in PATH - (positive) CreateProcessW skips a directory blocker and finds the real executable in a later PATH element Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/dacf2636-f293-46c2-a747-fcbbd01bdbf6 Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/dacf2636-f293-46c2-a747-fcbbd01bdbf6 Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>

Copilot

Pull request overview

This PR updates Unix PAL CreateProcessW PATH resolution to skip directories when searching for an executable (aligning more closely with Windows behavior) and adds a new palsuite test to validate the behavior.

Changes:

Add a getPath helper check to ignore PATH candidates that are directories.
Add new CreateProcessW/test3 (parent + child) to validate directory-skipping behavior in PATH resolution.
Register the new test in palsuite build/test lists.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/coreclr/pal/src/thread/process.cpp	Adds a helper used by `getPath` to skip directory matches during PATH/app-dir/current-dir probing.
src/coreclr/pal/tests/palsuite/threading/CreateProcessW/test3/test3.cpp	New parent test covering negative/positive PATH scenarios involving a directory named like the executable.
src/coreclr/pal/tests/palsuite/threading/CreateProcessW/test3/test3_child.cpp	New child entrypoint used by the parent test when launched via PATH resolution.
src/coreclr/pal/tests/palsuite/paltestlist.txt	Registers the new `CreateProcessW/test3` parent test for execution.
src/coreclr/pal/tests/palsuite/CMakeLists.txt	Includes the new test sources in the `paltests` build.

Copilot · 2026-04-14T17:56:30Z

src/coreclr/pal/tests/palsuite/threading/CreateProcessW/test3/test3.cpp

+    /* Save the current PATH */
+    savedPathLen = GetEnvironmentVariableA("PATH", savedPath, sizeof(savedPath));
+    if (savedPathLen == 0 && GetLastError() != ERROR_ENVVAR_NOT_FOUND)
+    {
+        CleanupTestDirs(tmpDir1, blockerDir, tmpDir2, symlinkPath);
+        Fail("GetEnvironmentVariableA(PATH) failed with error %u\n", GetLastError());
+    }
+    if (savedPathLen >= sizeof(savedPath))
+    {
+        CleanupTestDirs(tmpDir1, blockerDir, tmpDir2, symlinkPath);
+        Fail("PATH environment variable is too long to save\n");
+    }


GetEnvironmentVariableA("PATH", ...) can return 0 with ERROR_ENVVAR_NOT_FOUND, but savedPath is never initialized in that case and later gets used to restore PATH. That can leave PATH set to garbage or an empty string instead of restoring the original state. Track whether PATH existed and restore with SetEnvironmentVariableA("PATH", NULL) when it didn’t, and/or initialize savedPath[0] = '\0' before calling GetEnvironmentVariableA.

Copilot · 2026-04-14T17:56:30Z

src/coreclr/pal/tests/palsuite/threading/CreateProcessW/test3/test3.cpp

+    /* Create a symlink named CHILD_EXE_NAME inside tmpDir2 pointing to paltests */
+    snprintf(symlinkPath, sizeof(symlinkPath), "%s/" CHILD_EXE_NAME, tmpDir2);
+    if (symlink(argv[0], symlinkPath) != 0)
+    {
+        CleanupTestDirs(tmpDir1, blockerDir, tmpDir2, NULL);
+        Fail("symlink(%s -> %s) failed with errno %d\n", symlinkPath, argv[0], errno);
+    }


The symlink target uses argv[0] directly. If argv[0] is a relative path (common in test harnesses), the symlink created in tmpDir2 will point to a non-existent location when resolved from that directory, causing the positive test to fail. Resolve argv[0] to an absolute path (e.g., via realpath) before calling symlink (or create the symlink using an absolute target path).

Copilot · 2026-04-14T17:56:31Z

src/coreclr/pal/tests/palsuite/threading/CreateProcessW/test3/test3.cpp

+
+    /* Build the command line (wide): CHILD_EXE_NAME CHILD_TEST_NAME */
+    snprintf(cmdLineA, sizeof(cmdLineA), CHILD_EXE_NAME " " CHILD_TEST_NAME);
+    MultiByteToWideChar(CP_ACP, 0, cmdLineA, -1, cmdLineW, (int)(sizeof(cmdLineW) / sizeof(WCHAR)));


The return value of MultiByteToWideChar isn’t checked here. If conversion fails (or the buffer is too small), cmdLineW may be left uninitialized and CreateProcessW behavior becomes undefined. Capture the return value and Fail() on 0 (as done in the existing CreateProcessW test1/test2).

Suggested change

MultiByteToWideChar(CP_ACP, 0, cmdLineA, -1, cmdLineW, (int)(sizeof(cmdLineW) / sizeof(WCHAR)));

if (MultiByteToWideChar(CP_ACP, 0, cmdLineA, -1, cmdLineW, (int)(sizeof(cmdLineW) / sizeof(WCHAR))) == 0)

{

CleanupTestDirs(tmpDir1, blockerDir, tmpDir2, symlinkPath);

Fail("MultiByteToWideChar failed with error %u\n", GetLastError());

}

Copilot · 2026-04-14T17:56:31Z

src/coreclr/pal/tests/palsuite/threading/CreateProcessW/test3/test3.cpp

+    /* Build unique temp directory names using the process ID */
+    DWORD pid = GetCurrentProcessId();
+    snprintf(tmpDir1, sizeof(tmpDir1), "/tmp/paltest_cpw_t3_%u_d1", pid);
+    snprintf(tmpDir2, sizeof(tmpDir2), "/tmp/paltest_cpw_t3_%u_d2", pid);
+    blockerDir[0] = '\0';


The test hardcodes temp directories under /tmp. Other CreateProcessW palsuite tests use GetTempPath* to respect platform/temp directory configuration. Consider building the temp dir paths under GetTempPathA (and using a unique suffix like mkdtemp-style) to avoid failures on systems where /tmp is unavailable or not writable.

github-actions · 2026-04-14T18:00:43Z

🤖 Copilot Code Review — PR #126888

Note

This review was generated by Copilot and should be treated as AI-generated analysis.

Holistic Assessment

Motivation: The PR mirrors a fix from dotnet/diagnostics#5803 for a real bug: when a directory has the same name as the target executable in $PATH, getPath() incorrectly treated the directory as a match (because access(F_OK) succeeds for directories). This caused CreateProcessW to stop the PATH search and return ERROR_ACCESS_DENIED instead of continuing to find the real executable. The motivation is well justified and the bug is clearly real.

Approach: The fix is well-targeted — a small fileExistsAndIsNotDirectory helper replaces three access() calls in getPath() to filter out directories early. This is the right layer to fix: getPath() is responsible for finding candidates, and directories are never valid candidates. The existing checkFileType() already had directory detection but was called too late (after getPath() returned), so the PATH search had already stopped. The test coverage is solid with both negative and positive scenarios.

Summary: ✅ LGTM. The core fix in process.cpp is correct, minimal, and properly addresses the root cause. The test is well-structured and follows sibling test patterns (test2). Two minor observations below are non-blocking.

Detailed Findings

✅ Correctness — `fileExistsAndIsNotDirectory` helper

The new helper correctly uses stat() to both check existence and verify the candidate is not a directory. All three access() call sites in getPath() are consistently updated. The existing checkFileType() (called downstream after getPath() returns) provides defense-in-depth — if a directory somehow slipped through, it would still be caught. Verified the remaining access() call at line 3650 in checkFileType() is independent and correct (it serves a different purpose: checking the final resolved path).

✅ Test coverage — Negative and positive scenarios

The test properly covers both failure cases:

Negative: Only a blocker directory exists in PATH → ERROR_FILE_NOT_FOUND (not ERROR_ACCESS_DENIED)
Positive: Blocker directory precedes real executable → CreateProcessW skips directory, finds real executable

The WaitForSingleObject return value check (commit 2) correctly matches the pattern established in test2/parentprocess.cpp (line 162-167). Cleanup is thorough with CleanupTestDirs called on all error paths.

💡 Minor inconsistency — Negative test `WaitForSingleObject` (line 132)

The WaitForSingleObject call on line 132 (the negative test's unexpected-success cleanup path) still doesn't check its return value, unlike the fix applied at line 180 for the positive test. This is in an error path that immediately calls Fail(), so it's not a real bug — the process will terminate regardless. But for consistency with the pattern established by commit 2, the same return-value check could be applied here.

✅ Build integration

CMakeLists.txt and paltestlist.txt are both correctly updated. Test entry names follow established conventions (paltest_createprocessw_test3).

Generated by Code Review for issue #126888 · ◷

Copilot AI and others added 2 commits April 14, 2026 09:45

test3: check WaitForSingleObject return value for timeout handling

81126df

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/dacf2636-f293-46c2-a747-fcbbd01bdbf6 Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>

hoyosjs requested review from Copilot and janvorli April 14, 2026 17:51

github-actions bot added the area-PAL-coreclr label Apr 14, 2026

dotnet-policy-service bot assigned hoyosjs Apr 14, 2026

Copilot started reviewing on behalf of hoyosjs April 14, 2026 17:52 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

build-analysis bot mentioned this pull request Apr 14, 2026

System.Net.NameResolution.Tests DNS failures: Name or service not known #126641

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PAL: Skip directories when looking for executables for CreateProcess#126888

PAL: Skip directories when looking for executables for CreateProcess#126888
hoyosjs wants to merge 2 commits intomainfrom
copilot/add-same-change-to-pal-runtime

hoyosjs commented Apr 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

github-actions bot commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hoyosjs commented Apr 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 14, 2026

🤖 Copilot Code Review — PR #126888

Holistic Assessment

Detailed Findings

✅ Correctness — fileExistsAndIsNotDirectory helper

✅ Test coverage — Negative and positive scenarios

💡 Minor inconsistency — Negative test WaitForSingleObject (line 132)

✅ Build integration

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

✅ Correctness — `fileExistsAndIsNotDirectory` helper

💡 Minor inconsistency — Negative test `WaitForSingleObject` (line 132)