Skip to content

Fix race on DomainAssembly/Assembly/Module creation that results in incorrectly triggered ICorProfiler callbacks #120455

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.

Sign up for GitHub

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

Merged
elinor-fung merged 4 commits into from
Oct 7, 2025
Merged

Fix race on DomainAssembly/Assembly/Module creation that results in incorrectly triggered ICorProfiler callbacks #120455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3aa8a1
Fix race on assembly initialize - delay assembly creation until after…
elinor-fung Oct 4, 2025
26400c1
Update moduleload profiler test
elinor-fung Oct 6, 2025
01fa34a
Apply suggestions from code review
elinor-fung Oct 7, 2025
3a9aaf2
PR feedback - asserts
elinor-fung Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 84 additions & 52 deletions src/coreclr/vm/appdomain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1890,7 +1890,7 @@ DispIDCache* AppDomain::SetupRefDispIDCache()

#endif // FEATURE_COMINTEROP

FileLoadLock *FileLoadLock::Create(PEFileListLock *pLock, PEAssembly * pPEAssembly, Assembly *pAssembly)
FileLoadLock* FileLoadLock::Create(PEFileListLock* pLock, PEAssembly* pPEAssembly)
{
CONTRACTL
{
Expand All @@ -1903,7 +1903,7 @@ FileLoadLock *FileLoadLock::Create(PEFileListLock *pLock, PEAssembly * pPEAssemb
}
CONTRACTL_END;

NewHolder<FileLoadLock> result(new FileLoadLock(pLock, pPEAssembly, pAssembly));
NewHolder<FileLoadLock> result(new FileLoadLock(pLock, pPEAssembly));

pLock->AddElement(result);
result->AddRef(); // Add one ref on behalf of the ListLock's reference. The corresponding Release() happens in FileLoadLock::CompleteLoadLevel.
Expand All @@ -1929,6 +1929,14 @@ Assembly *FileLoadLock::GetAssembly()
return m_pAssembly;
}

PEAssembly* FileLoadLock::GetPEAssembly()
{
LIMITED_METHOD_CONTRACT;
// Underlying PEAssembly pointer is stored in the constructor in base ListLockEntry::m_data.
_ASSERTE(m_data != NULL);
return (PEAssembly*)m_data;
}

FileLoadLevel FileLoadLock::GetLoadLevel()
{
LIMITED_METHOD_CONTRACT;
Expand Down Expand Up @@ -1977,6 +1985,7 @@ BOOL FileLoadLock::CanAcquire(FileLoadLevel targetLevel)
static const char *fileLoadLevelName[] =
{
"CREATE", // FILE_LOAD_CREATE
"ALLOCATE", // FILE_LOAD_ALLOCATE
"BEGIN", // FILE_LOAD_BEGIN
"BEFORE_TYPE_LOAD", // FILE_LOAD_BEFORE_TYPE_LOAD
"EAGER_FIXUPS", // FILE_LOAD_EAGER_FIXUPS
Expand All @@ -2002,7 +2011,9 @@ BOOL FileLoadLock::CompleteLoadLevel(FileLoadLevel level, BOOL success)
if (level > m_level)
{
// Must complete each level in turn, unless we have an error
CONSISTENCY_CHECK(m_pAssembly->IsError() || (level == (m_level+1)));
CONSISTENCY_CHECK((level == (m_level+1)) || (m_pAssembly != nullptr && m_pAssembly->IsError()));
CONSISTENCY_CHECK(m_pAssembly != nullptr || level < FILE_LOAD_ALLOCATE);

// Remove the lock from the list if the load is completed
if (level >= FILE_ACTIVE)
{
Expand Down Expand Up @@ -2038,7 +2049,7 @@ BOOL FileLoadLock::CompleteLoadLevel(FileLoadLevel level, BOOL success)
{
m_level = (FileLoadLevel)level;

if (success)
if (success && level >= FILE_LOAD_ALLOCATE)
m_pAssembly->SetLoadLevel(level);
}

Expand All @@ -2061,6 +2072,18 @@ BOOL FileLoadLock::CompleteLoadLevel(FileLoadLevel level, BOOL success)
return FALSE;
}

void FileLoadLock::SetAssembly(Assembly* pAssembly)
{
LIMITED_METHOD_CONTRACT;

_ASSERTE(HasLock());
_ASSERTE(m_level == FILE_LOAD_CREATE); // Only valid to set during CREATE -> ALLOCATE
_ASSERTE(m_pAssembly == nullptr);
_ASSERTE(pAssembly != nullptr && pAssembly->GetPEAssembly() == (PEAssembly *)m_data);

m_pAssembly = pAssembly;
}

void FileLoadLock::SetError(Exception *ex)
{
CONTRACTL
Expand Down Expand Up @@ -2107,10 +2130,10 @@ UINT32 FileLoadLock::Release()
return count;
}

FileLoadLock::FileLoadLock(PEFileListLock *pLock, PEAssembly * pPEAssembly, Assembly *pAssembly)
FileLoadLock::FileLoadLock(PEFileListLock* pLock, PEAssembly* pPEAssembly)
: ListLockEntry(pLock, pPEAssembly, "File load lock"),
m_level((FileLoadLevel) (FILE_LOAD_CREATE)),
m_pAssembly(pAssembly),
m_pAssembly(nullptr),
m_cachedHR(S_OK)
{
WRAPPER_NO_CONTRACT;
Expand Down Expand Up @@ -2439,23 +2462,6 @@ Assembly *AppDomain::LoadAssemblyInternal(AssemblySpec* pIdentity,

if (result == NULL)
{
LoaderAllocator *pLoaderAllocator = NULL;

AssemblyBinder *pAssemblyBinder = pPEAssembly->GetAssemblyBinder();
// Assemblies loaded with CustomAssemblyBinder need to use a different LoaderAllocator if
// marked as collectible
pLoaderAllocator = pAssemblyBinder->GetLoaderAllocator();
if (pLoaderAllocator == NULL)
{
pLoaderAllocator = this->GetLoaderAllocator();
}

// Allocate the DomainAssembly a bit early to avoid GC mode problems. We could potentially avoid
// a rare redundant allocation by moving this closer to FileLoadLock::Create, but it's not worth it.
AllocMemTracker amTracker;
AllocMemTracker *pamTracker = &amTracker;
NewHolder<DomainAssembly> pDomainAssembly = new DomainAssembly(pPEAssembly, pLoaderAllocator, pamTracker);

LoadLockHolder lock(this);

// Find the list lock entry
Expand All @@ -2467,20 +2473,9 @@ Assembly *AppDomain::LoadAssemblyInternal(AssemblySpec* pIdentity,
result = FindAssembly(pPEAssembly, FindAssemblyOptions_IncludeFailedToLoad);
if (result == NULL)
{
// We are the first one in - create the DomainAssembly
// We are the first one in - create the FileLoadLock. Creation of the Assembly will happen at FILE_LOAD_ALLOCATE stage
registerNewAssembly = true;
fileLock = FileLoadLock::Create(lock, pPEAssembly, pDomainAssembly->GetAssembly());
pDomainAssembly.SuppressRelease();
pamTracker->SuppressRelease();

// Set the assembly module to be tenured now that we know it won't be deleted
pDomainAssembly->GetAssembly()->SetIsTenured();
if (pDomainAssembly->GetAssembly()->IsCollectible())
{
// We add the assembly to the LoaderAllocator only when we are sure that it can be added
// and won't be deleted in case of a concurrent load from the same ALC
((AssemblyLoaderAllocator *)pLoaderAllocator)->AddDomainAssembly(pDomainAssembly);
}
fileLock = FileLoadLock::Create(lock, pPEAssembly);
}
}
else
Expand All @@ -2498,6 +2493,8 @@ Assembly *AppDomain::LoadAssemblyInternal(AssemblySpec* pIdentity,
// so it will not be removed until app domain unload. So there is no need
// to release our ref count.
result = LoadAssembly(fileLock, targetLevel);
// By now FILE_LOAD_ALLOCATE should have run and the Assembly should exist
_ASSERTE(result != NULL);
}
else
{
Expand All @@ -2506,7 +2503,7 @@ Assembly *AppDomain::LoadAssemblyInternal(AssemblySpec* pIdentity,

if (registerNewAssembly)
{
pPEAssembly->GetAssemblyBinder()->AddLoadedAssembly(pDomainAssembly->GetAssembly());
pPEAssembly->GetAssemblyBinder()->AddLoadedAssembly(result);
}
}
else
Expand Down Expand Up @@ -2536,6 +2533,7 @@ Assembly *AppDomain::LoadAssembly(FileLoadLock *pLock, FileLoadLevel targetLevel
STANDARD_VM_CHECK;
PRECONDITION(CheckPointer(pLock));
PRECONDITION(AppDomain::GetCurrentDomain() == this);
PRECONDITION(targetLevel >= FILE_LOAD_ALLOCATE);
POSTCONDITION(RETVAL->GetLoadLevel() >= GetCurrentFileLoadLevel()
|| RETVAL->GetLoadLevel() >= targetLevel);
POSTCONDITION(RETVAL->CheckNoError(targetLevel));
Expand All @@ -2550,6 +2548,7 @@ Assembly *AppDomain::LoadAssembly(FileLoadLock *pLock, FileLoadLevel targetLevel
// Do a quick out check for the already loaded case.
if (pLock->GetLoadLevel() >= targetLevel)
{
_ASSERTE(pAssembly != nullptr);
pAssembly->ThrowIfError(targetLevel);

RETURN pAssembly;
Expand All @@ -2572,8 +2571,9 @@ Assembly *AppDomain::LoadAssembly(FileLoadLock *pLock, FileLoadLevel targetLevel
if (immediateTargetLevel > limit.GetLoadLevel())
immediateTargetLevel = limit.GetLoadLevel();

const char *simpleName = pLock->GetPEAssembly()->GetSimpleName();
LOG((LF_LOADER, LL_INFO100, "LOADER: ***%s*\t>>>Load initiated, %s/%s\n",
pAssembly->GetSimpleName(),
simpleName,
fileLoadLevelName[immediateTargetLevel], fileLoadLevelName[targetLevel]));

// Now loop and do the load incrementally to the target level.
Expand All @@ -2596,30 +2596,32 @@ Assembly *AppDomain::LoadAssembly(FileLoadLock *pLock, FileLoadLevel targetLevel

LOG((LF_LOADER,
(workLevel == FILE_LOAD_BEGIN
|| workLevel == FILE_LOADED
|| workLevel == FILE_ACTIVE)
? LL_INFO10 : LL_INFO1000,
"LOADER: %p:***%s*\t loading at level %s\n",
this, pAssembly->GetSimpleName(), fileLoadLevelName[workLevel]));
|| workLevel == FILE_LOADED
|| workLevel == FILE_ACTIVE)
? LL_INFO10 : LL_INFO1000,
"LOADER: %p:***%s*\t loading at level %s\n",
this, simpleName, fileLoadLevelName[workLevel]));

TryIncrementalLoad(pAssembly, workLevel, fileLock);
TryIncrementalLoad(workLevel, fileLock);
}
}

if (pLock->GetLoadLevel() == immediateTargetLevel-1)
{
LOG((LF_LOADER, LL_INFO100, "LOADER: ***%s*\t<<<Load limited due to detected deadlock, %s\n",
pAssembly->GetSimpleName(),
simpleName,
fileLoadLevelName[immediateTargetLevel-1]));
}
}

LOG((LF_LOADER, LL_INFO100, "LOADER: ***%s*\t<<<Load completed, %s\n",
pAssembly->GetSimpleName(),
simpleName,
fileLoadLevelName[pLock->GetLoadLevel()]));

}

pAssembly = pLock->GetAssembly();
_ASSERTE(pAssembly != nullptr); // We should always be loading to at least FILE_LOAD_ALLOCATE, so the assembly should be created

// There may have been an error stored on the domain file by another thread, or from a previous load
pAssembly->ThrowIfError(targetLevel);

Expand All @@ -2643,19 +2645,49 @@ Assembly *AppDomain::LoadAssembly(FileLoadLock *pLock, FileLoadLevel targetLevel
RETURN pAssembly;
}

void AppDomain::TryIncrementalLoad(Assembly *pAssembly, FileLoadLevel workLevel, FileLoadLockHolder &lockHolder)
void AppDomain::TryIncrementalLoad(FileLoadLevel workLevel, FileLoadLockHolder& lockHolder)
{
STANDARD_VM_CONTRACT;

// This is factored out so we don't call EX_TRY in a loop (EX_TRY can _alloca)

BOOL released = FALSE;
FileLoadLock* pLoadLock = lockHolder.GetValue();
Assembly* pAssembly = pLoadLock->GetAssembly();

EX_TRY
{
// Do the work
BOOL success = pAssembly->DoIncrementalLoad(workLevel);
BOOL success;
if (workLevel == FILE_LOAD_ALLOCATE)
{
// FileLoadLock should not have an assembly yet
_ASSERTE(pAssembly == NULL);

// Allocate DomainAssembly & Assembly
PEAssembly *pPEAssembly = pLoadLock->GetPEAssembly();
AssemblyBinder *pAssemblyBinder = pPEAssembly->GetAssemblyBinder();
LoaderAllocator *pLoaderAllocator = pAssemblyBinder->GetLoaderAllocator();
if (pLoaderAllocator == NULL)
pLoaderAllocator = this->GetLoaderAllocator();

AllocMemTracker amTracker;
AllocMemTracker *pamTracker = &amTracker;
NewHolder<DomainAssembly> pDomainAssembly = new DomainAssembly(pPEAssembly, pLoaderAllocator, pamTracker);
pLoadLock->SetAssembly(pDomainAssembly->GetAssembly());
pDomainAssembly->GetAssembly()->SetIsTenured();
if (pDomainAssembly->GetAssembly()->IsCollectible())
{
((AssemblyLoaderAllocator *)pLoaderAllocator)->AddDomainAssembly(pDomainAssembly);
}
pDomainAssembly.SuppressRelease();
pamTracker->SuppressRelease();
pAssembly = pLoadLock->GetAssembly();
success = TRUE;
}
else
{
success = pAssembly->DoIncrementalLoad(workLevel);
}

// Complete the level.
if (pLoadLock->CompleteLoadLevel(workLevel, success) &&
Expand All @@ -2670,9 +2702,9 @@ void AppDomain::TryIncrementalLoad(Assembly *pAssembly, FileLoadLevel workLevel,
{
Exception *pEx = GET_EXCEPTION();

//We will cache this error and wire this load to forever fail,
// We will cache this error and wire this load to forever fail,
// unless the exception is transient or the file is loaded OK but just cannot execute
if (!pEx->IsTransient() && !pAssembly->IsLoaded())
if (pAssembly != nullptr && !pEx->IsTransient() && !pAssembly->IsLoaded())
{
if (released)
{
Expand Down
12 changes: 8 additions & 4 deletions src/coreclr/vm/appdomain.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,14 +293,15 @@ class FileLoadLock : public ListLockEntry
{
private:
FileLoadLevel m_level;
Assembly* m_pAssembly;
Assembly* m_pAssembly; // Will be null until FILE_LOAD_ALLOCATE is completed successfully
HRESULT m_cachedHR;

public:
static FileLoadLock *Create(PEFileListLock *pLock, PEAssembly *pPEAssembly, Assembly *pAssembly);
static FileLoadLock* Create(PEFileListLock* pLock, PEAssembly* pPEAssembly);

~FileLoadLock();
Assembly *GetAssembly();
PEAssembly* GetPEAssembly();
FileLoadLevel GetLoadLevel();

// CanAcquire will return FALSE if Acquire will definitely not take the lock due
Expand All @@ -320,14 +321,17 @@ class FileLoadLock : public ListLockEntry
// returns TRUE if it updated load level, FALSE if the level was set already
BOOL CompleteLoadLevel(FileLoadLevel level, BOOL success);

// Associate an Assembly with this lock
void SetAssembly(Assembly* pAssembly);

void SetError(Exception *ex);

void AddRef();
UINT32 Release() DAC_EMPTY_RET(0);

private:

FileLoadLock(PEFileListLock *pLock, PEAssembly *pPEAssembly, Assembly *pAssembly);
FileLoadLock(PEFileListLock* pLock, PEAssembly* pPEAssembly);

static void HolderLeave(FileLoadLock *pThis);

Expand Down Expand Up @@ -1098,7 +1102,7 @@ class AppDomain final

Assembly *LoadAssembly(FileLoadLock *pLock, FileLoadLevel targetLevel);

void TryIncrementalLoad(Assembly *pFile, FileLoadLevel workLevel, FileLoadLockHolder &lockHolder);
void TryIncrementalLoad(FileLoadLevel workLevel, FileLoadLockHolder& lockHolder);

#ifndef DACCESS_COMPILE // needs AssemblySpec
public:
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/assemblyspec.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ enum FileLoadLevel
// Note that semantics here are description is the LAST step done, not what is
// currently being done.

FILE_LOAD_CREATE,
FILE_LOAD_CREATE, // List entry + FileLoadLock created, no Assembly/DomainAssembly yet
FILE_LOAD_ALLOCATE, // DomainAssembly & Assembly object allocated and associated with the lock
FILE_LOAD_BEGIN,
FILE_LOAD_BEFORE_TYPE_LOAD,
FILE_LOAD_EAGER_FIXUPS,
Expand Down
16 changes: 15 additions & 1 deletion src/tests/profiler/unittest/moduleload.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Threading;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.Loader;

namespace Profiler.Tests
{
Expand All @@ -21,13 +23,25 @@ public static int RunTest(string[] args)
.DefineDynamicModule("TestModule")
.DefineType("TestClass", TypeAttributes.Public)
.CreateType();

var obj = Activator.CreateInstance(type);
if (obj == null)
{
throw new NullReferenceException();
}

// Trigger module load in multiple threads
int threadCount = 20;
List<Thread> threads = new(threadCount);
for (int i = 0; i < threadCount; i++)
threads.Add(new Thread(() => AssemblyLoadContext.Default.LoadFromAssemblyName(new AssemblyName("unloadlibrary"))));

foreach (var thread in threads)
thread.Start();

foreach (var thread in threads)
thread.Join();

return 100;
}

Expand Down
1 change: 1 addition & 0 deletions src/tests/profiler/unittest/moduleload.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<Compile Include="$(MSBuildProjectName).cs" />
<ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
<ProjectReference Include="../common/profiler_common.csproj" />
<ProjectReference Include="unloadlibrary.csproj" />
<CMakeProjectReference Include="$(MSBuildThisFileDirectory)/../native/CMakeLists.txt" />
</ItemGroup>
</Project>