From d144919c20cad371ab0b720cbb1d9af776b81c72 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 21 Apr 2022 18:12:21 +0200 Subject: [PATCH 1/2] Fix locking around m_LoaderAllocatorReferences iteration The iteration that marks loader allocators in LoaderAllocator::Mark() was not being called under an appropriate lock, which lead to a rare and hard to reproduce race condition and a crash in the LoaderAllocator::Mark(). The issue happened when the m_LoaderAllocatorReferences.Begin() returned an iterator at the time the m_LoaderAllocatorReferences were empty and another thread has written something into those references before the m_LoaderAllocatorReferences.End() was called. The iter was then evaluated as not equal to the End and the iter was getting dereferenced. Since the iterator had m_table set to NULL, the dereferencing caused crash. --- src/coreclr/vm/loaderallocator.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index cbfda127700074..0488e34d6f3dad 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -384,6 +384,7 @@ LoaderAllocator * LoaderAllocator::GCLoaderAllocators_RemoveAssemblies(AppDomain AppDomain::AssemblyIterator i; // Iterate through every loader allocator, marking as we go { + CrstHolder chLoaderAllocatorReferencesLock(pAppDomain->GetLoaderAllocatorReferencesLock()); CrstHolder chAssemblyListLock(pAppDomain->GetAssemblyListLock()); i = pAppDomain->IterateAssembliesEx((AssemblyIterationFlags)( From 73fc6ec1a464e0651db923f826dd6d675e9f1b9f Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 21 Apr 2022 20:11:56 +0200 Subject: [PATCH 2/2] Reflect PR feedback --- src/coreclr/vm/loaderallocator.cpp | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index 0488e34d6f3dad..b234ede563d15c 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -382,8 +382,8 @@ 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()); @@ -406,17 +406,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())) {