Protect multi-threaded access to Module.m_GenericParamToDescMap. (#1907)

Same type can be loaded by multiple threads in parallel in rare situations

Fixes #1847

src/coreclr/src/inc/CrstTypes.def

@@ -105,7 +105,7 @@ Crst AssemblyDependencyGraph
 End
 
 Crst AvailableParamTypes
-    AcquiredBefore IbcProfile LoaderHeap
+    AcquiredBefore ModuleLookupTable IbcProfile LoaderHeap
 End
 
 Crst BaseDomain

src/coreclr/src/inc/crsttypes.h

@@ -188,7 +188,7 @@ int g_rgCrstLevelMap[] =
     0,          // CrstAssemblyList
     7,          // CrstAssemblyLoader
     3,          // CrstAvailableClass
-    3,          // CrstAvailableParamTypes
+    4,          // CrstAvailableParamTypes
     7,          // CrstBaseDomain
     -1,         // CrstCCompRC
     9,          // CrstCer

src/coreclr/src/vm/clsload.hpp

@@ -529,8 +529,8 @@ class ClassLoader
     friend class AppDomain;
     friend class Assembly;
     friend class Module;
+    friend class InstantiatedMethodDesc;
     friend class CLRPrivTypeCacheWinRT;
-    friend class CLRPrivTypeCacheReflectionOnlyWinRT;
 
     // the following two classes are friends because they will call LoadTypeHandleForTypeKey by token directly
     friend class COMDynamicWrite;

src/coreclr/src/vm/genmeth.cpp

@@ -1433,26 +1433,33 @@ void InstantiatedMethodDesc::SetupGenericMethodDefinition(IMDInternalImport *pIM
     m_pPerInstInfo.SetValue((Dictionary *) pamTracker->Track(pAllocator->GetLowFrequencyHeap()->AllocMem(dwAllocSize)));
 
     TypeHandle * pInstDest = (TypeHandle *) IMD_GetMethodDictionaryNonNull();
-    for(unsigned int i = 0; i < numTyPars; i++)
+
     {
-        hEnumTyPars.EnumNext(&tkTyPar);
+        // Protect multi-threaded access to Module.m_GenericParamToDescMap. Other threads may be loading the same type
+        // to break type recursion dead-locks
+
+        // m_AvailableTypesLock has to be taken in cooperative mode to avoid deadlocks during GC
+        GCX_COOP();
+        CrstHolder ch(&pModule->GetClassLoader()->m_AvailableTypesLock);
 
-        // code:Module.m_GenericParamToDescMap maps generic parameter RIDs to TypeVarTypeDesc
-        // instances so that we do not leak by allocating them all over again, if the declaring
-        // type repeatedly fails to load.
-        TypeVarTypeDesc *pTypeVarTypeDesc = pModule->LookupGenericParam(tkTyPar);
-        if (pTypeVarTypeDesc == NULL)
+        for (unsigned int i = 0; i < numTyPars; i++)
         {
-            // Do NOT use pamTracker for this memory as we need it stay allocated even if the load fails.
-            void *mem = (void *)pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(TypeVarTypeDesc)));
-            pTypeVarTypeDesc = new (mem) TypeVarTypeDesc(pModule, tok, i, tkTyPar);
-
-            // No race here - the row in GenericParam table is owned exclusively by this method and we
-            // are holding a lock preventing other threads from loading the declaring type and setting
-            // up this method desc.
-            pModule->StoreGenericParamThrowing(tkTyPar, pTypeVarTypeDesc);
+            hEnumTyPars.EnumNext(&tkTyPar);
+
+            // code:Module.m_GenericParamToDescMap maps generic parameter RIDs to TypeVarTypeDesc
+            // instances so that we do not leak by allocating them all over again, if the declaring
+            // type repeatedly fails to load.
+            TypeVarTypeDesc* pTypeVarTypeDesc = pModule->LookupGenericParam(tkTyPar);
+            if (pTypeVarTypeDesc == NULL)
+            {
+                // Do NOT use pamTracker for this memory as we need it stay allocated even if the load fails.
+                void* mem = (void*)pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(TypeVarTypeDesc)));
+                pTypeVarTypeDesc = new (mem) TypeVarTypeDesc(pModule, tok, i, tkTyPar);
+
+                pModule->StoreGenericParamThrowing(tkTyPar, pTypeVarTypeDesc);
+            }
+            pInstDest[i] = TypeHandle(pTypeVarTypeDesc);
         }
-        pInstDest[i] = TypeHandle(pTypeVarTypeDesc);
     }
     LOG((LF_JIT, LL_INFO10000, "GENERICSVER: Initialized typical  method instantiation with %d type handles\n",numTyPars));
 }

src/coreclr/src/vm/methodtablebuilder.cpp

@@ -11915,45 +11915,52 @@ MethodTableBuilder::GatherGenericsInfo(
         }
 
         TypeHandle * pDestInst = (TypeHandle *)inst.GetRawArgs();
-        for (unsigned int i = 0; i < numGenericArgs; i++)
         {
-            pInternalImport->EnumNext(&hEnumGenericPars, &tkTyPar);
-            DWORD flags;
-            if (FAILED(pInternalImport->GetGenericParamProps(tkTyPar, NULL, &flags, NULL, NULL, NULL)))
-            {
-                pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, cl, IDS_CLASSLOAD_BADFORMAT);
-            }
+            // Protect multi-threaded access to Module.m_GenericParamToDescMap. Other threads may be loading the same type
+            // to break type recursion dead-locks
 
-            if (bmtGenericsInfo->fTypicalInstantiation)
+            // m_AvailableTypesLock has to be taken in cooperative mode to avoid deadlocks during GC
+            GCX_COOP();
+            CrstHolder ch(&pModule->GetClassLoader()->m_AvailableTypesLock);
+
+            for (unsigned int i = 0; i < numGenericArgs; i++)
             {
-                // code:Module.m_GenericParamToDescMap maps generic parameter RIDs to TypeVarTypeDesc
-                // instances so that we do not leak by allocating them all over again, if the type
-                // repeatedly fails to load.
-                TypeVarTypeDesc *pTypeVarTypeDesc = pModule->LookupGenericParam(tkTyPar);
-                if (pTypeVarTypeDesc == NULL)
+                pInternalImport->EnumNext(&hEnumGenericPars, &tkTyPar);
+                DWORD flags;
+                if (FAILED(pInternalImport->GetGenericParamProps(tkTyPar, NULL, &flags, NULL, NULL, NULL)))
                 {
-                    // Do NOT use the alloc tracker for this memory as we need it stay allocated even if the load fails.
-                    void *mem = (void *)pModule->GetLoaderAllocator()->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(TypeVarTypeDesc)));
-                    pTypeVarTypeDesc = new (mem) TypeVarTypeDesc(pModule, cl, i, tkTyPar);
-
-                    // No race here - the row in GenericParam table is owned exclusively by this type and we
-                    // are holding a lock preventing other threads from concurrently loading it.
-                    pModule->StoreGenericParamThrowing(tkTyPar, pTypeVarTypeDesc);
+                    pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, cl, IDS_CLASSLOAD_BADFORMAT);
                 }
-                pDestInst[i] = TypeHandle(pTypeVarTypeDesc);
-            }
 
-            DWORD varianceAnnotation = flags & gpVarianceMask;
-            bmtGenericsInfo->pVarianceInfo[i] = static_cast<BYTE>(varianceAnnotation);
-            if (varianceAnnotation != gpNonVariant)
-            {
-                if (varianceAnnotation != gpContravariant && varianceAnnotation != gpCovariant)
+                if (bmtGenericsInfo->fTypicalInstantiation)
                 {
-                    pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, cl, IDS_CLASSLOAD_BADVARIANCE);
+                    // code:Module.m_GenericParamToDescMap maps generic parameter RIDs to TypeVarTypeDesc
+                    // instances so that we do not leak by allocating them all over again, if the type
+                    // repeatedly fails to load.
+                    TypeVarTypeDesc* pTypeVarTypeDesc = pModule->LookupGenericParam(tkTyPar);
+                    if (pTypeVarTypeDesc == NULL)
+                    {
+                        // Do NOT use the alloc tracker for this memory as we need it stay allocated even if the load fails.
+                        void* mem = (void*)pModule->GetLoaderAllocator()->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(TypeVarTypeDesc)));
+                        pTypeVarTypeDesc = new (mem) TypeVarTypeDesc(pModule, cl, i, tkTyPar);
+
+                        pModule->StoreGenericParamThrowing(tkTyPar, pTypeVarTypeDesc);
+                    }
+                    pDestInst[i] = TypeHandle(pTypeVarTypeDesc);
                 }
-                else
+
+                DWORD varianceAnnotation = flags & gpVarianceMask;
+                bmtGenericsInfo->pVarianceInfo[i] = static_cast<BYTE>(varianceAnnotation);
+                if (varianceAnnotation != gpNonVariant)
                 {
-                    fHasVariance = TRUE;
+                    if (varianceAnnotation != gpContravariant && varianceAnnotation != gpCovariant)
+                    {
+                        pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, cl, IDS_CLASSLOAD_BADVARIANCE);
+                    }
+                    else
+                    {
+                        fHasVariance = TRUE;
+                    }
                 }
             }
         }