-
Notifications
You must be signed in to change notification settings - Fork 5k
[release/9.0-staging] Fix absolute path check when loading hostfxr/hostpolicy/coreclr #116775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/9.0-staging
Are you sure you want to change the base?
[release/9.0-staging] Fix absolute path check when loading hostfxr/hostpolicy/coreclr #116775
Conversation
On Windows, we were incorrectly only checking for <letter>: and incorrectly determining that UNC and device paths were not absolute. This updates pal::is_path_rooted to match the managed Path.IsPathRooted and adds a pal::is_path_absolute with the logic of Path.IsPartiallyQualified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the absolute path validation when loading components such as hostfxr, hostpolicy, and coreclr by replacing the old pal::is_path_rooted calls with the more robust pal::is_path_fully_qualified. Key changes include updates in multiple host resolver modules, improved path-check implementations in pal.windows.cpp and pal.unix.cpp, and new tests for device paths.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/native/corehost/hostpolicy/standalone/coreclr_resolver.cpp | Updated absolute path check for coreclr DLL loading |
src/native/corehost/hostpolicy/deps_resolver.cpp | Replaced is_path_rooted with is_path_fully_qualified in assertion |
src/native/corehost/hostmisc/pal.windows.cpp | Revised logic for detecting UNC/device paths and drive-letter paths |
src/native/corehost/hostmisc/pal.unix.cpp | Aligned Unix path qualification check with new API |
src/native/corehost/fxr_resolver.h & hostpolicy_resolver.cpp & apphost/standalone/hostfxr_resolver.cpp | Switched absolute path checks to use is_path_fully_qualified for hostfxr/hostpolicy |
src/native/corehost/corehost.cpp | Updated error trace to display hostfxr loading details |
src/installer/tests/HostActivation.Tests/*.cs | Added tests to validate device path scenarios |
Comments suppressed due to low confidence (8)
src/native/corehost/hostmisc/pal.windows.cpp:654
- The updated is_path_rooted now correctly recognizes UNC and device paths; ensure that all call sites are compatible with this enhanced behavior.
return (path.length() >= 1 && is_directory_separator(path[0])) // UNC or device paths
src/native/corehost/hostpolicy/standalone/coreclr_resolver.cpp:17
- Using pal::is_path_fully_qualified here ensures that the absolute path check properly distinguishes drive-letter and UNC/device paths; please confirm that this change covers all expected Windows path formats.
if (!pal::is_path_fully_qualified(coreclr_dll_path))
src/native/corehost/hostpolicy/deps_resolver.cpp:604
- Replacing is_path_rooted with is_path_fully_qualified in the assertion enforces a stricter check; please verify that existing assumptions about path validity remain valid.
assert(pal::is_path_fully_qualified(path));
src/native/corehost/hostmisc/pal.unix.cpp:195
- Switching to pal::is_path_fully_qualified aligns the Unix path qualification logic with the updated Windows implementation; please ensure this change does not induce regressions on Unix-like systems.
if (!pal::is_path_fully_qualified(library_name))
src/native/corehost/fxr_resolver.h:59
- Using is_path_fully_qualified for verifying hostfxr paths ensures consistency with other resolver modules; please confirm that this validation matches the managed implementation expectations.
if (!pal::is_path_fully_qualified(fxr_path))
src/native/corehost/fxr/standalone/hostpolicy_resolver.cpp:184
- The change to use is_path_fully_qualified provides a stricter and more accurate validation for hostpolicy paths; verify that this update does not conflict with how host paths are formatted in all scenarios.
if (!pal::is_path_fully_qualified(host_path))
src/native/corehost/apphost/standalone/hostfxr_resolver.cpp:121
- Updating the hostfxr path check to is_path_fully_qualified enforces more rigorous validation; please ensure that this change maintains compatibility with hostfxr loading expectations.
else if (!pal::is_path_fully_qualified(m_fxr_path))
src/native/corehost/corehost.cpp:203
- The enhanced error trace now better differentiates between a missing hostfxr path and a found but invalid path; please ensure the message provides clear guidance for troubleshooting.
trace::error(_X("Failed to resolve %s [%s]. Error code: 0x%x"), LIBFXR_NAME, fxr.fxr_path().empty() ? _X("not found") : fxr.fxr_path().c_str(), rc);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. we will take for consideration in 9.0.x
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
Backport of #116597 to release/9.0-staging
On Windows, we were incorrectly only checking for
<letter>:
and incorrectly determining that UNC and device paths were not absolute. This updatespal::is_path_rooted
to match the managedPath.IsPathRooted
and adds apal::is_path_absolute
with the logic ofPath.IsPartiallyQualified
cc @dotnet/appmodel @AaronRobinsonMSFT
Customer Impact
Customers are unable to run self-contained apps from or point framework-dependent apps to use a .NET runtime from a UNC share or device path.
Issue: #116521
Regression
Regressed in last servicing release
214743e
Testing
Manual testing for UNC share and device path. Automated test added for device paths.
Risk
Medium. The fix has to be in a path that every app will exercise. The updated logic is a mirror of existing logic in managed APIs.