Fix SIGSEGV in EventPipe on Shutdown (#14123)

* Fix a crash that occurs when a provider is registered after the configuration object has been destroyed.

* Code review feedback.

diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp
index eebd274..8f2e8ff 100644 (file)
--- a/src/vm/eventpipe.cpp
+++ b/src/vm/eventpipe.cpp
@@ -390,7 +390,14 @@ EventPipeProvider* EventPipe::CreateProvider(const SString &providerName, EventP
     }
     CONTRACTL_END;
 
-    return new EventPipeProvider(providerName, pCallbackFunction, pCallbackData);
+    EventPipeProvider *pProvider = NULL;
+    if (s_pConfig != NULL)
+    {
+        pProvider = s_pConfig->CreateProvider(providerName, pCallbackFunction, pCallbackData);
+    }
+
+    return pProvider;
+
 }
 
 void EventPipe::DeleteProvider(EventPipeProvider *pProvider)
@@ -418,8 +425,10 @@ void EventPipe::DeleteProvider(EventPipeProvider *pProvider)
         else
         {
             // Delete the provider now.
-            // NOTE: This will remove it from all of the EventPipe data structures.
-            delete(pProvider);
+            if (s_pConfig != NULL)
+            {
+                s_pConfig->DeleteProvider(pProvider);
+            }
         }
     }
 }

diff --git a/src/vm/eventpipeconfiguration.cpp b/src/vm/eventpipeconfiguration.cpp
index 8b70371..ae1dd4e 100644 (file)
--- a/src/vm/eventpipeconfiguration.cpp
+++ b/src/vm/eventpipeconfiguration.cpp
@@ -75,7 +75,7 @@ void EventPipeConfiguration::Initialize()
     CONTRACTL_END;
 
     // Create the configuration provider.
-    m_pConfigProvider = EventPipe::CreateProvider(SL(s_configurationProviderName));
+    m_pConfigProvider = CreateProvider(SL(s_configurationProviderName), NULL, NULL);
 
     // Create the metadata event.
     m_pMetadataEvent = m_pConfigProvider->AddEvent(
@@ -86,6 +86,49 @@ void EventPipeConfiguration::Initialize()
         false); /* needStack */
 }
 
+EventPipeProvider* EventPipeConfiguration::CreateProvider(const SString &providerName, EventPipeCallback pCallbackFunction, void *pCallbackData)
+{
+    CONTRACTL
+    {
+        THROWS;
+        GC_NOTRIGGER;
+        MODE_ANY;
+    }
+    CONTRACTL_END;
+
+    // Allocate a new provider.
+    EventPipeProvider *pProvider = new EventPipeProvider(this, providerName, pCallbackFunction, pCallbackData);
+
+    // Register the provider with the configuration system.
+    RegisterProvider(*pProvider);
+
+    return pProvider;
+}
+
+void EventPipeConfiguration::DeleteProvider(EventPipeProvider *pProvider)
+{
+    CONTRACTL
+    {
+        THROWS;
+        GC_NOTRIGGER;
+        MODE_ANY;
+        PRECONDITION(pProvider != NULL);
+    }
+    CONTRACTL_END;
+
+    if (pProvider == NULL)
+    {
+        return;
+    }
+
+    // Unregister the provider.
+    UnregisterProvider(*pProvider);
+
+    // Free the provider itself.
+    delete(pProvider);
+}
+
+
 bool EventPipeConfiguration::RegisterProvider(EventPipeProvider &provider)
 {
     CONTRACTL

diff --git a/src/vm/eventpipeconfiguration.h b/src/vm/eventpipeconfiguration.h
index 1d16136..baca069 100644 (file)
--- a/src/vm/eventpipeconfiguration.h
+++ b/src/vm/eventpipeconfiguration.h
@@ -35,6 +35,12 @@ public:
     // Perform initialization that cannot be performed in the constructor.
     void Initialize();
 
+    // Create a new provider.
+    EventPipeProvider* CreateProvider(const SString &providerName, EventPipeCallback pCallbackFunction, void *pCallbackData);
+
+    // Delete a provider.
+    void DeleteProvider(EventPipeProvider *pProvider);
+
     // Register a provider.
     bool RegisterProvider(EventPipeProvider &provider);
 

diff --git a/src/vm/eventpipeprovider.cpp b/src/vm/eventpipeprovider.cpp
index 7361541..c10dd33 100644 (file)
--- a/src/vm/eventpipeprovider.cpp
+++ b/src/vm/eventpipeprovider.cpp
@@ -11,13 +11,14 @@
 
 #ifdef FEATURE_PERFTRACING
 
-EventPipeProvider::EventPipeProvider(const SString &providerName, EventPipeCallback pCallbackFunction, void *pCallbackData)
+EventPipeProvider::EventPipeProvider(EventPipeConfiguration *pConfig, const SString &providerName, EventPipeCallback pCallbackFunction, void *pCallbackData)
 {
     CONTRACTL
     {
         THROWS;
         GC_NOTRIGGER;
         MODE_ANY;
+        PRECONDITION(pConfig != NULL);
     }
     CONTRACTL_END;
 
@@ -28,11 +29,7 @@ EventPipeProvider::EventPipeProvider(const SString &providerName, EventPipeCallb
     m_pEventList = new SList<SListElem<EventPipeEvent*>>();
     m_pCallbackFunction = pCallbackFunction;
     m_pCallbackData = pCallbackData;
-    m_pConfig = EventPipe::GetConfiguration();
-    _ASSERTE(m_pConfig != NULL);
-
-    // Register the provider.
-    m_pConfig->RegisterProvider(*this);
+    m_pConfig = pConfig;
 }
 
 EventPipeProvider::~EventPipeProvider()
@@ -45,15 +42,6 @@ EventPipeProvider::~EventPipeProvider()
     }
     CONTRACTL_END;
 
-    // Unregister the provider.
-    // This call is re-entrant.
-    // NOTE: We don't use the cached event pipe configuration pointer
-    // in case this runs during shutdown and the configuration has already
-    // been freed.
-    EventPipeConfiguration* pConfig = EventPipe::GetConfiguration();
-    _ASSERTE(pConfig != NULL);
-    pConfig->UnregisterProvider(*this);
-
     // Free all of the events.
     if(m_pEventList != NULL)
     {

diff --git a/src/vm/eventpipeprovider.h b/src/vm/eventpipeprovider.h
index 7b92fac..405ce32 100644 (file)
--- a/src/vm/eventpipeprovider.h
+++ b/src/vm/eventpipeprovider.h
@@ -64,7 +64,7 @@ private:
     bool m_deleteDeferred;
 
     // Private constructor because all providers are created through EventPipe::CreateProvider.
-    EventPipeProvider(const SString &providerName, EventPipeCallback pCallbackFunction = NULL, void *pCallbackData = NULL);
+    EventPipeProvider(EventPipeConfiguration *pConfig, const SString &providerName, EventPipeCallback pCallbackFunction = NULL, void *pCallbackData = NULL);
 
 public: