Fix AbsolutePath.GetCanonicalForm process state leak on Windows #13788
Open
OvesN wants to merge 6 commits into
Open
Fix AbsolutePath.GetCanonicalForm process state leak on Windows #13788OvesN wants to merge 6 commits into
OvesN wants to merge 6 commits into
Conversation
OvesN
force-pushed
the
dev/veronikao/fix-absolutepath-canonicalform-leak-13748
branch
from
61f2570 to
68dbbfa
Compare
…h on Windows (#13748) On Windows, Path.Combine 'resets' on a rooted second argument, so AbsolutePath(string, AbsolutePath) was leaving root-relative ('\foo') and drive-relative ('X:foo') inputs as Value unchanged. The result was a path that was rooted but not fully qualified — i.e. not a valid absolute path. Any subsequent use of Value (direct I/O or GetCanonicalForm) then resolved the path against process-global state (current drive, per-drive cwd), producing non-deterministic results across host processes and breaking multithreaded task isolation. Anchor those shapes against basePath inside the constructor using purely string-based operations, so Value is always a valid, fully qualified absolute path. GetCanonicalForm is left untouched and continues to call System.IO.Path.GetFullPath(string) — preserving its existing canonicalization semantics (including invalid-character rejection) across .NET and .NET Framework. The two-argument Path.GetFullPath overload is intentionally avoided: on .NET Framework it lives in Microsoft.IO.Path with different internal logic. Adds regression theory GetCanonicalForm_WindowsRootedButNotFullyQualifiedPath_AnchorsToBasePath_NotProcessState covering both root-relative and drive-relative inputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OvesN
force-pushed
the
dev/veronikao/fix-absolutepath-canonicalform-leak-13748
branch
from
68dbbfa to
839fd38
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses non-deterministic Windows path canonicalization in Microsoft.Build.Framework.AbsolutePath by ensuring rooted-but-not-fully-qualified inputs (e.g. \foo, X:foo) are anchored to the provided base path rather than process-global state (current drive / per-drive CWD), improving multithreaded task isolation.
Changes:
- Updates
AbsolutePath(string, AbsolutePath)to pre-anchor Windows rooted-but-not-fully-qualified shapes during construction via a helper. - Adds a Windows-only regression theory covering root-relative, drive-relative, UNC, and device-path base paths to ensure determinism for both
ValueandGetCanonicalForm(). - (Review finding) The anchoring logic is currently excluded from the
.NETStandardTFM, leaving the original behavior there.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Framework/PathHelpers/AbsolutePath.cs | Adds Windows anchoring logic to avoid Path.GetFullPath resolving against process-global state for certain rooted-but-not-fully-qualified inputs. |
| src/Framework.UnitTests/AbsolutePath_Tests.cs | Adds Windows-only regression coverage asserting construction and canonicalization are anchored to the supplied base path. |
Comment on lines
+114
to
+116
| #if NETFRAMEWORK || NET | ||
| combined = MakeFullyQualified(combined, basePath.Value); | ||
| #endif |
Contributor
Author
There was a problem hiding this comment.
The netstandard2.0 build of Microsoft.Build.Framework exists only as a compatibility surface — MSBuild itself never executes against it at runtime
Member
There was a problem hiding this comment.
nit: This might be a good comment to have in the code as well to make it more friendly, I was baffled the first time I saw such a condition.
Member
JanProvaznik
left a comment
JanProvaznik
left a comment
There was a problem hiding this comment.
the base idea is right
only must fix is the static
JanProvaznik
approved these changes
May 26, 2026
AR-May
requested changes
May 28, 2026
Comment on lines
+114
to
+116
| #if NETFRAMEWORK || NET | ||
| combined = MakeFullyQualified(combined, basePath.Value); | ||
| #endif |
Member
There was a problem hiding this comment.
nit: This might be a good comment to have in the code as well to make it more friendly, I was baffled the first time I saw such a condition.
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.
Fixes #13748
Context
AbsolutePath.Valuewas not guaranteed to be fully qualified when the struct was constructed viaAbsolutePath(string, AbsolutePath). On Windows,Path.Combine(basePath, path)returns the second argument unchanged when it is rooted-but-not-fully-qualified (\fooroot-relative,X:foodrive-relative), soValuecould end up as a non-absolute string. A laterGetCanonicalForm()call would then resolve it viaSystem.IO.Path.GetFullPath, which consults process-global state (current drive, per-drive cwd) — breaking determinism and multithreaded task isolation.Changes
AbsolutePath(string, AbsolutePath)anchors the two Windows rooted-but-not-fully-qualified shapes againstbasePathat construction time, via a new privateMakeFullyQualifiedhelper:\foo,/foo) → re-rooted underbasePath's root usingPath.GetPathRoot.X:foo) → drive dropped, remainder anchored underbasePathviaPath.Combine.Path.GetFullPath(path, basePath)so the un-normalized form is preserved.GetCanonicalForm()remains the single normalization step.#if NETFRAMEWORK || NETbecausePath.IsPathFullyQualifiedis not available in netstandard2.0. The netstandard2.0 build is a compat surface only and never runs at runtime in MSBuild.Testing
AnchorsToBasePath_NotProcessStatecovers:foo,sub\file.txt) anchored tobasePath— confirms CWD is never consulted.