From bedf4e3deba9f33defe1e8070c179686a9e69991 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Sat, 19 Sep 2026 12:29:24 -0400 Subject: [PATCH 1/2] Fix type event cache lifetime race Keep global and per-module type cache access under the same lock so tracing disable and module unload cannot reclaim borrowed cache state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/eventtrace.cpp | 153 ++++++++++++++-------------------- 1 file changed, 63 insertions(+), 90 deletions(-) diff --git a/src/coreclr/vm/eventtrace.cpp b/src/coreclr/vm/eventtrace.cpp index 885ecb33feeb2c..fb0a9e1f0bfc44 100644 --- a/src/coreclr/vm/eventtrace.cpp +++ b/src/coreclr/vm/eventtrace.cpp @@ -1503,122 +1503,95 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p } CONTRACTL_END; - BOOL fSucceeded = FALSE; - - { - CrstHolder _crst(GetHashCrst()); - - // Check if ETW is enabled, and if not, bail here. - // We do this inside of the lock to ensure that we don't immediately - // re-allocate the global type hash after it has been cleaned up. - if (!ETW_TRACING_CATEGORY_ENABLED( - MICROSOFT_WINDOWS_DOTNETRUNTIME_PROVIDER_DOTNET_Context, - TRACE_LEVEL_INFORMATION, - CLR_TYPE_KEYWORD)) - { - *pfCreatedNew = FALSE; - return fSucceeded; - } + *pfCreatedNew = FALSE; - if (s_pAllLoggedTypes == NULL) - { - s_pAllLoggedTypes = new (nothrow) AllLoggedTypes; - if (s_pAllLoggedTypes == NULL) - { - // out of memory. Bail on ETW stuff - *pfCreatedNew = FALSE; - return fSucceeded; - } - } - } + BOOL fSucceeded = FALSE; - // Step 1: go from LoaderModule to hash of types. Module * pLoaderModule = th.GetLoaderModule(); _ASSERTE(pLoaderModule != NULL); - LoggedTypesFromModule * pLoggedTypesFromModule = nullptr; - { - CrstHolder _crst(GetHashCrst()); - pLoggedTypesFromModule = s_pAllLoggedTypes->allLoggedTypesHash.Lookup(pLoaderModule); - } + TypeLoggingInfo typeLoggingInfoNew(th); + LoggedTypesFromModule * pNewLoggedTypesFromModule = NULL; - if (pLoggedTypesFromModule == NULL) + while (TRUE) { - pLoggedTypesFromModule = new (nothrow) LoggedTypesFromModule(pLoaderModule); - if (pLoggedTypesFromModule == NULL) - { - // out of memory. Bail on ETW stuff - *pfCreatedNew = FALSE; - return fSucceeded; - } + BOOL fAllocateLoggedTypesFromModule = FALSE; + { CrstHolder _crst(GetHashCrst()); - // recheck if the type has been added by another thread since we last checked above - LoggedTypesFromModule * recheckLoggedTypesFromModule = s_pAllLoggedTypes->allLoggedTypesHash.Lookup(pLoaderModule); - if (recheckLoggedTypesFromModule == NULL) + + // Check if ETW is enabled, and if not, bail here. + // We do this inside of the lock to ensure that we don't immediately + // re-allocate the global type hash after it has been cleaned up. + if (!ETW_TRACING_CATEGORY_ENABLED( + MICROSOFT_WINDOWS_DOTNETRUNTIME_PROVIDER_DOTNET_Context, + TRACE_LEVEL_INFORMATION, + CLR_TYPE_KEYWORD)) { - EX_TRY + break; + } + + if (s_pAllLoggedTypes == NULL) + { + s_pAllLoggedTypes = new (nothrow) AllLoggedTypes; + if (s_pAllLoggedTypes == NULL) { - s_pAllLoggedTypes->allLoggedTypesHash.Add(pLoggedTypesFromModule); - fSucceeded = TRUE; + break; } - EX_SWALLOW_NONTERMINAL } - else + + LoggedTypesFromModule * pLoggedTypesFromModule = + s_pAllLoggedTypes->allLoggedTypesHash.Lookup(pLoaderModule); + if (pLoggedTypesFromModule == NULL) { - delete pLoggedTypesFromModule; - pLoggedTypesFromModule = recheckLoggedTypesFromModule; + if (pNewLoggedTypesFromModule == NULL) + { + fAllocateLoggedTypesFromModule = TRUE; + } + else + { + EX_TRY + { + s_pAllLoggedTypes->allLoggedTypesHash.Add(pNewLoggedTypesFromModule); + pLoggedTypesFromModule = pNewLoggedTypesFromModule; + pNewLoggedTypesFromModule = NULL; + } + EX_SWALLOW_NONTERMINAL + } } - if (!fSucceeded) + if (!fAllocateLoggedTypesFromModule && pLoggedTypesFromModule != NULL) { - *pfCreatedNew = FALSE; - return fSucceeded; + // The cache objects must not be used after releasing this lock because + // provider disable and module unload can delete them under the same lock. + if (pLoggedTypesFromModule->loggedTypesFromModuleHash.Lookup(th).th.IsNull()) + { + EX_TRY + { + pLoggedTypesFromModule->loggedTypesFromModuleHash.Add(typeLoggingInfoNew); + fSucceeded = TRUE; + } + EX_SWALLOW_NONTERMINAL + } + + break; } } - } - // Step 2: From hash of types, see if our TypeHandle is there already - TypeLoggingInfo typeLoggingInfoPreexisting; - { - CrstHolder _crst(GetHashCrst()); - typeLoggingInfoPreexisting = pLoggedTypesFromModule->loggedTypesFromModuleHash.Lookup(th); - if (!typeLoggingInfoPreexisting.th.IsNull()) + if (!fAllocateLoggedTypesFromModule) { - // Type is already hashed, so it's already logged, so we don't need to - // log it again. - *pfCreatedNew = FALSE; - return fSucceeded; + break; } - } - // We haven't logged this type, so we need to continue with this function to - // log it below. Add it to the hash table first so any recursive calls will - // see that this type is already being taken care of - fSucceeded = FALSE; - TypeLoggingInfo typeLoggingInfoNew(th); - { - CrstHolder _crst(GetHashCrst()); - // Like above, check if the type has been added from a different thread since we last looked it up. - if (!pLoggedTypesFromModule->loggedTypesFromModuleHash.Lookup(th).th.IsNull()) + pNewLoggedTypesFromModule = new (nothrow) LoggedTypesFromModule(pLoaderModule); + if (pNewLoggedTypesFromModule == NULL) { - *pfCreatedNew = FALSE; - return fSucceeded; + break; } + } - EX_TRY - { - pLoggedTypesFromModule->loggedTypesFromModuleHash.Add(typeLoggingInfoNew); - fSucceeded = TRUE; - } - EX_SWALLOW_NONTERMINAL - if (!fSucceeded) - { - *pfCreatedNew = FALSE; - return fSucceeded; - } - } // RELEASE: CrstHolder _crst(GetHashCrst()); + delete pNewLoggedTypesFromModule; - *pfCreatedNew = TRUE; + *pfCreatedNew = fSucceeded; return fSucceeded; } From 3d98a8b74587dfbd2c700e12d105ceab7bb4ff9e Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Sat, 26 Sep 2026 09:05:18 -0400 Subject: [PATCH 2/2] Document type cache allocation locking Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/eventtrace.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/eventtrace.cpp b/src/coreclr/vm/eventtrace.cpp index fb0a9e1f0bfc44..f4ad68c6e1c47f 100644 --- a/src/coreclr/vm/eventtrace.cpp +++ b/src/coreclr/vm/eventtrace.cpp @@ -1582,6 +1582,8 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p break; } + // Allocate outside the lock, then revalidate and publish the candidate + // under the lock on the next iteration. pNewLoggedTypesFromModule = new (nothrow) LoggedTypesFromModule(pLoaderModule); if (pNewLoggedTypesFromModule == NULL) {