Fix LoadFromStream to stop checking/loading from APP_PATHS#123049
Fix LoadFromStream to stop checking/loading from APP_PATHS#123049elinor-fung wants to merge 3 commits intodotnet:mainfrom
LoadFromStream to stop checking/loading from APP_PATHS#123049Conversation
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where AssemblyLoadContext.LoadFromStream for the default ALC was incorrectly loading assemblies from APP_PATHS if they existed instead of the provided stream. The fix changes BindUsingPEImage to only check if the assembly is already loaded in the context, rather than performing a full bind that could probe APP_PATHS.
Key Changes:
- Modified
BindUsingPEImageto only check if an assembly with the same name is already loaded viaFindInExecutionContext, removing the previous full bind logic - Removed the
excludeAppPathsparameter fromBindUsingPEImageand its entire call chain since APP_PATHS is no longer checked at all in this code path - Added regression tests to verify that
LoadFromStreamcorrectly loads from the stream even whenAPP_PATHSis set
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/binder/assemblybindercommon.cpp | Core fix: Changed BindUsingPEImage to only check if assembly is already loaded via FindInExecutionContext instead of performing full bind with APP_PATHS probing; preserves MVID checking logic |
| src/coreclr/binder/inc/assemblybindercommon.hpp | Removed excludeAppPaths parameter from BindUsingPEImage function signature |
| src/coreclr/binder/defaultassemblybinder.cpp | Updated BindUsingPEImage implementation to remove excludeAppPaths parameter |
| src/coreclr/binder/inc/defaultassemblybinder.h | Updated BindUsingPEImage signature to remove excludeAppPaths parameter |
| src/coreclr/binder/customassemblybinder.cpp | Updated BindUsingPEImage implementation to remove excludeAppPaths parameter; includes minor trailing whitespace cleanup |
| src/coreclr/binder/inc/customassemblybinder.h | Updated BindUsingPEImage signature to remove excludeAppPaths parameter |
| src/coreclr/vm/assemblybinder.h | Updated virtual method signature to remove excludeAppPaths parameter; minor trailing whitespace cleanup |
| src/coreclr/vm/assemblynative.cpp | Updated call to LoadFromPEImage to remove excludeAppPaths argument; added explanatory comment about stream-based loading behavior |
| src/coreclr/vm/assemblynative.hpp | Updated LoadFromPEImage signature to remove excludeAppPaths parameter |
| src/coreclr/vm/assemblyspec.cpp | Updated call to LoadFromPEImage to remove excludeAppPaths argument |
| src/tests/Loader/binding/AppPaths/AppPaths.cs | New test verifying LoadFromStream loads from stream (not APP_PATHS) for both default and custom ALCs |
| src/tests/Loader/binding/AppPaths/AppPaths.csproj | Test project configuration |
| src/tests/Loader/binding/AppPaths/AssemblyToLoad.cs | Simple test assembly to be loaded from stream |
| src/tests/Loader/binding/AppPaths/AssemblyToLoad.csproj | Test assembly project configuration |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
To me, this seems to be expected behavior. I believe that the design has been that all assemblies on APP_PATHS and TPA are assumed to be logically loaded and it should not be possible to overwrite them. Is this PR changing it for APP_PATHS only or for TPA as well? Do we really want to change this? It creates opportunities for interesting loader race conditions. |
|
This would only change it for APP_PATHS. There's a separate explicit check for the TPA only earlier. I was kind of undecided about where APP_PATHS fell, since it is a configured probe directory rather than known assemblies. I can see treating anything in / that ends up in it being logically loaded just like the TPA as reasonable logical group. |
AssemblyLoadContext.LoadFromStreamfor the default ALC was loading assemblies fromAPP_PATHSif they exist instead of the provided stream.BindUsingPEImagewas going through a regular full bind instead of just checking if the assembly was already loaded. This change updates to just check if the assembly is already in the context instead.Fixes: #122304
cc @dotnet/appmodel @AaronRobinsonMSFT