[fix] InProcessVsTestConsoleWrapper drops Windows drive-relative env vars (#15740)#15773
[fix] InProcessVsTestConsoleWrapper drops Windows drive-relative env vars (#15740)#15773nohwnd wants to merge 3 commits into
Conversation
On Windows, Environment.GetEnvironmentVariables() (managed API) silently omits environment entries whose keys start with '=' (e.g. '=C:=C:\path'). These are drive-relative current directory entries maintained by the native Windows environment block. In the in-process vstest path, InProcessVsTestConsoleWrapper snapshots the environment via the managed API and passes it to testhost via ProcessHelper.ExternalEnvironmentVariables. As a result, these '='-prefixed entries were dropped, causing drive-relative paths in testhost to resolve incorrectly. Fix: on Windows, supplement the managed snapshot with the native entries by P/Invoking GetEnvironmentStringsW/FreeEnvironmentStringsW. Only '='-prefixed entries are extracted from the native block; all other entries are already captured by GetEnvironmentVariables(). Fixes #15740 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific environment inheritance gap in the in-process vstest execution path by ensuring =-prefixed drive-relative environment entries (present in the native environment block but omitted by Environment.GetEnvironmentVariables()) are forwarded to the testhost process.
Changes:
- Supplement the managed environment snapshot with
=-prefixed entries read from the native Windows environment block. - Add a private helper that P/Invokes
GetEnvironmentStringsW/FreeEnvironmentStringsWand parses only=-prefixed variables.
| // On Windows, Environment.GetEnvironmentVariables() omits entries whose keys start | ||
| // with '=' (e.g. "=C:=C:\path" — drive-relative current directory entries kept by | ||
| // the native environment block). Supplement the managed snapshot with those entries | ||
| // so that testhost receives a complete environment. | ||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| foreach (var (key, value) in GetWindowsNativeEqualsEnvironmentVariables()) | ||
| { | ||
| environmentVariableBaseline[key] = value; | ||
| } | ||
| } |
| private static IEnumerable<(string Key, string? Value)> GetWindowsNativeEqualsEnvironmentVariables() | ||
| { | ||
| IntPtr block = GetEnvironmentStringsW(); | ||
| if (block == IntPtr.Zero) | ||
| { | ||
| yield break; | ||
| } |
| } | ||
| else | ||
| { | ||
| // Key with no value (just "=key" or "=key="). |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
The fix is correct and well-scoped.
What was reviewed:
- P/Invoke declarations for
GetEnvironmentStringsW/FreeEnvironmentStringsW— valid; CA1416 is globally suppressed inDirectory.Build.props, consistent with the rest of the codebase yield returninsidetry/finally— valid C#;FreeEnvironmentStringsWis guaranteed to be called when theforeachdisposes the enumerator- Entry parsing (
IndexOf('=', 1)to skip the leading=and find the key/value separator) — correct for the canonical=C:=C:\pathformat - Block termination (zero-length string →
break) — correctly handles the double-null Windows native environment block end - Offset arithmetic (
offset * 2for char→byte conversion) — correct; environment blocks are negligibly small compared toint.MaxValue / 2 - Guard placement — supplement only runs inside
InheritEnvironmentVariables = truebranch and only on Windows
One minor nit (not blocking): the else branch comment says it handles "=key=" but that case is actually handled by the if branch (the else only covers "=key" with no second =). The code logic itself is correct.
🧠 Reviewed by Expert Code Reviewer 🧠
…ariables The else branch only handles '=key' entries (no second '='). Entries like '=key=value' are handled by the if branch above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
The fix is correct and well-scoped. Re-confirmed the same findings as the prior review run:
- P/Invoke declarations for
GetEnvironmentStringsW/FreeEnvironmentStringsWare valid; CA1416 is globally suppressed inDirectory.Build.props yield returninsidetry/finallyis valid —FreeEnvironmentStringsWis guaranteed to run when the enumerator is disposed, even on early iteration exitIndexOf('=', 1)combined withseparatorIndex > 0correctly identifies the key/value separator (minimum found index is 1, not-found returns -1)offset * 2char→byte arithmetic is correct; environment blocks are negligibly small vs.int.MaxValue / 2- Guard placement is correct: supplement only runs inside
InheritEnvironmentVariables = trueand only on Windows - No public API surface changes, no IPC protocol changes, no binding redirect implications
🧠 Reviewed by Expert Code Reviewer 🧠
This comment has been minimized.
This comment has been minimized.
On Windows, GetWindowsNativeEqualsEnvironmentVariables() adds '='-prefixed drive-relative current-directory entries from the native environment block. These are valid additions but cause the exact-count assertion to fail on CI. Change the count check to exclude '='-prefixed keys, which are the native entries added by the new code. The test still validates that all three expected managed-API keys are present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
| int offset = 0; | ||
| while (true) | ||
| { | ||
| string entry = Marshal.PtrToStringUni(IntPtr.Add(block, offset * 2))!; |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Reviewed the new test-fix commit (2026-05-16) added after the prior two review passes.
Activated dimensions (routed via src/vstest.console/): Environment Variable & Feature Flag Contracts · Process Architecture & Host Resolution · Null Safety & Boundary Validation
New commit analysis (InProcessVsTestConsoleWrapperTests.cs):
?.Keys.Cast<string>().Count(...)— null-safe:?.propagates through the chain;?? 0correctly handles a null dictionary.Cast<string>()is required becauseIDictionary.Keysreturns a non-genericICollection..Count(k => !k.StartsWith("=", StringComparison.Ordinal))— correctly excludes the=-prefixed drive-relative entries that the newGetWindowsNativeEqualsEnvironmentVariables()adds on Windows. The subsequentContainsKeyassertions still validate all three expected managed-API variables.- No logic errors or boundary issues found.
Summary: The test fix is correct and the PR is sound end-to-end. No new concerns.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
|
All CI checks are now fully green ✅ (Windows Release, Ubuntu, macOS all passed — build 1423112, 2026-05-16). All review feedback has been addressed. This PR is ready to merge but is still in draft state. Please click "Ready for review" to undraft it.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "dev.azure.com"See Network Configuration for more information.
|
Summary
Fixes #15740
Root Cause
On Windows,
Environment.GetEnvironmentVariables()(managed API) silently omits environment entries whose keys start with=(e.g.=C:=C:\path). These are drive-relative current directory entries maintained by the native Windows environment block (GetEnvironmentStringsW).In the in-process vstest path,
InProcessVsTestConsoleWrappersnapshots the current process environment via the managed API and passes it to the testhost process viaProcessHelper.ExternalEnvironmentVariables. Because the managed API omits=-prefixed entries, the testhost never receives them, causing drive-relative path resolution to behave differently in the testhost than in the host process.The out-of-process path is unaffected: it uses
ProcessStartInfowith OS-level inheritance, so the native environment block (including=-prefixed entries) is inherited automatically.Fix
In
InProcessVsTestConsoleWrapper.cs, after populatingenvironmentVariableBaselinefrom the managed snapshot, supplement it on Windows with entries from the raw native environment block obtained viaGetEnvironmentStringsW/FreeEnvironmentStringsWP/Invoke. Only=-prefixed entries are extracted from the native block; all other entries are already present in the managed snapshot.Changes
src/vstest.console/InProcessVsTestConsoleWrapper.cs: AddedGetWindowsNativeEqualsEnvironmentVariables()private static helper and called it whenInheritEnvironmentVariables = trueon Windows.