From 3506992ef19b8d61d6fbe922732a8137aa276fa6 Mon Sep 17 00:00:00 2001 From: Brian Robbins Date: Thu, 21 Sep 2017 15:41:40 -0700 Subject: [PATCH 1/2] Fix a crash that occurs when a provider is registered after the configuration object has been destroyed. --- src/vm/eventpipe.cpp | 15 ++++++++--- src/vm/eventpipeconfiguration.cpp | 45 ++++++++++++++++++++++++++++++- src/vm/eventpipeconfiguration.h | 6 +++++ 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 eebd2744a4a6..957fef710511 100644 --- 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 8b703718b27c..5c8e14e3ee65 100644 --- 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 1d161367b25f..baca06920a25 100644 --- a/src/vm/eventpipeconfiguration.h +++ b/src/vm/eventpipeconfiguration.h @@ -35,6 +35,12 @@ class EventPipeConfiguration // 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 7361541e7748..c10dd33638ff 100644 --- 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>(); 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 7b92faca729c..405ce3215492 100644 --- a/src/vm/eventpipeprovider.h +++ b/src/vm/eventpipeprovider.h @@ -64,7 +64,7 @@ class EventPipeProvider 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: From 1eb370cef70da5ddd6245acfd98b0b2019f7e5b1 Mon Sep 17 00:00:00 2001 From: Brian Robbins Date: Thu, 21 Sep 2017 16:54:57 -0700 Subject: [PATCH 2/2] Code review feedback. --- src/vm/eventpipe.cpp | 4 ++-- src/vm/eventpipeconfiguration.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index 957fef710511..8f2e8ff93775 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -391,7 +391,7 @@ EventPipeProvider* EventPipe::CreateProvider(const SString &providerName, EventP CONTRACTL_END; EventPipeProvider *pProvider = NULL; - if(s_pConfig != NULL) + if (s_pConfig != NULL) { pProvider = s_pConfig->CreateProvider(providerName, pCallbackFunction, pCallbackData); } @@ -425,7 +425,7 @@ void EventPipe::DeleteProvider(EventPipeProvider *pProvider) else { // Delete the provider now. - if(s_pConfig != NULL) + if (s_pConfig != NULL) { s_pConfig->DeleteProvider(pProvider); } diff --git a/src/vm/eventpipeconfiguration.cpp b/src/vm/eventpipeconfiguration.cpp index 5c8e14e3ee65..ae1dd4e09995 100644 --- a/src/vm/eventpipeconfiguration.cpp +++ b/src/vm/eventpipeconfiguration.cpp @@ -116,7 +116,7 @@ void EventPipeConfiguration::DeleteProvider(EventPipeProvider *pProvider) } CONTRACTL_END; - if(pProvider == NULL) + if (pProvider == NULL) { return; }