Fix SIGSEGV in EventPipe on Shutdown (#16152)

* 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.

* Use DeleteProvider to avoid phantom registers providers

Author: Victor "Nate" Graf

src/vm/eventpipe.cpp

@@ -248,7 +248,13 @@ EventPipeProvider* EventPipe::CreateProvider(const GUID &providerID, EventPipeCa
     }
     CONTRACTL_END;
 
-    return new EventPipeProvider(providerID, pCallbackFunction, pCallbackData);
+    EventPipeProvider *pProvider = NULL;
+    if (s_pConfig != NULL)
+    {
+        pProvider = s_pConfig->CreateProvider(providerID, pCallbackFunction, pCallbackData);
+    }
+
+    return pProvider;
 }
 
 void EventPipe::DeleteProvider(EventPipeProvider *pProvider)
@@ -276,8 +282,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);
+            }
         }
     }
 }

src/vm/eventpipeconfiguration.cpp

@@ -34,6 +34,11 @@ EventPipeConfiguration::~EventPipeConfiguration()
         MODE_ANY;
     }
     CONTRACTL_END;
+    
+    if(m_pConfigProvider != NULL)
+    {
+        DeleteProvider(m_pConfigProvider);
+    }
 
     if(m_pEnabledProviderList != NULL)
     {
@@ -59,7 +64,7 @@ void EventPipeConfiguration::Initialize()
     CONTRACTL_END;
 
     // Create the configuration provider.
-    m_pConfigProvider = EventPipe::CreateProvider(s_configurationProviderID);
+    m_pConfigProvider = CreateProvider(s_configurationProviderID, NULL, NULL);
 
     // Create the metadata event.
     m_pMetadataEvent = m_pConfigProvider->AddEvent(
@@ -70,6 +75,49 @@ void EventPipeConfiguration::Initialize()
         false); /* needStack */
 }
 
+EventPipeProvider* EventPipeConfiguration::CreateProvider(const GUID &providerID, EventPipeCallback pCallbackFunction, void *pCallbackData)
+{
+    CONTRACTL
+    {
+        THROWS;
+        GC_NOTRIGGER;
+        MODE_ANY;
+    }
+    CONTRACTL_END;
+
+    // Allocate a new provider.
+    EventPipeProvider *pProvider = new EventPipeProvider(this, providerID, 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
@@ -400,8 +448,7 @@ void EventPipeConfiguration::DeleteDeferredProviders()
         EventPipeProvider *pProvider = pElem->GetValue();
         if(pProvider->GetDeleteDeferred())
         {
-            // The act of deleting the provider unregisters it and removes it from the list.
-            delete(pProvider);
+            DeleteProvider(pProvider);
         }
 
         pElem = m_pProviderList->GetNext(pElem);

src/vm/eventpipeconfiguration.h

@@ -6,6 +6,7 @@
 
 #ifdef FEATURE_PERFTRACING
 
+#include "eventpipe.h"
 #include "slist.h"
 
 class EventPipeEnabledProvider;
@@ -35,6 +36,12 @@ class EventPipeConfiguration
     // Perform initialization that cannot be performed in the constructor.
     void Initialize();
 
+    // Create a new provider.
+    EventPipeProvider* CreateProvider(const GUID &providerID, EventPipeCallback pCallbackFunction, void *pCallbackData);
+
+    // Delete a provider.
+    void DeleteProvider(EventPipeProvider *pProvider);
+
     // Register a provider.
     bool RegisterProvider(EventPipeProvider &provider);
 

src/vm/eventpipeprovider.cpp

@@ -10,13 +10,14 @@
 
 #ifdef FEATURE_PERFTRACING
 
-EventPipeProvider::EventPipeProvider(const GUID &providerID, EventPipeCallback pCallbackFunction, void *pCallbackData)
+EventPipeProvider::EventPipeProvider(EventPipeConfiguration *pConfig, const GUID &providerID, EventPipeCallback pCallbackFunction, void *pCallbackData)
 {
     CONTRACTL
     {
         THROWS;
         GC_NOTRIGGER;
         MODE_ANY;
+        PRECONDITION(pConfig != NULL);
     }
     CONTRACTL_END;
 
@@ -27,11 +28,7 @@ EventPipeProvider::EventPipeProvider(const GUID &providerID, EventPipeCallback p
     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()
@@ -44,15 +41,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)
     {

src/vm/eventpipeprovider.h

@@ -61,7 +61,7 @@ class EventPipeProvider
     bool m_deleteDeferred;
 
     // Private constructor because all providers are created through EventPipe::CreateProvider.
-    EventPipeProvider(const GUID &providerID, EventPipeCallback pCallbackFunction = NULL, void *pCallbackData = NULL);
+    EventPipeProvider(EventPipeConfiguration *pConfig, const GUID &providerID, EventPipeCallback pCallbackFunction = NULL, void *pCallbackData = NULL);
 
 public: