From bd3814b7e5c50983b0738948e65ae6aced2b1042 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 9 Dec 2019 23:42:13 +0100 Subject: [PATCH] Fix DynamicMethodDesc::Destroy vs code heap enumeration race There is a race between DynamicMethodDesc::Destroy called from the finalizer thread and the MethodDescs enumeration called from ETW::MethodLog::SendEventsForJitMethods at process exit. DynamicMethodDesc::Destroy cleanos up its members m_pSig and m_pszMethodName and then it calls GetLCGMethodResolver()->Destroy(); That calls EEJitManager::FreeCodeMemory, which tries to take the m_CodeHeapCritSec lock. But this lock is already held by the ETW::MethodLog::SendEventsForJitMethods. So the iterator can see half-destroyed DynamicMethodDesc and a crash happens when trying to get the dynamic method name from the m_pszMethodName for the ETW event purposes. The fix is to call the GetLCGMethodResolver()->Destroy() before destroying the m_pSig and m_pszMethodName. --- src/coreclr/src/vm/dynamicmethod.cpp | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/coreclr/src/vm/dynamicmethod.cpp b/src/coreclr/src/vm/dynamicmethod.cpp index a9d350c46ba78..b25dfb73e2774 100644 --- a/src/coreclr/src/vm/dynamicmethod.cpp +++ b/src/coreclr/src/vm/dynamicmethod.cpp @@ -849,22 +849,27 @@ void DynamicMethodDesc::Destroy() _ASSERTE(IsDynamicMethod()); LoaderAllocator *pLoaderAllocator = GetLoaderAllocator(); - LOG((LF_BCL, LL_INFO1000, "Level3 - Destroying DynamicMethod {0x%p}\n", this)); - if (!m_pSig.IsNull()) + + // The m_pSig and m_pszMethodName need to be destroyed after the GetLCGMethodResolver()->Destroy() call + // otherwise the EEJitManager::CodeHeapIterator could return DynamicMethodDesc with these members NULLed, but + // the nibble map for the corresponding code memory indicating that this DynamicMethodDesc is still alive. + PCODE pSig = m_pSig.GetValue(); + PTR_CUTF8 pszMethodName = m_pszMethodName.GetValue(); + + GetLCGMethodResolver()->Destroy(); + // The current DynamicMethodDesc storage is destroyed at this point + + if (pszMethodName != NULL) { - delete[] (BYTE*)m_pSig.GetValue(); - m_pSig.SetValueMaybeNull(NULL); + delete[] pszMethodName; } - m_cSig = 0; - if (!m_pszMethodName.IsNull()) + + if (pSig != NULL) { - delete[] m_pszMethodName.GetValue(); - m_pszMethodName.SetValueMaybeNull(NULL); + delete[] (BYTE*)pSig; } - GetLCGMethodResolver()->Destroy(); - if (pLoaderAllocator->IsCollectible()) { if (pLoaderAllocator->Release())