From 565788b97aca80e2d9296da96f91b5097c1ffbb2 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Sat, 23 May 2020 11:37:21 -0700 Subject: [PATCH 1/4] fix race condition in AddTypeToGlobalCacheIfNotExists during rundown --- src/coreclr/src/inc/CrstTypes.def | 7 +- src/coreclr/src/inc/crsttypes.h | 8 +- src/coreclr/src/vm/eventtrace.cpp | 118 ++++++++++++++++++------------ 3 files changed, 77 insertions(+), 56 deletions(-) diff --git a/src/coreclr/src/inc/CrstTypes.def b/src/coreclr/src/inc/CrstTypes.def index 06f900a79be900..6aeee77b85910f 100644 --- a/src/coreclr/src/inc/CrstTypes.def +++ b/src/coreclr/src/inc/CrstTypes.def @@ -509,12 +509,7 @@ End // used to remember which types have been logged (to avoid duplicate logging of the // same type). Crst EtwTypeLogHash - AcquiredAfter ThreadStore AllowedFiles Cer TPMethodTable - AcquiredBefore AvailableParamTypes ConnectionNameTable DeadlockDetection DebuggerController - DebuggerHeapLock DebuggerJitInfo DynamicIL ExecuteManRangeLock HandleTable IbcProfile - JitGenericHandleCache JumpStubCache LoaderHeap ModuleLookupTable ProfilingAPIStatus - ProfilerGCRefDataFreeList RWLock SingleUseLock SyncBlockCache SystemDomainDelayedUnloadList - ThreadIdDispenser ThreadStaticDataHashTable + AcquiredAfter AllowedFiles Cer SingleUseLock TPMethodTable End Crst Remoting diff --git a/src/coreclr/src/inc/crsttypes.h b/src/coreclr/src/inc/crsttypes.h index 98d24c2efafcae..82777eeb4a9145 100644 --- a/src/coreclr/src/inc/crsttypes.h +++ b/src/coreclr/src/inc/crsttypes.h @@ -182,7 +182,7 @@ enum CrstType // An array mapping CrstType to level. int g_rgCrstLevelMap[] = { - 9, // CrstAllowedFiles + 7, // CrstAllowedFiles 9, // CrstAppDomainCache 14, // CrstAppDomainHandleTable 0, // CrstArgBasedStubCache @@ -194,7 +194,7 @@ int g_rgCrstLevelMap[] = 4, // CrstAvailableParamTypes 7, // CrstBaseDomain -1, // CrstCCompRC - 9, // CrstCer + 7, // CrstCer 13, // CrstClassFactInfoHash 8, // CrstClassInit -1, // CrstClrNotification @@ -223,7 +223,7 @@ int g_rgCrstLevelMap[] = 0, // CrstDynamicIL 3, // CrstDynamicMT 3, // CrstDynLinkZapItems - 7, // CrstEtwTypeLogHash + 0, // CrstEtwTypeLogHash 18, // CrstEventPipe 0, // CrstEventStore 0, // CrstException @@ -326,7 +326,7 @@ int g_rgCrstLevelMap[] = 4, // CrstThreadStaticDataHashTable 12, // CrstThreadStore 9, // CrstTieredCompilation - 9, // CrstTPMethodTable + 7, // CrstTPMethodTable 3, // CrstTypeEquivalenceMap 7, // CrstTypeIDMap 3, // CrstUMEntryThunkCache diff --git a/src/coreclr/src/vm/eventtrace.cpp b/src/coreclr/src/vm/eventtrace.cpp index d5099e498c2a67..3764b07e84cd5f 100644 --- a/src/coreclr/src/vm/eventtrace.cpp +++ b/src/coreclr/src/vm/eventtrace.cpp @@ -3467,67 +3467,84 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p BOOL fSucceeded = FALSE; + // 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)) { - CrstHolder _crst(GetHashCrst()); + *pfCreatedNew = FALSE; + return fSucceeded; + } - // 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)) + // TODO: Do under crst? + if (s_pAllLoggedTypes == NULL) + { + s_pAllLoggedTypes = new (nothrow) AllLoggedTypes; + if (s_pAllLoggedTypes == NULL) { + // out of memory. Bail on ETW stuff *pfCreatedNew = FALSE; return fSucceeded; } + } - if (s_pAllLoggedTypes == NULL) - { - s_pAllLoggedTypes = new (nothrow) AllLoggedTypes; - if (s_pAllLoggedTypes == NULL) - { - // out of memory. Bail on ETW stuff - *pfCreatedNew = FALSE; - return fSucceeded; - } - } - - // Step 1: go from LoaderModule to hash of types. + // 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); + } - Module * pLoaderModule = th.GetLoaderModule(); - _ASSERTE(pLoaderModule != NULL); - LoggedTypesFromModule * pLoggedTypesFromModule = s_pAllLoggedTypes->allLoggedTypesHash.Lookup(pLoaderModule); + if (pLoggedTypesFromModule == NULL) + { + pLoggedTypesFromModule = new (nothrow) LoggedTypesFromModule(pLoaderModule); if (pLoggedTypesFromModule == NULL) { - pLoggedTypesFromModule = new (nothrow) LoggedTypesFromModule(pLoaderModule); - if (pLoggedTypesFromModule == NULL) - { - // out of memory. Bail on ETW stuff - *pfCreatedNew = FALSE; - return fSucceeded; - } - - fSucceeded = FALSE; - EX_TRY + // out of memory. Bail on ETW stuff + *pfCreatedNew = FALSE; + return fSucceeded; + } + { + 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) { - s_pAllLoggedTypes->allLoggedTypesHash.Add(pLoggedTypesFromModule); - fSucceeded = TRUE; + EX_TRY + { + s_pAllLoggedTypes->allLoggedTypesHash.Add(pLoggedTypesFromModule); + fSucceeded = TRUE; + } + EX_CATCH + { + fSucceeded = FALSE; + } + EX_END_CATCH(RethrowTerminalExceptions); } - EX_CATCH + else { - fSucceeded = FALSE; + free(pLoggedTypesFromModule); + pLoggedTypesFromModule = recheckLoggedTypesFromModule; } - EX_END_CATCH(RethrowTerminalExceptions); + if (!fSucceeded) { *pfCreatedNew = FALSE; return fSucceeded; } } + } - // Step 2: From hash of types, see if our TypeHandle is there already - TypeLoggingInfo typeLoggingInfoPreexisting = pLoggedTypesFromModule->loggedTypesFromModuleHash.Lookup(th); + // 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()) { // Type is already hashed, so it's already logged, so we don't need to @@ -3535,12 +3552,22 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p *pfCreatedNew = FALSE; return fSucceeded; } + } + + // 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()) + { + *pfCreatedNew = FALSE; + return fSucceeded; + } - // 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); EX_TRY { pLoggedTypesFromModule->loggedTypesFromModuleHash.Add(typeLoggingInfoNew); @@ -3560,7 +3587,6 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p *pfCreatedNew = TRUE; return fSucceeded; - } //--------------------------------------------------------------------------------------- From 607806fb3caab7e227b1f64d4588e0e6e3bbcc21 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Sat, 23 May 2020 11:42:11 -0700 Subject: [PATCH 2/4] check for s_pAllLoggedTypes==NULL under lock --- src/coreclr/src/vm/eventtrace.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/vm/eventtrace.cpp b/src/coreclr/src/vm/eventtrace.cpp index 3764b07e84cd5f..eb6d847bade1c2 100644 --- a/src/coreclr/src/vm/eventtrace.cpp +++ b/src/coreclr/src/vm/eventtrace.cpp @@ -3479,15 +3479,17 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p return fSucceeded; } - // TODO: Do under crst? - if (s_pAllLoggedTypes == NULL) { - s_pAllLoggedTypes = new (nothrow) AllLoggedTypes; + CrstHolder _crst(GetHashCrst()); if (s_pAllLoggedTypes == NULL) { - // out of memory. Bail on ETW stuff - *pfCreatedNew = FALSE; - return fSucceeded; + s_pAllLoggedTypes = new (nothrow) AllLoggedTypes; + if (s_pAllLoggedTypes == NULL) + { + // out of memory. Bail on ETW stuff + *pfCreatedNew = FALSE; + return fSucceeded; + } } } From f1601642297cc439902357756fd0d95ceba7e8ab Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Sat, 23 May 2020 11:54:04 -0700 Subject: [PATCH 3/4] delete --- src/coreclr/src/vm/eventtrace.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/eventtrace.cpp b/src/coreclr/src/vm/eventtrace.cpp index eb6d847bade1c2..6af0fbafb840d5 100644 --- a/src/coreclr/src/vm/eventtrace.cpp +++ b/src/coreclr/src/vm/eventtrace.cpp @@ -3530,7 +3530,7 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p } else { - free(pLoggedTypesFromModule); + delete pLoggedTypesFromModule; pLoggedTypesFromModule = recheckLoggedTypesFromModule; } From ee3de7c1f09d65b70293e540504c9fb8c133c4cb Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 3 Jun 2020 00:15:29 -0700 Subject: [PATCH 4/4] code review feedback --- src/coreclr/src/vm/eventtrace.cpp | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/coreclr/src/vm/eventtrace.cpp b/src/coreclr/src/vm/eventtrace.cpp index 9a37579bc0d1d0..dbae224d1ee58f 100644 --- a/src/coreclr/src/vm/eventtrace.cpp +++ b/src/coreclr/src/vm/eventtrace.cpp @@ -3462,20 +3462,21 @@ BOOL ETW::TypeSystemLog::AddTypeToGlobalCacheIfNotExists(TypeHandle th, BOOL * p BOOL fSucceeded = FALSE; - // 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; - } - - { + { 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; + } + if (s_pAllLoggedTypes == NULL) { s_pAllLoggedTypes = new (nothrow) AllLoggedTypes;