From 6ba93eb016258a39442669fc35f645687be679a2 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Thu, 8 Aug 2024 15:01:44 -0400 Subject: [PATCH 1/3] [EventPipe] Block ep_delete_provider for pending callbacks --- src/native/eventpipe/ep.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/native/eventpipe/ep.c b/src/native/eventpipe/ep.c index ae0af76d64680d..e97d59988acff0 100644 --- a/src/native/eventpipe/ep.c +++ b/src/native/eventpipe/ep.c @@ -1319,22 +1319,15 @@ ep_delete_provider (EventPipeProvider *provider) // where we hold a provider after tracing has been disabled. bool wait_for_provider_callbacks_completion = false; EP_LOCK_ENTER (section1) - if (enabled ()) { - // Save the provider until the end of the tracing session. - ep_provider_set_delete_deferred (provider, true); - - // The callback func must be previously set to null, - // otherwise callbacks might never stop coming. - EP_ASSERT (provider->callback_func == NULL); - - // Calling ep_delete_provider within a Callback will result in a deadlock - // as deleting the provider with an active tracing session will block - // until all of the provider's callbacks are completed. - if (provider->callbacks_pending > 0) - wait_for_provider_callbacks_completion = true; - } else { - config_delete_provider (ep_config_get (), provider); - } + // The callback func must be set to null, + // otherwise callbacks might never stop coming. + provider->callback_func = NULL; + + // Calling ep_delete_provider within a Callback will result in a deadlock + // as deleting the provider with an active tracing session will block + // until all of the provider's callbacks are completed. + if (provider->callbacks_pending > 0) + wait_for_provider_callbacks_completion = true; EP_LOCK_EXIT (section1) // Block provider deletion until all pending callbacks are completed. @@ -1344,6 +1337,15 @@ ep_delete_provider (EventPipeProvider *provider) if (wait_for_provider_callbacks_completion) ep_rt_wait_event_wait (&provider->callbacks_complete_event, EP_INFINITE_WAIT, false); + EP_LOCK_ENTER (section2) + if (enabled ()) { + // Save the provider until the end of the tracing session. + ep_provider_set_delete_deferred (provider, true); + } else { + config_delete_provider (ep_config_get (), provider); + } + EP_LOCK_EXIT (section2) + ep_on_exit: ep_requires_lock_not_held (); return; From 0ff62864cffee7e8b44cb5dec15d1f6126218818 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Thu, 8 Aug 2024 15:02:11 -0400 Subject: [PATCH 2/3] Free callbacks complete event --- src/native/eventpipe/ep-provider.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index 0c4f4441bba567..cf7d9eb51d3a2a 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -493,6 +493,7 @@ provider_free (EventPipeProvider * provider) dn_list_custom_free (provider->event_list, event_free_func); + ep_rt_wait_event_free (&provider->callbacks_complete_event); ep_rt_utf16_string_free (provider->provider_name_utf16); ep_rt_utf8_string_free (provider->provider_name); ep_rt_object_free (provider); From 4c575fea37312913af9fdf5eb2089a447c935080 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Fri, 9 Aug 2024 13:20:32 -0400 Subject: [PATCH 3/3] Address feedback --- src/native/eventpipe/ep.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/native/eventpipe/ep.c b/src/native/eventpipe/ep.c index e97d59988acff0..d57a01d9c0978d 100644 --- a/src/native/eventpipe/ep.c +++ b/src/native/eventpipe/ep.c @@ -644,7 +644,9 @@ disable_holding_lock ( ep_session_free (session); // Providers can't be deleted during tracing because they may be needed when serializing the file. - config_delete_deferred_providers(ep_config_get ()); + // Allow delete deferred providers to accumulate to mitigate potential use-after-free should + // another EventPipe session hold a reference to a provider set for deferred deletion. + // config_delete_deferred_providers(ep_config_get ()); } ep_requires_lock_held (); @@ -1319,9 +1321,12 @@ ep_delete_provider (EventPipeProvider *provider) // where we hold a provider after tracing has been disabled. bool wait_for_provider_callbacks_completion = false; EP_LOCK_ENTER (section1) + // Save the provider until the end of the tracing session. + ep_provider_set_delete_deferred (provider, true); + // The callback func must be set to null, // otherwise callbacks might never stop coming. - provider->callback_func = NULL; + EP_ASSERT (provider->callback_func == NULL); // Calling ep_delete_provider within a Callback will result in a deadlock // as deleting the provider with an active tracing session will block @@ -1338,12 +1343,8 @@ ep_delete_provider (EventPipeProvider *provider) ep_rt_wait_event_wait (&provider->callbacks_complete_event, EP_INFINITE_WAIT, false); EP_LOCK_ENTER (section2) - if (enabled ()) { - // Save the provider until the end of the tracing session. - ep_provider_set_delete_deferred (provider, true); - } else { + if (!enabled ()) config_delete_provider (ep_config_get (), provider); - } EP_LOCK_EXIT (section2) ep_on_exit: