From c05b0b890eda066e715e09c0847f7a4dc49c420e Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 22 Apr 2022 16:11:03 +0200 Subject: [PATCH 1/6] Rework the unloadability fix The recent fix had a problem when GC collected the managed Assembly loaded via the Load override or the Resolving event before we added the reference between the related native LoaderAllocators. The managed LoaderAllocator of the ALC we've loaded the Assembly into was collected and we couldn't create the managed Type objects for the interfaces from that assembly in the GetInterfaces call on a type that implemented those. This change fixes it by moving the creation of reference between the native LoaderAllocators to the RuntimeInvokeHostAssemblyResolver where we still have a live managed reference to the loaded Assembly. --- src/coreclr/binder/assemblybindercommon.cpp | 8 ++-- src/coreclr/binder/customassemblybinder.cpp | 4 +- src/coreclr/binder/defaultassemblybinder.cpp | 3 +- .../binder/inc/assemblybindercommon.hpp | 2 + src/coreclr/binder/inc/customassemblybinder.h | 1 + .../binder/inc/defaultassemblybinder.h | 2 + src/coreclr/vm/appdomain.cpp | 45 +++++-------------- src/coreclr/vm/assemblybinder.cpp | 3 +- src/coreclr/vm/assemblybinder.h | 7 ++- src/coreclr/vm/coreassemblyspec.cpp | 14 +++++- 10 files changed, 47 insertions(+), 42 deletions(-) diff --git a/src/coreclr/binder/assemblybindercommon.cpp b/src/coreclr/binder/assemblybindercommon.cpp index 4e0bad0e843393..a06ea1f2f10392 100644 --- a/src/coreclr/binder/assemblybindercommon.cpp +++ b/src/coreclr/binder/assemblybindercommon.cpp @@ -30,6 +30,7 @@ extern HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, DefaultAssemblyBinder *pDefaultBinder, + LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly **ppLoadedAssembly); #endif // !defined(DACCESS_COMPILE) @@ -1153,9 +1154,10 @@ namespace BINDER_SPACE #if !defined(DACCESS_COMPILE) HRESULT AssemblyBinderCommon::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin, - /* in */ AssemblyName *pAssemblyName, + /* in */ AssemblyName *pAssemblyName, /* in */ DefaultAssemblyBinder *pDefaultBinder, - /* out */ Assembly **ppAssembly) + /* in */ LoaderAllocator *pParentLoaderAllocator, + /* out */ Assembly **ppAssembly) { HRESULT hr = E_FAIL; @@ -1164,7 +1166,7 @@ HRESULT AssemblyBinderCommon::BindUsingHostAssemblyResolver(/* in */ INT_PTR pMa // RuntimeInvokeHostAssemblyResolver will perform steps 2-4 of CustomAssemblyBinder::BindAssemblyByName. BINDER_SPACE::Assembly *pLoadedAssembly = NULL; hr = RuntimeInvokeHostAssemblyResolver(pManagedAssemblyLoadContextToBindWithin, - pAssemblyName, pDefaultBinder, &pLoadedAssembly); + pAssemblyName, pDefaultBinder, pParentLoaderAllocator, &pLoadedAssembly); if (SUCCEEDED(hr)) { _ASSERTE(pLoadedAssembly != NULL); diff --git a/src/coreclr/binder/customassemblybinder.cpp b/src/coreclr/binder/customassemblybinder.cpp index d82e150c13237a..b493d988936f87 100644 --- a/src/coreclr/binder/customassemblybinder.cpp +++ b/src/coreclr/binder/customassemblybinder.cpp @@ -39,6 +39,7 @@ HRESULT CustomAssemblyBinder::BindAssemblyByNameWorker(BINDER_SPACE::AssemblyNam } HRESULT CustomAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, + LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) { // When LoadContext needs to resolve an assembly reference, it will go through the following lookup order: @@ -72,7 +73,8 @@ HRESULT CustomAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName* // 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 = AssemblyBinderCommon::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, m_pDefaultBinder, &pCoreCLRFoundAssembly); + hr = AssemblyBinderCommon::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, + m_pDefaultBinder, pParentLoaderAllocator, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { // We maybe returned an assembly that was bound to a different AssemblyBinder instance. diff --git a/src/coreclr/binder/defaultassemblybinder.cpp b/src/coreclr/binder/defaultassemblybinder.cpp index 96399f972dc613..79bcea8d31e71a 100644 --- a/src/coreclr/binder/defaultassemblybinder.cpp +++ b/src/coreclr/binder/defaultassemblybinder.cpp @@ -39,6 +39,7 @@ HRESULT DefaultAssemblyBinder::BindAssemblyByNameWorker(BINDER_SPACE::AssemblyNa // DefaultAssemblyBinder implementation // ============================================================================ HRESULT DefaultAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName *pAssemblyName, + LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly **ppAssembly) { HRESULT hr = S_OK; @@ -85,7 +86,7 @@ HRESULT DefaultAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName if (pManagedAssemblyLoadContext != NULL) { hr = AssemblyBinderCommon::BindUsingHostAssemblyResolver(pManagedAssemblyLoadContext, pAssemblyName, - NULL, &pCoreCLRFoundAssembly); + NULL, pParentLoaderAllocator, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { // We maybe returned an assembly that was bound to a different AssemblyLoadContext instance. diff --git a/src/coreclr/binder/inc/assemblybindercommon.hpp b/src/coreclr/binder/inc/assemblybindercommon.hpp index 4c2f1db618e941..54dd1b0b0d583e 100644 --- a/src/coreclr/binder/inc/assemblybindercommon.hpp +++ b/src/coreclr/binder/inc/assemblybindercommon.hpp @@ -22,6 +22,7 @@ class AssemblyBinder; class DefaultAssemblyBinder; class PEAssembly; class PEImage; +class LoaderAllocator; namespace BINDER_SPACE { @@ -52,6 +53,7 @@ namespace BINDER_SPACE static HRESULT BindUsingHostAssemblyResolver (/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin, /* in */ AssemblyName *pAssemblyName, /* in */ DefaultAssemblyBinder *pDefaultBinder, + /* in */ LoaderAllocator *pParentLoaderAllocator, /* out */ Assembly **ppAssembly); static HRESULT BindUsingPEImage(/* in */ AssemblyBinder *pBinder, diff --git a/src/coreclr/binder/inc/customassemblybinder.h b/src/coreclr/binder/inc/customassemblybinder.h index 9d59832f3c9798..cb317367e01ab5 100644 --- a/src/coreclr/binder/inc/customassemblybinder.h +++ b/src/coreclr/binder/inc/customassemblybinder.h @@ -21,6 +21,7 @@ class CustomAssemblyBinder final : public AssemblyBinder BINDER_SPACE::Assembly** ppAssembly) override; HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, + LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) override; AssemblyLoaderAllocator* GetLoaderAllocator() override; diff --git a/src/coreclr/binder/inc/defaultassemblybinder.h b/src/coreclr/binder/inc/defaultassemblybinder.h index 398174c65a078b..8235a28b357594 100644 --- a/src/coreclr/binder/inc/defaultassemblybinder.h +++ b/src/coreclr/binder/inc/defaultassemblybinder.h @@ -10,6 +10,7 @@ class PEAssembly; class PEImage; +class LoaderAllocator; class DefaultAssemblyBinder final : public AssemblyBinder { @@ -19,6 +20,7 @@ class DefaultAssemblyBinder final : public AssemblyBinder BINDER_SPACE::Assembly** ppAssembly) override; HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, + LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) override; AssemblyLoaderAllocator* GetLoaderAllocator() override diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 7c6f55a242a8c8..5b94292ee2472f 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -3370,37 +3370,6 @@ BOOL AppDomain::AddFileToCache(AssemblySpec* pSpec, PEAssembly * pPEAssembly, BO } } - // 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 is no host assembly, so we don't 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. - DomainAssembly *pParentAssembly = pSpec->GetParentAssembly(); - BINDER_SPACE::Assembly *pBinderSpaceAssembly = pPEAssembly->GetHostAssembly(); - _ASSERTE(pBinderSpaceAssembly != NULL); - DomainAssembly *pResultAssembly = pBinderSpaceAssembly->GetDomainAssembly(); - if ((pParentAssembly != NULL) && (pResultAssembly != NULL)) - { - LoaderAllocator *pParentAssemblyLoaderAllocator = pParentAssembly->GetLoaderAllocator(); - LoaderAllocator *pResultAssemblyLoaderAllocator = pResultAssembly->GetLoaderAllocator(); - _ASSERTE(pParentAssemblyLoaderAllocator); - _ASSERTE(pResultAssemblyLoaderAllocator); - if (pResultAssemblyLoaderAllocator->IsCollectible()) - { - pParentAssemblyLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); - } - } - return TRUE; } @@ -4937,7 +4906,7 @@ AppDomain::AssemblyIterator::Next_Unlocked( #if !defined(DACCESS_COMPILE) // Returns S_OK if the assembly was successfully loaded -HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, DefaultAssemblyBinder *pDefaultBinder, BINDER_SPACE::Assembly **ppLoadedAssembly) +HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, DefaultAssemblyBinder *pDefaultBinder, LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly **ppLoadedAssembly) { CONTRACTL { @@ -5015,7 +4984,7 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB // Switch to pre-emp mode before calling into the binder GCX_PREEMP(); BINDER_SPACE::Assembly *pCoreCLRFoundAssembly = NULL; - hr = pDefaultBinder->BindUsingAssemblyName(pAssemblyName, &pCoreCLRFoundAssembly); + hr = pDefaultBinder->BindUsingAssemblyName(pAssemblyName, pParentLoaderAllocator, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { _ASSERTE(pCoreCLRFoundAssembly != NULL); @@ -5115,6 +5084,16 @@ 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(); + _ASSERTE(pParentLoaderAllocator); + _ASSERTE(pResultAssemblyLoaderAllocator); + pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); + } + pResolvedAssembly = pLoadedPEAssembly->GetHostAssembly(); } diff --git a/src/coreclr/vm/assemblybinder.cpp b/src/coreclr/vm/assemblybinder.cpp index 2e231741efbd34..d2aa3d645752db 100644 --- a/src/coreclr/vm/assemblybinder.cpp +++ b/src/coreclr/vm/assemblybinder.cpp @@ -9,6 +9,7 @@ #ifndef DACCESS_COMPILE HRESULT AssemblyBinder::BindAssemblyByName(AssemblyNameData* pAssemblyNameData, + LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) { _ASSERTE(pAssemblyNameData != nullptr && ppAssembly != nullptr); @@ -20,7 +21,7 @@ HRESULT AssemblyBinder::BindAssemblyByName(AssemblyNameData* pAssemblyNameData, SAFE_NEW(pAssemblyName, BINDER_SPACE::AssemblyName); IF_FAIL_GO(pAssemblyName->Init(*pAssemblyNameData)); - hr = BindUsingAssemblyName(pAssemblyName, ppAssembly); + hr = BindUsingAssemblyName(pAssemblyName, pParentLoaderAllocator, ppAssembly); Exit: return hr; diff --git a/src/coreclr/vm/assemblybinder.h b/src/coreclr/vm/assemblybinder.h index e737d92c2de7c0..7e48e4770f6b35 100644 --- a/src/coreclr/vm/assemblybinder.h +++ b/src/coreclr/vm/assemblybinder.h @@ -12,14 +12,17 @@ class NativeImage; class Assembly; class Module; class AssemblyLoaderAllocator; +class LoaderAllocator; class AssemblyBinder { public: - HRESULT BindAssemblyByName(AssemblyNameData* pAssemblyNameData, BINDER_SPACE::Assembly** ppAssembly); + HRESULT BindAssemblyByName(AssemblyNameData* pAssemblyNameData, LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly); virtual HRESULT BindUsingPEImage(PEImage* pPEImage, BINDER_SPACE::Assembly** ppAssembly) = 0; - virtual HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, BINDER_SPACE::Assembly** ppAssembly) = 0; + virtual HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, + LoaderAllocator *pParentLoaderAllocator, + BINDER_SPACE::Assembly** ppAssembly) = 0; /// /// Get LoaderAllocator for binders that contain it. For other binders, return NULL. diff --git a/src/coreclr/vm/coreassemblyspec.cpp b/src/coreclr/vm/coreassemblyspec.cpp index 00231364042cf4..fcb17b1ac2ebdd 100644 --- a/src/coreclr/vm/coreassemblyspec.cpp +++ b/src/coreclr/vm/coreassemblyspec.cpp @@ -87,7 +87,19 @@ HRESULT AssemblySpec::Bind(AppDomain *pAppDomain, BINDER_SPACE::Assembly** ppAs { AssemblyNameData assemblyNameData = { 0 }; PopulateAssemblyNameData(assemblyNameData); - hr = pBinder->BindAssemblyByName(&assemblyNameData, &pPrivAsm); + + LoaderAllocator *pParentLoaderAllocator = NULL; + DomainAssembly *pParentAssembly = GetParentAssembly(); + if (pParentAssembly != NULL) + { + pParentLoaderAllocator = pParentAssembly->GetLoaderAllocator(); + } + else + { + pParentLoaderAllocator = pBinder->GetLoaderAllocator(); + } + + hr = pBinder->BindAssemblyByName(&assemblyNameData, pParentLoaderAllocator, &pPrivAsm); } if (SUCCEEDED(hr)) From 83f01722d8d8dafc22ad7756947ba30c3706f1cd Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 22 Apr 2022 21:35:15 +0200 Subject: [PATCH 2/6] Use LoaderAllocator from binder only --- src/coreclr/binder/customassemblybinder.cpp | 2 +- src/coreclr/binder/defaultassemblybinder.cpp | 2 +- src/coreclr/binder/inc/customassemblybinder.h | 1 - src/coreclr/binder/inc/defaultassemblybinder.h | 2 -- src/coreclr/vm/appdomain.cpp | 2 +- src/coreclr/vm/assemblybinder.cpp | 3 +-- src/coreclr/vm/assemblybinder.h | 7 ++----- src/coreclr/vm/coreassemblyspec.cpp | 14 +------------- 8 files changed, 7 insertions(+), 26 deletions(-) diff --git a/src/coreclr/binder/customassemblybinder.cpp b/src/coreclr/binder/customassemblybinder.cpp index b493d988936f87..42b8ce90136b51 100644 --- a/src/coreclr/binder/customassemblybinder.cpp +++ b/src/coreclr/binder/customassemblybinder.cpp @@ -39,7 +39,6 @@ HRESULT CustomAssemblyBinder::BindAssemblyByNameWorker(BINDER_SPACE::AssemblyNam } HRESULT CustomAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, - LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) { // When LoadContext needs to resolve an assembly reference, it will go through the following lookup order: @@ -73,6 +72,7 @@ HRESULT CustomAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName* // 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. // + LoaderAllocator* pParentLoaderAllocator = GetLoaderAllocator(); hr = AssemblyBinderCommon::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, m_pDefaultBinder, pParentLoaderAllocator, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) diff --git a/src/coreclr/binder/defaultassemblybinder.cpp b/src/coreclr/binder/defaultassemblybinder.cpp index 79bcea8d31e71a..69967a3c253bee 100644 --- a/src/coreclr/binder/defaultassemblybinder.cpp +++ b/src/coreclr/binder/defaultassemblybinder.cpp @@ -39,7 +39,6 @@ HRESULT DefaultAssemblyBinder::BindAssemblyByNameWorker(BINDER_SPACE::AssemblyNa // DefaultAssemblyBinder implementation // ============================================================================ HRESULT DefaultAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName *pAssemblyName, - LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly **ppAssembly) { HRESULT hr = S_OK; @@ -85,6 +84,7 @@ HRESULT DefaultAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName if (pManagedAssemblyLoadContext != NULL) { + LoaderAllocator* pParentLoaderAllocator = GetLoaderAllocator(); hr = AssemblyBinderCommon::BindUsingHostAssemblyResolver(pManagedAssemblyLoadContext, pAssemblyName, NULL, pParentLoaderAllocator, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) diff --git a/src/coreclr/binder/inc/customassemblybinder.h b/src/coreclr/binder/inc/customassemblybinder.h index cb317367e01ab5..9d59832f3c9798 100644 --- a/src/coreclr/binder/inc/customassemblybinder.h +++ b/src/coreclr/binder/inc/customassemblybinder.h @@ -21,7 +21,6 @@ class CustomAssemblyBinder final : public AssemblyBinder BINDER_SPACE::Assembly** ppAssembly) override; HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, - LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) override; AssemblyLoaderAllocator* GetLoaderAllocator() override; diff --git a/src/coreclr/binder/inc/defaultassemblybinder.h b/src/coreclr/binder/inc/defaultassemblybinder.h index 8235a28b357594..398174c65a078b 100644 --- a/src/coreclr/binder/inc/defaultassemblybinder.h +++ b/src/coreclr/binder/inc/defaultassemblybinder.h @@ -10,7 +10,6 @@ class PEAssembly; class PEImage; -class LoaderAllocator; class DefaultAssemblyBinder final : public AssemblyBinder { @@ -20,7 +19,6 @@ class DefaultAssemblyBinder final : public AssemblyBinder BINDER_SPACE::Assembly** ppAssembly) override; HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, - LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) override; AssemblyLoaderAllocator* GetLoaderAllocator() override diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 5b94292ee2472f..e8c4927104ed3b 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -4984,7 +4984,7 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB // Switch to pre-emp mode before calling into the binder GCX_PREEMP(); BINDER_SPACE::Assembly *pCoreCLRFoundAssembly = NULL; - hr = pDefaultBinder->BindUsingAssemblyName(pAssemblyName, pParentLoaderAllocator, &pCoreCLRFoundAssembly); + hr = pDefaultBinder->BindUsingAssemblyName(pAssemblyName, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { _ASSERTE(pCoreCLRFoundAssembly != NULL); diff --git a/src/coreclr/vm/assemblybinder.cpp b/src/coreclr/vm/assemblybinder.cpp index d2aa3d645752db..2e231741efbd34 100644 --- a/src/coreclr/vm/assemblybinder.cpp +++ b/src/coreclr/vm/assemblybinder.cpp @@ -9,7 +9,6 @@ #ifndef DACCESS_COMPILE HRESULT AssemblyBinder::BindAssemblyByName(AssemblyNameData* pAssemblyNameData, - LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly) { _ASSERTE(pAssemblyNameData != nullptr && ppAssembly != nullptr); @@ -21,7 +20,7 @@ HRESULT AssemblyBinder::BindAssemblyByName(AssemblyNameData* pAssemblyNameData, SAFE_NEW(pAssemblyName, BINDER_SPACE::AssemblyName); IF_FAIL_GO(pAssemblyName->Init(*pAssemblyNameData)); - hr = BindUsingAssemblyName(pAssemblyName, pParentLoaderAllocator, ppAssembly); + hr = BindUsingAssemblyName(pAssemblyName, ppAssembly); Exit: return hr; diff --git a/src/coreclr/vm/assemblybinder.h b/src/coreclr/vm/assemblybinder.h index 7e48e4770f6b35..e737d92c2de7c0 100644 --- a/src/coreclr/vm/assemblybinder.h +++ b/src/coreclr/vm/assemblybinder.h @@ -12,17 +12,14 @@ class NativeImage; class Assembly; class Module; class AssemblyLoaderAllocator; -class LoaderAllocator; class AssemblyBinder { public: - HRESULT BindAssemblyByName(AssemblyNameData* pAssemblyNameData, LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly** ppAssembly); + HRESULT BindAssemblyByName(AssemblyNameData* pAssemblyNameData, BINDER_SPACE::Assembly** ppAssembly); virtual HRESULT BindUsingPEImage(PEImage* pPEImage, BINDER_SPACE::Assembly** ppAssembly) = 0; - virtual HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, - LoaderAllocator *pParentLoaderAllocator, - BINDER_SPACE::Assembly** ppAssembly) = 0; + virtual HRESULT BindUsingAssemblyName(BINDER_SPACE::AssemblyName* pAssemblyName, BINDER_SPACE::Assembly** ppAssembly) = 0; /// /// Get LoaderAllocator for binders that contain it. For other binders, return NULL. diff --git a/src/coreclr/vm/coreassemblyspec.cpp b/src/coreclr/vm/coreassemblyspec.cpp index fcb17b1ac2ebdd..00231364042cf4 100644 --- a/src/coreclr/vm/coreassemblyspec.cpp +++ b/src/coreclr/vm/coreassemblyspec.cpp @@ -87,19 +87,7 @@ HRESULT AssemblySpec::Bind(AppDomain *pAppDomain, BINDER_SPACE::Assembly** ppAs { AssemblyNameData assemblyNameData = { 0 }; PopulateAssemblyNameData(assemblyNameData); - - LoaderAllocator *pParentLoaderAllocator = NULL; - DomainAssembly *pParentAssembly = GetParentAssembly(); - if (pParentAssembly != NULL) - { - pParentLoaderAllocator = pParentAssembly->GetLoaderAllocator(); - } - else - { - pParentLoaderAllocator = pBinder->GetLoaderAllocator(); - } - - hr = pBinder->BindAssemblyByName(&assemblyNameData, pParentLoaderAllocator, &pPrivAsm); + hr = pBinder->BindAssemblyByName(&assemblyNameData, &pPrivAsm); } if (SUCCEEDED(hr)) From d833894ea1c5c828503b4758e40b9ba275404305 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 22 Apr 2022 23:35:09 +0200 Subject: [PATCH 3/6] Pass binder instead of LoaderAllocator --- src/coreclr/binder/assemblybindercommon.cpp | 6 +++--- src/coreclr/binder/customassemblybinder.cpp | 3 +-- src/coreclr/binder/defaultassemblybinder.cpp | 3 +-- src/coreclr/binder/inc/assemblybindercommon.hpp | 3 +-- src/coreclr/vm/appdomain.cpp | 3 ++- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/coreclr/binder/assemblybindercommon.cpp b/src/coreclr/binder/assemblybindercommon.cpp index a06ea1f2f10392..466bdf41d6dfab 100644 --- a/src/coreclr/binder/assemblybindercommon.cpp +++ b/src/coreclr/binder/assemblybindercommon.cpp @@ -30,7 +30,7 @@ extern HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, DefaultAssemblyBinder *pDefaultBinder, - LoaderAllocator *pParentLoaderAllocator, + AssemblyBinder *pBinder, BINDER_SPACE::Assembly **ppLoadedAssembly); #endif // !defined(DACCESS_COMPILE) @@ -1156,7 +1156,7 @@ namespace BINDER_SPACE HRESULT AssemblyBinderCommon::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin, /* in */ AssemblyName *pAssemblyName, /* in */ DefaultAssemblyBinder *pDefaultBinder, - /* in */ LoaderAllocator *pParentLoaderAllocator, + /* in */ AssemblyBinder *pBinder, /* out */ Assembly **ppAssembly) { HRESULT hr = E_FAIL; @@ -1166,7 +1166,7 @@ HRESULT AssemblyBinderCommon::BindUsingHostAssemblyResolver(/* in */ INT_PTR pMa // RuntimeInvokeHostAssemblyResolver will perform steps 2-4 of CustomAssemblyBinder::BindAssemblyByName. BINDER_SPACE::Assembly *pLoadedAssembly = NULL; hr = RuntimeInvokeHostAssemblyResolver(pManagedAssemblyLoadContextToBindWithin, - pAssemblyName, pDefaultBinder, pParentLoaderAllocator, &pLoadedAssembly); + pAssemblyName, pDefaultBinder, pBinder, &pLoadedAssembly); if (SUCCEEDED(hr)) { _ASSERTE(pLoadedAssembly != NULL); diff --git a/src/coreclr/binder/customassemblybinder.cpp b/src/coreclr/binder/customassemblybinder.cpp index 42b8ce90136b51..b857526a2d34a0 100644 --- a/src/coreclr/binder/customassemblybinder.cpp +++ b/src/coreclr/binder/customassemblybinder.cpp @@ -72,9 +72,8 @@ HRESULT CustomAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName* // 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. // - LoaderAllocator* pParentLoaderAllocator = GetLoaderAllocator(); hr = AssemblyBinderCommon::BindUsingHostAssemblyResolver(GetManagedAssemblyLoadContext(), pAssemblyName, - m_pDefaultBinder, pParentLoaderAllocator, &pCoreCLRFoundAssembly); + m_pDefaultBinder, this, &pCoreCLRFoundAssembly); if (SUCCEEDED(hr)) { // We maybe returned an assembly that was bound to a different AssemblyBinder instance. diff --git a/src/coreclr/binder/defaultassemblybinder.cpp b/src/coreclr/binder/defaultassemblybinder.cpp index 69967a3c253bee..28157a11244cd0 100644 --- a/src/coreclr/binder/defaultassemblybinder.cpp +++ b/src/coreclr/binder/defaultassemblybinder.cpp @@ -84,9 +84,8 @@ HRESULT DefaultAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName if (pManagedAssemblyLoadContext != NULL) { - LoaderAllocator* pParentLoaderAllocator = GetLoaderAllocator(); hr = AssemblyBinderCommon::BindUsingHostAssemblyResolver(pManagedAssemblyLoadContext, pAssemblyName, - NULL, pParentLoaderAllocator, &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/assemblybindercommon.hpp b/src/coreclr/binder/inc/assemblybindercommon.hpp index 54dd1b0b0d583e..8fd43587bfd3a8 100644 --- a/src/coreclr/binder/inc/assemblybindercommon.hpp +++ b/src/coreclr/binder/inc/assemblybindercommon.hpp @@ -22,7 +22,6 @@ class AssemblyBinder; class DefaultAssemblyBinder; class PEAssembly; class PEImage; -class LoaderAllocator; namespace BINDER_SPACE { @@ -53,7 +52,7 @@ namespace BINDER_SPACE static HRESULT BindUsingHostAssemblyResolver (/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin, /* in */ AssemblyName *pAssemblyName, /* in */ DefaultAssemblyBinder *pDefaultBinder, - /* in */ LoaderAllocator *pParentLoaderAllocator, + /* in */ AssemblyBinder *pBinder, /* out */ Assembly **ppAssembly); static HRESULT BindUsingPEImage(/* in */ AssemblyBinder *pBinder, diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index e8c4927104ed3b..b0f7933a9e4de2 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -4906,7 +4906,7 @@ AppDomain::AssemblyIterator::Next_Unlocked( #if !defined(DACCESS_COMPILE) // Returns S_OK if the assembly was successfully loaded -HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, DefaultAssemblyBinder *pDefaultBinder, LoaderAllocator *pParentLoaderAllocator, BINDER_SPACE::Assembly **ppLoadedAssembly) +HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToBindWithin, BINDER_SPACE::AssemblyName *pAssemblyName, DefaultAssemblyBinder *pDefaultBinder, AssemblyBinder *pBinder, BINDER_SPACE::Assembly **ppLoadedAssembly) { CONTRACTL { @@ -5089,6 +5089,7 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB if (pDomainAssembly->IsCollectible()) { LoaderAllocator *pResultAssemblyLoaderAllocator = pDomainAssembly->GetLoaderAllocator(); + LoaderAllocator *pParentLoaderAllocator = pBinder->GetLoaderAllocator(); _ASSERTE(pParentLoaderAllocator); _ASSERTE(pResultAssemblyLoaderAllocator); pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); From 08c9fb2a85f513032655262c978adadbf3aac15f Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 26 Apr 2022 15:43:35 +0200 Subject: [PATCH 4/6] Move clearing of the m_pAssemblyLoaderAllocator to a later stage --- src/coreclr/binder/customassemblybinder.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/coreclr/binder/customassemblybinder.cpp b/src/coreclr/binder/customassemblybinder.cpp index b857526a2d34a0..98e0fd25de81b6 100644 --- a/src/coreclr/binder/customassemblybinder.cpp +++ b/src/coreclr/binder/customassemblybinder.cpp @@ -225,9 +225,13 @@ void CustomAssemblyBinder::PrepareForLoadContextRelease(INT_PTR ptrManagedStrong // 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)); @@ -252,6 +256,10 @@ void CustomAssemblyBinder::ReleaseLoadContext() handle = reinterpret_cast(m_ptrManagedStrongAssemblyLoadContext); DestroyHandle(handle); SetManagedAssemblyLoadContext(NULL); + + // The AssemblyLoaderAllocator is in a process of shutdown and should not be used + // after this point. + m_pAssemblyLoaderAllocator = NULL; } #endif // !defined(DACCESS_COMPILE) From 97956f53d781b048b33a5363f3863664c752ce9c Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 27 Apr 2022 00:08:00 +0200 Subject: [PATCH 5/6] Throw exception on collectible assembly in non-collectible ALC --- src/coreclr/vm/appdomain.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index b0f7933a9e4de2..1129bcdec1d350 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -5090,7 +5090,12 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB { LoaderAllocator *pResultAssemblyLoaderAllocator = pDomainAssembly->GetLoaderAllocator(); LoaderAllocator *pParentLoaderAllocator = pBinder->GetLoaderAllocator(); - _ASSERTE(pParentLoaderAllocator); + if (pParentLoaderAllocator == NULL) + { + // The AssemblyLoadContext for which we are resolving the the Assembly is not collectible. + COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleBoundNonCollectible")); + } + _ASSERTE(pResultAssemblyLoaderAllocator); pParentLoaderAllocator->EnsureReference(pResultAssemblyLoaderAllocator); } From e5a73df626d451a29b8c34b411ddc14f90f060e3 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 27 Apr 2022 15:23:33 +0200 Subject: [PATCH 6/6] Update the ResolvedFromDifferentContext test Make it test that we throw an exception when the parent ALC is not collectible. --- .../ResolvedFromDifferentContext.cs | 129 ++++++++++++------ 1 file changed, 91 insertions(+), 38 deletions(-) diff --git a/src/tests/Loader/CollectibleAssemblies/ResolvedFromDifferentContext/ResolvedFromDifferentContext.cs b/src/tests/Loader/CollectibleAssemblies/ResolvedFromDifferentContext/ResolvedFromDifferentContext.cs index 6dbe8bc79c8310..1dff2299104f07 100644 --- a/src/tests/Loader/CollectibleAssemblies/ResolvedFromDifferentContext/ResolvedFromDifferentContext.cs +++ b/src/tests/Loader/CollectibleAssemblies/ResolvedFromDifferentContext/ResolvedFromDifferentContext.cs @@ -30,8 +30,8 @@ protected override Assembly Load(AssemblyName assemblyName) if (assemblyName.Name == "TestInterface") { AssemblyLoadContext alc1 = new AssemblyLoadContext("Dependencies", true); - Console.WriteLine($"Loading TestInterface by alc {alc1} for alc {this}"); - Assembly a = alc1.LoadFromAssemblyPath(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, @"..\TestInterface\TestInterface.dll")); + Console.WriteLine($"Loading TestInterface by alc {alc1} for {(IsCollectible ? "collectible" : "non-collectible")} alc {this}"); + Assembly a = alc1.LoadFromAssemblyPath(Test.GetTestAssemblyPath(@"..\TestInterface\TestInterface.dll")); interfaceAssemblyRef = new WeakReference(a); return a; } @@ -45,13 +45,18 @@ class Test static AssemblyLoadContext alc1 = null; static WeakReference interfaceAssemblyRef = null; + public static string GetTestAssemblyPath(string subPath) + { + return Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), subPath); + } + [MethodImpl(MethodImplOptions.NoInlining)] - public static Assembly LoadUsingResolvingEvent() + private static Assembly LoadUsingResolvingEvent(bool collectibleParent) { alc1 = new AssemblyLoadContext("Dependencies", true); - AssemblyLoadContext alc2 = new AssemblyLoadContext("Test1", true); + AssemblyLoadContext alc2 = new AssemblyLoadContext("Test1", collectibleParent); alc2.Resolving += Alc2_Resolving; - Assembly assembly = alc2.LoadFromAssemblyPath(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, @"..\TestClass\TestClass.dll")); + Assembly assembly = alc2.LoadFromAssemblyPath(Test.GetTestAssemblyPath(@"..\TestClass\TestClass.dll")); Type t = assembly.GetType("TestClass.Class"); Console.WriteLine($"Type {t} obtained"); @@ -70,8 +75,8 @@ private static Assembly Alc2_Resolving(AssemblyLoadContext arg1, AssemblyName ar Console.WriteLine($"Resolving event by alc {alc1} for alc {arg1}"); if (alc1 != null && arg2.Name == "TestInterface") { - Console.WriteLine($"Loading TestInterface by alc {alc1} for alc {arg1}"); - Assembly a = alc1.LoadFromAssemblyPath(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, @"..\TestInterface\TestInterface.dll")); + Console.WriteLine($"Loading TestInterface by alc {alc1} for {(arg1.IsCollectible ? "collectible" : "non-collectible")} alc {arg1}"); + Assembly a = alc1.LoadFromAssemblyPath(Test.GetTestAssemblyPath(@"..\TestInterface\TestInterface.dll")); interfaceAssemblyRef = new WeakReference(a); return a; } @@ -80,10 +85,10 @@ private static Assembly Alc2_Resolving(AssemblyLoadContext arg1, AssemblyName ar } [MethodImpl(MethodImplOptions.NoInlining)] - public static Assembly LoadUsingLoadOverride() + private static Assembly LoadUsingLoadOverride(bool collectibleParent) { - TestAssemblyLoadContext alc2 = new TestAssemblyLoadContext("Test2", true); - Assembly assembly = alc2.LoadFromAssemblyPath(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, @"..\TestClass\TestClass.dll")); + TestAssemblyLoadContext alc2 = new TestAssemblyLoadContext("Test2", collectibleParent); + Assembly assembly = alc2.LoadFromAssemblyPath(Test.GetTestAssemblyPath(@"..\TestClass\TestClass.dll")); Type t = assembly.GetType("TestClass.Class"); @@ -94,18 +99,33 @@ public static Assembly LoadUsingLoadOverride() return assembly; } + private enum TestCase + { + ResolvingEvent, + LoadOverride, + ResolvingEventInNonCollectible, + LoadOverrideInNonCollectible + } + [MethodImpl(MethodImplOptions.NoInlining)] - private static WeakReference TestDependencies(string testCase) + private static WeakReference TestDependencies(TestCase testCase) { - Assembly assembly; + Assembly assembly = null; - if (testCase == "ResolvingEvent") - { - assembly = LoadUsingResolvingEvent(); - } - else + switch (testCase) { - assembly = LoadUsingLoadOverride(); + case TestCase.ResolvingEvent: + assembly = LoadUsingResolvingEvent(collectibleParent: true); + break; + case TestCase.LoadOverride: + assembly = LoadUsingLoadOverride(collectibleParent: true); + break; + case TestCase.ResolvingEventInNonCollectible: + assembly = LoadUsingResolvingEvent(collectibleParent: false); + break; + case TestCase.LoadOverrideInNonCollectible: + assembly = LoadUsingLoadOverride(collectibleParent: false); + break; } for (int i = 0; interfaceAssemblyRef.IsAlive && (i < 10); i++) @@ -124,45 +144,78 @@ private static WeakReference TestDependencies(string testCase) return new WeakReference(assembly); } - public static int TestFullUnload(string testCase) + private static bool ShouldThrow(TestCase testCase) { - Console.WriteLine($"Running test case {testCase}"); + return (testCase == TestCase.LoadOverrideInNonCollectible) || (testCase == TestCase.ResolvingEventInNonCollectible); + } - WeakReference assemblyRef = TestDependencies(testCase); - if (assemblyRef == null) - { - return 101; - } + private static int TestFullUnload(TestCase testCase) + { + Console.WriteLine($"Running test case {testCase}"); - for (int i = 0; assemblyRef.IsAlive && (i < 10); i++) + try { - GC.Collect(); - GC.WaitForPendingFinalizers(); + WeakReference assemblyRef = TestDependencies(testCase); + if (assemblyRef == null) + { + return 101; + } + + for (int i = 0; assemblyRef.IsAlive && (i < 10); i++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + + if (assemblyRef.IsAlive) + { + Console.WriteLine("Failed to unload alc2"); + return 102; + } + + if (interfaceAssemblyRef.IsAlive) + { + Console.WriteLine("Failed to unload alc1"); + return 103; + } + + Console.WriteLine(); } - - if (assemblyRef.IsAlive) + catch (System.IO.FileLoadException e) { - Console.WriteLine("Failed to unload alc2"); - return 102; + if (!ShouldThrow(testCase)) + { + Console.WriteLine("Failure - unexpected exception"); + return 104; + } + if ((e.InnerException == null) || e.InnerException.GetType() != typeof(System.NotSupportedException)) + { + Console.WriteLine($"Failure - unexpected exception type {e.InnerException}"); + return 105; + } + + return 100; } - if (interfaceAssemblyRef.IsAlive) + if (ShouldThrow(testCase)) { - Console.WriteLine("Failed to unload alc1"); - return 103; + Console.WriteLine("Failure - resolved collectible assembly into non-collectible context without throwing exception"); + return 106; } - Console.WriteLine(); return 100; - } public static int Main(string[] args) { int status = 100; - foreach (string testCase in new string[] {"LoadOverride", "ResolvingEvent"}) + foreach (TestCase testCase in Enum.GetValues(typeof(TestCase))) { status = TestFullUnload(testCase); + if (status != 100) + { + break; + } } return status;