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

Fix Assembly reference in unloadability #67922

Merged
merged 5 commits into from
Apr 15, 2022

Conversation

janvorli
Copy link
Member

When an assembly A1 from an unloadable AssemblyLoadContext 1 is referenced
by another assembly A2 from an unloadable AssemblyLoadContext 2, e.g.
when a class from A2 implements an interface from A1 and there are no
instances of types from A1 and the managed assembly type and the related
AssemblyLoadContext is gone too, the AssemblyLoadContext 1 can
incorrectly get collected, resulting in later crashes, null references
etc.

The problem is caused by the fact that we were missing adding references
between native LoaderAllocators corresponding to the
AssemblyLoadContexts in these cases.

The fix is to add such reference whenever an AssemblyLoadContext.Load or
the Resolving event of an AssemblyLoadContext returns an assembly from a
collectible AssemblyLoadContext. That way, that collectible
AssemblyLoadContext is held alive for the lifetime of the other context.

I have also added a regression test that crashes without this fix.

Close #67144

When an assembly A1 from an unloadable AssemblyLoadContext 1 is referenced
by another assembly A2 from an unloadable AssemblyLoadContext 2, e.g.
when a class from A2 implements an interface from A1 and there are no
instances of types from A1 and the managed assembly type and the related
AssemblyLoadContext is gone too, the AssemblyLoadContext 1 can
incorrectly get collected, resulting in later crashes, null references
etc.

The problem is caused by the fact that we were missing adding references
between native LoaderAllocators corresponding to the
AssemblyLoadContexts in these cases.

The fix is to add such reference whenever an AssemblyLoadContext.Load or
the Resolving event of an AssemblyLoadContext returns an assembly from a
collectible AssemblyLoadContext. That way, that collectible
AssemblyLoadContext is held alive for the lifetime of the other context.
@janvorli janvorli added this to the 6.0.x milestone Apr 12, 2022
@janvorli janvorli self-assigned this Apr 12, 2022
src/coreclr/vm/appdomain.cpp Show resolved Hide resolved
src/tests/Regressions/coreclr/GitHub_67144/test67144.cs Outdated Show resolved Hide resolved

EEFileLoadException::Throw(pSpec, FUSION_E_CACHEFILE_FAILED, NULL);
DomainAssembly *pParentAssembly = pSpec->GetParentAssembly();
BINDER_SPACE::Assembly *pBinderSpaceAssembly = pPEAssembly->GetHostAssembly();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably check HasHostAssembly / that pBinderSpaceAssembly is not null. Some cases (dynamically emitted assemblies is one, I think) might not have one - should we be adding the reference in those cases too?.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no host assembly, how can we get the LoaderAllocator of such an assembly? It seems it is possible to return a dynamically emitted collectible assembly from the ALC.Load / Resolving event handler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to GetAssemblyBinder and then get the LoaderAllocator off off that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the default ALC/binder (AssemblyBinder::IsDefault=true), I think the loader allocator on the binder will be null, but we shouldn't need the reference in that case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem with GetAssemblyBinder usage in general - if the assembly was marked for unload before it was returned from the ALC.Load / event, the binder had the LoaderAllocator already set to NULL. I have to check if it is a problem for the dynamic assemblies too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is @vitek-karas' request (which I'd second) for documenting expectations:

I think this is worth a comment in the code. Not the "create reference" that one is relatively easy to see, but when we expect NULLs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the following look good?

There are situations when `pParentAssembly` is NULL. For example when assembly is loaded via reflection. It is ok   
to ignore those.    
The purpose of the following code is to catch scenarios when resolution happens in another ALC solely based on  
ALC.Load or ALC.Resolving event handler. In such cases we need to add a reference here as it is not guaranteed  
to be added otherwise. These are also cases when `pParentAssembly` will always be set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to call out both 'parent assembly' for the request and the 'result assembly' for the result. Thoughts on something like the below?

If the ALC for the result (pPEAssembly) is collectible and not the same as the ALC for the request (pSpec),
which can happen when extension points (AssemblyLoadContext.Load, AssemblyLoadContext.Resolving) resolve the
assembly, we need to ensure the result ALC is not collected before the request ALC. We do this by adding an
explicit reference between the LoaderAllocators corresponding to the ALCs.

To get the LoaderAllocators, we rely on the DomainAssembly corresponding to:
- Parent assembly for the request
  - For loads via explicit load or reflection, this will be NULL. In these cases, the request ALC should not
    implicitly (as in not detectable via managed references) depend on the result ALC, so it is fine to not
    add the reference.
  - For loads via assembly references, this will never be NULL.
- Result assembly for the result
  - For dynamic assemblies, there will be no host assembly, so we have the result assembly / LoaderAllocator.
    We currently block resolving to dynamic assemblies, so we simply assert that we have a host assembly.
  - For non-dynamic assemblies, we should be able to get the host assembly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better indeed. These details may not be obvious.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines +3362 to +3367
// TODO: Disabling the below assertion as currently we experience
// inconsistency on resolving the Microsoft.Office.Interop.MSProject.dll
// This causes below assertion to fire and crashes the VS. This issue
// is being tracked with Dev10 Bug 658555. Brought back it when this bug
// is fixed.
// _ASSERTE(FALSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note - can you please create an issue to get this removed/resolved?
The comment is about .NET Framework, so it should be stale... hopefully.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've logged #68005 to follow up.

There are some really old TODOs in loader area and in runtime in general. Could be a good idea to clean that up, just as a matter of code hygiene.

VSadov and others added 2 commits April 14, 2022 16:32
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
src/coreclr/vm/appdomain.cpp Outdated Show resolved Hide resolved
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@VSadov
Copy link
Member

VSadov commented Apr 15, 2022

Thanks everybody!!
I think we have addressed all the concerns and I will merge this shortly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random exceptions when calling System.Type.GetInterface
5 participants