From e23a69ea8a687d3cb185b5ac3a0c443933151060 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 28 Apr 2022 21:00:09 +0200 Subject: [PATCH 1/2] [Release/6.0] Port unloadability fixes This change ports two unloadability fixes for issues found by a customer. * #68550 that adds reference between native `LoaderAllocator`s of two collectible `AssemblyLoadContexts` when one of them is used to resolve assembly using the other one. This reference ensures that the one that provides the resolved `Assembly` isn't collected before the one that uses it. * #68336 that adds a missing lock around `m_LoaderAllocatorReferences` iteration during assembly load context destruction. --- src/coreclr/binder/assemblybinder.cpp | 4 +++- .../clrprivbinderassemblyloadcontext.cpp | 16 ++++++++++----- src/coreclr/binder/clrprivbindercoreclr.cpp | 2 +- src/coreclr/binder/inc/assemblybinder.hpp | 2 ++ src/coreclr/vm/appdomain.cpp | 20 ++++++++++++++++++- src/coreclr/vm/loaderallocator.cpp | 13 ++++-------- 6 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src/coreclr/binder/assemblybinder.cpp b/src/coreclr/binder/assemblybinder.cpp index c4a2e85d8f5ac8..658e0e031086c6 100644 --- a/src/coreclr/binder/assemblybinder.cpp +++ b/src/coreclr/binder/assemblybinder.cpp @@ -33,6 +33,7 @@ extern HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, CLRPrivBinderCoreCLR *pTPABinder, + AssemblyLoadContext *pBinder, ICLRPrivAssembly **ppLoadedAssembly); #endif // !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) @@ -1408,6 +1409,7 @@ namespace BINDER_SPACE HRESULT AssemblyBinder::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin, /* in */ AssemblyName *pAssemblyName, /* in */ CLRPrivBinderCoreCLR *pTPABinder, + /* in */ AssemblyLoadContext *pBinder, /* out */ Assembly **ppAssembly) { HRESULT hr = E_FAIL; @@ -1417,7 +1419,7 @@ HRESULT AssemblyBinder::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedA // RuntimeInvokeHostAssemblyResolver will perform steps 2-4 of CLRPrivBinderAssemblyLoadContext::BindAssemblyByName. ICLRPrivAssembly *pLoadedAssembly = NULL; hr = RuntimeInvokeHostAssemblyResolver(pManagedAssemblyLoadContextToBindWithin, - pAssemblyName, pTPABinder, &pLoadedAssembly); + pAssemblyName, pTPABinder, pBinder, &pLoadedAssembly); if (SUCCEEDED(hr)) { _ASSERTE(pLoadedAssembly != NULL); diff --git a/src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp b/src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp index 92d83d75b111a3..f44db5af13342d 100644 --- a/src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp +++ b/src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp @@ -84,7 +84,7 @@ HRESULT CLRPrivBinderAssemblyLoadContext::BindAssemblyByName(AssemblyNameData *p // of what to do next. The host-overridden binder can either fail the bind or return reference to an existing assembly // that has been loaded. // - hr = AssemblyBinder::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, m_pTPABinder, &pCoreCLRFoundAssembly); + hr = AssemblyBinder::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, m_pTPABinder, this, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { // We maybe returned an assembly that was bound to a different AssemblyLoadContext instance. @@ -258,11 +258,13 @@ void CLRPrivBinderAssemblyLoadContext::PrepareForLoadContextRelease(INT_PTR ptrM _ASSERTE(m_pAssemblyLoaderAllocator != NULL); _ASSERTE(m_loaderAllocatorHandle != NULL); - // We cannot delete the binder here as it is used indirectly when comparing assemblies with the same binder - // It will be deleted when the LoaderAllocator will be deleted - // But we can release the LoaderAllocator as we are no longer using it here + // We need to keep the LoaderAllocator pointer set as it still may be needed for creating references between the + // native LoaderAllocators of two collectible contexts in case the AssemblyLoadContext.Unload was called on the current + // context before returning from its AssemblyLoadContext.Load override or the context's Resolving event. + // But we need to release the LoaderAllocator so that it doesn't prevent completion of the final phase of unloading in + // some cases. It is safe to do as the AssemblyLoaderAllocator is guaranteed to be alive at least until the + // CustomAssemblyBinder::ReleaseLoadContext is called, where we NULL this pointer. m_pAssemblyLoaderAllocator->Release(); - m_pAssemblyLoaderAllocator = NULL; // Destroy the strong handle to the LoaderAllocator in order to let it reach its finalizer DestroyHandle(reinterpret_cast(m_loaderAllocatorHandle)); @@ -287,6 +289,10 @@ void CLRPrivBinderAssemblyLoadContext::ReleaseLoadContext() handle = reinterpret_cast(m_ptrManagedStrongAssemblyLoadContext); DestroyHandle(handle); m_ptrManagedAssemblyLoadContext = NULL; + + // The AssemblyLoaderAllocator is in a process of shutdown and should not be used + // after this point. + m_pAssemblyLoaderAllocator = NULL; } #endif // !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) diff --git a/src/coreclr/binder/clrprivbindercoreclr.cpp b/src/coreclr/binder/clrprivbindercoreclr.cpp index fec66a75458800..8a8aabb0976a5c 100644 --- a/src/coreclr/binder/clrprivbindercoreclr.cpp +++ b/src/coreclr/binder/clrprivbindercoreclr.cpp @@ -107,7 +107,7 @@ HRESULT CLRPrivBinderCoreCLR::BindUsingAssemblyName(BINDER_SPACE::AssemblyName * if (pManagedAssemblyLoadContext != NULL) { hr = AssemblyBinder::BindUsingHostAssemblyResolver(pManagedAssemblyLoadContext, pAssemblyName, - NULL, &pCoreCLRFoundAssembly); + NULL, this, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { // We maybe returned an assembly that was bound to a different AssemblyLoadContext instance. diff --git a/src/coreclr/binder/inc/assemblybinder.hpp b/src/coreclr/binder/inc/assemblybinder.hpp index 2ba11b7ac0183d..ffaab93e4fc3ff 100644 --- a/src/coreclr/binder/inc/assemblybinder.hpp +++ b/src/coreclr/binder/inc/assemblybinder.hpp @@ -21,6 +21,7 @@ class CLRPrivBinderCoreCLR; class PEAssembly; class PEImage; +class AssemblyLoadContext; namespace BINDER_SPACE { @@ -59,6 +60,7 @@ namespace BINDER_SPACE static HRESULT BindUsingHostAssemblyResolver (/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin, /* in */ AssemblyName *pAssemblyName, /* in */ CLRPrivBinderCoreCLR *pTPABinder, + /* in */ AssemblyLoadContext *pBinder, /* out */ Assembly **ppAssembly); static HRESULT BindUsingPEImage(/* in */ ApplicationContext *pApplicationContext, diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 13201596b7cfc3..683179bbc86453 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -5275,7 +5275,7 @@ AppDomain::AssemblyIterator::Next_Unlocked( #if !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) // Returns S_OK if the assembly was successfully loaded -HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, CLRPrivBinderCoreCLR *pTPABinder, ICLRPrivAssembly **ppLoadedAssembly) +HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, CLRPrivBinderCoreCLR *pTPABinder, AssemblyLoadContext *pBinder, ICLRPrivAssembly **ppLoadedAssembly) { CONTRACTL { @@ -5453,6 +5453,24 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB COMPlusThrowHR(COR_E_INVALIDOPERATION, IDS_HOST_ASSEMBLY_RESOLVER_DYNAMICALLY_EMITTED_ASSEMBLIES_UNSUPPORTED, name); } + // For collectible assemblies, ensure that the parent loader allocator keeps the assembly's loader allocator + // alive for all its lifetime. + if (pDomainAssembly->IsCollectible()) + { + LoaderAllocator *pResultAssemblyLoaderAllocator = pDomainAssembly->GetLoaderAllocator(); + LoaderAllocator *pParentLoaderAllocator = NULL; + hr = pBinder->GetLoaderAllocator((LPVOID*)&pParentLoaderAllocator); + if (!SUCCEEDED(hr)) + { + // The AssemblyLoadContext for which we are resolving the the Assembly is not collectible. + COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleBoundNonCollectible")); + } + + _ASSERTE(pParentLoaderAllocator); + _ASSERTE(pResultAssemblyLoaderAllocator); + pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); + } + pResolvedAssembly = pLoadedPEAssembly->GetHostAssembly(); } diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index 08ef69c1281acc..b4b710ea34483c 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -401,8 +401,9 @@ LoaderAllocator * LoaderAllocator::GCLoaderAllocators_RemoveAssemblies(AppDomain #endif //0 AppDomain::AssemblyIterator i; - // Iterate through every loader allocator, marking as we go { + // Iterate through every loader allocator, marking as we go + CrstHolder chLoaderAllocatorReferencesLock(pAppDomain->GetLoaderAllocatorReferencesLock()); CrstHolder chAssemblyListLock(pAppDomain->GetAssemblyListLock()); i = pAppDomain->IterateAssembliesEx((AssemblyIterationFlags)( @@ -424,17 +425,11 @@ LoaderAllocator * LoaderAllocator::GCLoaderAllocators_RemoveAssemblies(AppDomain } } } - } - - // Iterate through every loader allocator, unmarking marked loaderallocators, and - // build a free list of unmarked ones - { - CrstHolder chLoaderAllocatorReferencesLock(pAppDomain->GetLoaderAllocatorReferencesLock()); - CrstHolder chAssemblyListLock(pAppDomain->GetAssemblyListLock()); + // Iterate through every loader allocator, unmarking marked loaderallocators, and + // build a free list of unmarked ones i = pAppDomain->IterateAssembliesEx((AssemblyIterationFlags)( kIncludeExecution | kIncludeLoaded | kIncludeCollected)); - CollectibleAssemblyHolder pDomainAssembly; while (i.Next_Unlocked(pDomainAssembly.This())) { From f981a073ca481795251047114aa9e930858cbdad Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 28 Apr 2022 21:17:36 +0200 Subject: [PATCH 2/2] Remove breaking change part The change in main has added an exception in case a collectible `Assembly` is resolved into a non-collectible `AssemblyLoadContext`. Although that is incorrect, there are cases when it would work. So the added exception is a breaking change that I think we likely don't want to get into 6.0 --- src/coreclr/vm/appdomain.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 683179bbc86453..66795acaf03762 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -5460,15 +5460,12 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB LoaderAllocator *pResultAssemblyLoaderAllocator = pDomainAssembly->GetLoaderAllocator(); LoaderAllocator *pParentLoaderAllocator = NULL; hr = pBinder->GetLoaderAllocator((LPVOID*)&pParentLoaderAllocator); - if (!SUCCEEDED(hr)) + if (SUCCEEDED(hr)) { - // The AssemblyLoadContext for which we are resolving the the Assembly is not collectible. - COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleBoundNonCollectible")); + _ASSERTE(pParentLoaderAllocator); + _ASSERTE(pResultAssemblyLoaderAllocator); + pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); } - - _ASSERTE(pParentLoaderAllocator); - _ASSERTE(pResultAssemblyLoaderAllocator); - pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); } pResolvedAssembly = pLoadedPEAssembly->GetHostAssembly();