From d9ad93dc3141a648d11f7d54bee59c318c97dd4e Mon Sep 17 00:00:00 2001 From: Brian Robbins Date: Fri, 22 Sep 2017 01:17:48 -0700 Subject: [PATCH] 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. --- src/vm/eventpipe.cpp | 14 +++++++++--- src/vm/eventpipeconfiguration.cpp | 45 ++++++++++++++++++++++++++++++++++++++- src/vm/eventpipeconfiguration.h | 7 ++++++ src/vm/eventpipeprovider.cpp | 18 +++------------- src/vm/eventpipeprovider.h | 2 +- 5 files changed, 66 insertions(+), 20 deletions(-) diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index e041615..50909a1 100644 --- a/src/vm/eventpipe.cpp +++ b/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); + } } } } diff --git a/src/vm/eventpipeconfiguration.cpp b/src/vm/eventpipeconfiguration.cpp index 42f9daf..69e65e6 100644 --- a/src/vm/eventpipeconfiguration.cpp +++ b/src/vm/eventpipeconfiguration.cpp @@ -59,7 +59,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 +70,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 diff --git a/src/vm/eventpipeconfiguration.h b/src/vm/eventpipeconfiguration.h index de8e79d..96be50e 100644 --- a/src/vm/eventpipeconfiguration.h +++ b/src/vm/eventpipeconfiguration.h @@ -6,6 +6,7 @@ #ifdef FEATURE_PERFTRACING +#include "eventpipe.h" #include "slist.h" class EventPipeEnabledProvider; @@ -35,6 +36,12 @@ public: // 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); diff --git a/src/vm/eventpipeprovider.cpp b/src/vm/eventpipeprovider.cpp index 896f9b26..4cc02c1 100644 --- a/src/vm/eventpipeprovider.cpp +++ b/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>(); 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) { diff --git a/src/vm/eventpipeprovider.h b/src/vm/eventpipeprovider.h index d2c459e..b0e9cc9 100644 --- a/src/vm/eventpipeprovider.h +++ b/src/vm/eventpipeprovider.h @@ -61,7 +61,7 @@ private: 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: -- 2.7.4