-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Handle relative path loading on non-Windows for AppContext.BaseDirectory on NativeAOT #113171
base: main
Are you sure you want to change the base?
Conversation
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.
PR Overview
This PR addresses failures related to AppContext.BaseDirectory on NativeAOT by handling relative module paths on non‑Windows systems. Key changes include:
- Adding a ModuleInitializer to capture the working directory at startup.
- Implementing logic to convert relative module paths to absolute ones using the captured working directory.
- Updating tests to verify that AppContext.BaseDirectory is a rooted path.
Reviewed Changes
File | Description |
---|---|
src/coreclr/nativeaot/System.Private.CoreLib/src/System/AppContext.NativeAot.cs | Introduces logic to handle relative module paths and initializes the working directory at startup. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/AppContext/AppContextTests.cs | Adds a test to assert that BaseDirectory is rooted. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/coreclr/nativeaot/System.Private.CoreLib/src/System/AppContext.NativeAot.cs:69
- [nitpick] The use of GetRuntimeModulePath in both the top-level method and within RuntimeModulePathHolder might cause confusion. Consider renaming the private method inside RuntimeModulePathHolder to better clarify its distinct role.
public static string RuntimeModulePath { get; } = GetRuntimeModulePath();
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/AppContext/AppContextTests.cs:35
- Consider adding tests that simulate a scenario where the module path is relative and requires adjustment by the working directory logic to fully validate the new behavior.
Assert.True(Path.IsPathRooted(AppContext.BaseDirectory), "BaseDirectory should be a rooted path");
/azp run runtime-nativeaot-outerloop |
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
You mean this returns relative paths even for libraries, not just executables? Then this has a problem that we only run this initialization code when the first method from the library is called, not when the library is loaded, so there's still a window where current directory can change. We could make it so the directory capturing code runs very early, but that only addresses races with current-directory-changing code that is done with managed code. I'm honestly not a fan of the current directory capturing code. |
{ | ||
return modulePath; | ||
s_workingDirectoryAtStartup = Environment.CurrentDirectory; |
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.
Does this mean that close to every NAOT app is going to compute current directory during startup now?
I think we should revert the change that introduced the regression instead of trying to patch it up with a problematic fix. |
I agree, this is why I wrote "reliably" in #112290 (comment). If we can't do this reliably, it's better to not do this. We had security fixes around DLL load paths, this needs to be something we can trust. Revert at #113211. |
The other idea I have is to say "relative path loading means we fall back to ProcessPath", which would solve the immediate case, but it would still be messy. If I find a more reliable way to do this I'll bring this back. |
The module path we are getting, is it relative to process working directory or process path? If it is latter, then |
I need to do some more testing, but from what I could figure out, it was whatever path was passed to dlopen. |
Looking into this more, it looks like we could fall back to using /proc/self/maps when the path is relative and we can access /proc/self. Don't know if that's reliable enough for our scenario though. |
Addresses failures from #112457 (comment)
Should we attempt to handle the
LD_LIBRARY_PATH
case as well? I don't think it's necessary (we can doc as a limitation of this best-effort process), but we can.