Skip to content
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

AssemblyLoadContext incorrectly validates name of dynamic assembly when returned from Load #53181

Closed
vitek-karas opened this issue May 24, 2021 · 4 comments
Labels
area-AssemblyLoader-coreclr untriaged New issue has not been triaged by the area owner

Comments

@vitek-karas
Copy link
Member

AssemblyLoadContext validates that the assembly returned from Load has the same name as the name which was asked for. This validation incorrectly handles dynamic assemblies and will always fail for them.

This is the cause because ValidateAssemblyNameWithSimpleName simply casts the returned assembly to RuntimeAssembly:

RuntimeAssembly? rtLoadedAssembly = assembly as RuntimeAssembly;

But this doesn't work for assemblies created by AssemblyBuilder.

The code should call GetRuntimeAssembly instead, which handles this case correctly.

Full repro for both affected public APIs (Load and Resolving) is here:
https://gist.github.com/vitek-karas/3c8f938dc15730ad00a375d8d745f36a

This is basically a derivative issue from the bug fix in #49387.

@ghost
Copy link

ghost commented May 24, 2021

Tagging subscribers to this area: @vitek-karas, @agocke, @CoffeeFlux, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

AssemblyLoadContext validates that the assembly returned from Load has the same name as the name which was asked for. This validation incorrectly handles dynamic assemblies and will always fail for them.

This is the cause because ValidateAssemblyNameWithSimpleName simply casts the returned assembly to RuntimeAssembly:

RuntimeAssembly? rtLoadedAssembly = assembly as RuntimeAssembly;

But this doesn't work for assemblies created by AssemblyBuilder.

The code should call GetRuntimeAssembly instead, which handles this case correctly.

Full repro for both affected public APIs (Load and Resolving) is here:
https://gist.github.com/vitek-karas/3c8f938dc15730ad00a375d8d745f36a

This is basically a derivative issue from the bug fix in #49387.

Author: vitek-karas
Assignees: -
Labels:

area-AssemblyLoader-coreclr

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 24, 2021
@Ap0k
Copy link
Contributor

Ap0k commented May 25, 2021

@vitek-karas
This error can be easily treated. But in the process of fixing it, I found the following case.

// Reflection emitted assemblies will not have a domain assembly.

If I understood everything correctly, then the dynamic assembly is not domain-specific and the code execution goes along the specified branch with the subsequent generation of an exception.

If fixing the definition of ALC for a dynamic assembly is not urgent, then I can investigate the above problem in more detail within the framework of this task. Otherwise, I propose to create a separate ticket for the error in the definition of ALC, merge it, merge the fixes of the tests (#48579) and then tackle the problem of this ticket tightly.

@vitek-karas
Copy link
Member Author

Honestly I was expecting this - that dynamic assemblies will be prevented from actually succeeding in Load. I still think it's worth fixing the name validation logic, mainly because it's inconsistent and fails with really bad error.

The potential support for dynamic assemblies in load resolution is probably a much larger work item (I'm not even sure what are all the potential problems with it for example), so I would definitely track that separately. There's also the question if it's actually worth going after at all.

@Ap0k
Copy link
Contributor

Ap0k commented May 25, 2021

Okay, I fill in both fixes, but without the ValidateAssemblyNameWithSimpleName tests

Ap0k added a commit to Ap0k/runtime that referenced this issue May 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-AssemblyLoader-coreclr untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

2 participants