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

[mono] Fix crash in AssemblyBuilder::GetModules #55202

Merged
1 commit merged into from
Jul 8, 2021
Merged

[mono] Fix crash in AssemblyBuilder::GetModules #55202

1 commit merged into from
Jul 8, 2021

Conversation

uweigand
Copy link
Contributor

@uweigand uweigand commented Jul 6, 2021

  • The "modules" field may be null if called before the end of the
    AssemblyBuilder ctor (which may happen in an OnAssemblyLoad event
    handler invoked during the basic_init VM call).

This happens during some aspnetcore test cases. I'm not completely sure if the OnAssemblyLoad event handler is supposed to see that singleton module or not (looking at the CoreCLR sources I believe it may there), but that seems difficult to implement in Mono due to interactions between the basic_init calls in AssemblyBuilder vs. ModuleBuilder.

In any case, this patch at least removes the crash by special-casing a null modules fields (like several other AssemblyBuilder accessor functions already do!), and the aspnetcore tests now pass.

* The "modules" field may be null if called before the end of the
  AssemblyBuilder ctor (which may happen in an OnAssemblyLoad event
  handler invoked during the basic_init VM call).
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@lambdageek
Copy link
Member

May be related to the null GetModules that we saw in dotnet/aspnetcore#33152 (comment) that I wasn't able to reproduce with a straightforward test.

@uweigand
Copy link
Contributor Author

uweigand commented Jul 6, 2021

May be related to the null GetModules that we saw in dotnet/aspnetcore#33152 (comment) that I wasn't able to reproduce with a straightforward test.

Yes, this is exactly the code path in aspnetcore where I saw the crash. (I'm still using the preview5 level of aspnetcore which doesn't have dotnet/aspnetcore#33263 applied yet.)

@lambdageek lambdageek added the area-VM-reflection-mono Reflection issues specific to MonoVM label Jul 8, 2021
@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details
  • The "modules" field may be null if called before the end of the
    AssemblyBuilder ctor (which may happen in an OnAssemblyLoad event
    handler invoked during the basic_init VM call).

This happens during some aspnetcore test cases. I'm not completely sure if the OnAssemblyLoad event handler is supposed to see that singleton module or not (looking at the CoreCLR sources I believe it may there), but that seems difficult to implement in Mono due to interactions between the basic_init calls in AssemblyBuilder vs. ModuleBuilder.

In any case, this patch at least removes the crash by special-casing a null modules fields (like several other AssemblyBuilder accessor functions already do!), and the aspnetcore tests now pass.

Author: uweigand
Assignees: -
Labels:

area-System.Reflection-mono

Milestone: -

@ghost
Copy link

ghost commented Jul 8, 2021

Hello @lambdageek!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 88732bc into dotnet:main Jul 8, 2021
@uweigand uweigand deleted the mono-getmodules-null branch July 14, 2021 12:19
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-reflection-mono Reflection issues specific to MonoVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants