[release/6.0] Fix memory leak in enqueue/dequeue of EventPipe callback data. (#58244)

* Fix memory leak in enqueue/dequeue of EventPipe callback data.

https://github.com/dotnet/runtime/pull/56104 made sure provider
callback data gets its own copy of filter data. This created a couple
of memory leaks when queue/dequeue the callback data since callback data
was not correctly freed in these scenarios leading to leaks of the copied
filter data.

Was detected running the manual EventPipe native unit tests on Windows
using its build in use of _CrtMemCheckpoint (only available in debug
builds) automatically detecting memory leaks.

Fix makes sure callback data is moved into queue on enqueue and moved
out in dequeue and that caller of dequeue make sure to
free returned callback data using ep_provider_callback_data_fini
when done using it. Doing a move instead of copy will also reduce
the number of allocations when enqueue/dequeue callback data in
provider callback queue.

* Fix build error.

Co-authored-by: lateralusX <lateralusx.github@gmail.com>

diff --git a/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c b/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c
index b60af23..32acf62 100644 (file)
--- a/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c
+++ b/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c
@@ -46,23 +46,26 @@ test_provider_callback_data_queue (void)
        EventPipeProviderCallbackDataQueue callback_data_queue;
        EventPipeProviderCallbackDataQueue *provider_callback_data_queue = ep_provider_callback_data_queue_init (&callback_data_queue);
 
-       EventPipeProviderCallbackData enqueue_callback_data;
-       EventPipeProviderCallbackData *provider_enqueue_callback_data = ep_provider_callback_data_init (
-               &enqueue_callback_data,
-               "",
-               provider_callback,
-               NULL,
-               1,
-               EP_EVENT_LEVEL_LOGALWAYS,
-               true);
-
-       for (uint32_t i = 0; i < 1000; ++i)
+       for (uint32_t i = 0; i < 1000; ++i) {
+               EventPipeProviderCallbackData enqueue_callback_data;
+               EventPipeProviderCallbackData *provider_enqueue_callback_data = ep_provider_callback_data_init (
+                       &enqueue_callback_data,
+                       "",
+                       provider_callback,
+                       NULL,
+                       1,
+                       EP_EVENT_LEVEL_LOGALWAYS,
+                       true);
                ep_provider_callback_data_queue_enqueue (provider_callback_data_queue, provider_enqueue_callback_data);
+               ep_provider_callback_data_fini (provider_enqueue_callback_data);
+       }
 
        EventPipeProviderCallbackData dequeue_callback_data;
        uint32_t deque_counter = 0;
-       while (ep_provider_callback_data_queue_try_dequeue (provider_callback_data_queue, &dequeue_callback_data))
+       while (ep_provider_callback_data_queue_try_dequeue(provider_callback_data_queue, &dequeue_callback_data)) {
                deque_counter++;
+               ep_provider_callback_data_fini (&dequeue_callback_data);
+       }
 
        if (deque_counter != 1000) {
                result = FAILED ("Unexpected number of provider callback invokes");

diff --git a/src/mono/mono/eventpipe/test/ep-tests.c b/src/mono/mono/eventpipe/test/ep-tests.c
index 5ff65d1..e9e30c3 100644 (file)
--- a/src/mono/mono/eventpipe/test/ep-tests.c
+++ b/src/mono/mono/eventpipe/test/ep-tests.c
@@ -620,7 +620,7 @@ test_create_delete_provider_with_callback (void)
        EventPipeProvider *test_provider = NULL;
        EventPipeProviderConfiguration provider_config;
 
-       EventPipeProviderConfiguration *current_provider_config =ep_provider_config_init (&provider_config, TEST_PROVIDER_NAME, 1, EP_EVENT_LEVEL_LOGALWAYS, "");
+       EventPipeProviderConfiguration *current_provider_config = ep_provider_config_init (&provider_config, TEST_PROVIDER_NAME, 1, EP_EVENT_LEVEL_LOGALWAYS, "");
        ep_raise_error_if_nok (current_provider_config != NULL);
 
        test_location = 1;

diff --git a/src/native/eventpipe/ep-config.c b/src/native/eventpipe/ep-config.c
index 8b7b9a8..489127b 100644 (file)
--- a/src/native/eventpipe/ep-config.c
+++ b/src/native/eventpipe/ep-config.c
@@ -113,6 +113,7 @@ config_register_provider (
                        EventPipeSessionProvider *session_provider = ep_rt_session_provider_list_find_by_name (ep_session_provider_list_get_providers_cref (providers), ep_provider_get_provider_name (provider));
                        if (session_provider) {
                                EventPipeProviderCallbackData provider_callback_data;
+                               memset (&provider_callback_data, 0, sizeof (provider_callback_data));
                                provider_set_config (
                                        provider,
                                        keyword_for_all_sessions,
@@ -124,6 +125,7 @@ config_register_provider (
                                        &provider_callback_data);
                                if (provider_callback_data_queue)
                                        ep_provider_callback_data_queue_enqueue (provider_callback_data_queue, &provider_callback_data);
+                               ep_provider_callback_data_fini (&provider_callback_data);
                        }
                }
        }
@@ -179,6 +181,7 @@ ep_config_init (EventPipeConfiguration *config)
        while (ep_provider_callback_data_queue_try_dequeue (provider_callback_data_queue, &provider_callback_data)) {
                ep_rt_prepare_provider_invoke_callback (&provider_callback_data);
                provider_invoke_callback (&provider_callback_data);
+               ep_provider_callback_data_fini (&provider_callback_data);
        }
 
        // Create the metadata event.
@@ -552,6 +555,7 @@ config_enable_disable (
                                        int64_t keyword_for_all_sessions;
                                        EventPipeEventLevel level_for_all_sessions;
                                        EventPipeProviderCallbackData provider_callback_data;
+                                       memset (&provider_callback_data, 0, sizeof (provider_callback_data));
                                        config_compute_keyword_and_level (config, provider, &keyword_for_all_sessions, &level_for_all_sessions);
                                        if (enable) {
                                                provider_set_config (
@@ -576,6 +580,7 @@ config_enable_disable (
                                        }
                                        if (provider_callback_data_queue)
                                                ep_provider_callback_data_queue_enqueue (provider_callback_data_queue, &provider_callback_data);
+                                       ep_provider_callback_data_fini (&provider_callback_data);
                                }
                        }
                }

diff --git a/src/native/eventpipe/ep-types.h b/src/native/eventpipe/ep-types.h
index 8bec5f1..ffe7188 100644 (file)
--- a/src/native/eventpipe/ep-types.h
+++ b/src/native/eventpipe/ep-types.h
@@ -97,6 +97,9 @@ EventPipeProviderCallbackData *
 ep_provider_callback_data_alloc_copy (EventPipeProviderCallbackData *provider_callback_data_src);
 
 EventPipeProviderCallbackData *
+ep_provider_callback_data_alloc_move (EventPipeProviderCallbackData *provider_callback_data_src);
+
+EventPipeProviderCallbackData *
 ep_provider_callback_data_init (
        EventPipeProviderCallbackData *provider_callback_data,
        const ep_char8_t *filter_data,
@@ -111,6 +114,11 @@ ep_provider_callback_data_init_copy (
        EventPipeProviderCallbackData *provider_callback_data_dst,
        EventPipeProviderCallbackData *provider_callback_data_src);
 
+EventPipeProviderCallbackData *
+ep_provider_callback_data_init_move (
+       EventPipeProviderCallbackData *provider_callback_data_dst,
+       EventPipeProviderCallbackData *provider_callback_data_src);
+
 void
 ep_provider_callback_data_fini (EventPipeProviderCallbackData *provider_callback_data);
 

diff --git a/src/native/eventpipe/ep.c b/src/native/eventpipe/ep.c
index eb401dc..ae8f3ce 100644 (file)
--- a/src/native/eventpipe/ep.c
+++ b/src/native/eventpipe/ep.c
@@ -261,6 +261,26 @@ ep_on_error:
 }
 
 EventPipeProviderCallbackData *
+ep_provider_callback_data_alloc_move (EventPipeProviderCallbackData *provider_callback_data_src)
+{
+       EventPipeProviderCallbackData *instance = ep_rt_object_alloc (EventPipeProviderCallbackData);
+       ep_raise_error_if_nok (instance != NULL);
+
+       if (provider_callback_data_src) {
+               *instance = *provider_callback_data_src;
+               memset (provider_callback_data_src, 0, sizeof (*provider_callback_data_src));
+       }
+
+ep_on_exit:
+       return instance;
+
+ep_on_error:
+       ep_provider_callback_data_free (instance);
+       instance = NULL;
+       ep_exit_error_handler ();
+}
+
+EventPipeProviderCallbackData *
 ep_provider_callback_data_init (
        EventPipeProviderCallbackData *provider_callback_data,
        const ep_char8_t *filter_data,
@@ -295,6 +315,19 @@ ep_provider_callback_data_init_copy (
        return provider_callback_data_dst;
 }
 
+EventPipeProviderCallbackData *
+ep_provider_callback_data_init_move (
+       EventPipeProviderCallbackData *provider_callback_data_dst,
+       EventPipeProviderCallbackData *provider_callback_data_src)
+{
+       EP_ASSERT (provider_callback_data_dst != NULL);
+       EP_ASSERT (provider_callback_data_src != NULL);
+
+       *provider_callback_data_dst = *provider_callback_data_src;
+       memset (provider_callback_data_src, 0, sizeof (*provider_callback_data_src));
+       return provider_callback_data_dst;
+}
+
 void
 ep_provider_callback_data_fini (EventPipeProviderCallbackData *provider_callback_data)
 {
@@ -621,6 +654,7 @@ disable_helper (EventPipeSessionID id)
                while (ep_provider_callback_data_queue_try_dequeue (provider_callback_data_queue, &provider_callback_data)) {
                        ep_rt_prepare_provider_invoke_callback (&provider_callback_data);
                        provider_invoke_callback (&provider_callback_data);
+                       ep_provider_callback_data_fini (&provider_callback_data);
                }
 
                ep_provider_callback_data_queue_fini (provider_callback_data_queue);
@@ -951,6 +985,7 @@ ep_enable (
        while (ep_provider_callback_data_queue_try_dequeue (provider_callback_data_queue, &provider_callback_data)) {
                ep_rt_prepare_provider_invoke_callback (&provider_callback_data);
                provider_invoke_callback (&provider_callback_data);
+               ep_provider_callback_data_fini (&provider_callback_data);
        }
 
 ep_on_exit:
@@ -1185,6 +1220,7 @@ ep_create_provider (
        while (ep_provider_callback_data_queue_try_dequeue (provider_callback_data_queue, &provider_callback_data)) {
                ep_rt_prepare_provider_invoke_callback (&provider_callback_data);
                provider_invoke_callback (&provider_callback_data);
+               ep_provider_callback_data_fini (&provider_callback_data);
        }
 
        ep_rt_notify_profiler_provider_created (provider);
@@ -1556,14 +1592,14 @@ ep_provider_callback_data_queue_enqueue (
        EventPipeProviderCallbackData *provider_callback_data)
 {
        EP_ASSERT (provider_callback_data_queue != NULL);
-       EventPipeProviderCallbackData *provider_callback_data_copy = ep_provider_callback_data_alloc_copy (provider_callback_data);
-       ep_raise_error_if_nok (provider_callback_data_copy != NULL);
-       ep_raise_error_if_nok (ep_rt_provider_callback_data_queue_push_tail (ep_provider_callback_data_queue_get_queue_ref (provider_callback_data_queue), provider_callback_data_copy));
+       EventPipeProviderCallbackData *provider_callback_data_move = ep_provider_callback_data_alloc_move (provider_callback_data);
+       ep_raise_error_if_nok (provider_callback_data_move != NULL);
+       ep_raise_error_if_nok (ep_rt_provider_callback_data_queue_push_tail (ep_provider_callback_data_queue_get_queue_ref (provider_callback_data_queue), provider_callback_data_move));
 
        return true;
 
 ep_on_error:
-       ep_provider_callback_data_free (provider_callback_data_copy);
+       ep_provider_callback_data_free (provider_callback_data_move);
        return false;
 }
 
@@ -1578,7 +1614,7 @@ ep_provider_callback_data_queue_try_dequeue (
 
        EventPipeProviderCallbackData *value = NULL;
        ep_raise_error_if_nok (ep_rt_provider_callback_data_queue_pop_head (ep_provider_callback_data_queue_get_queue_ref (provider_callback_data_queue), &value));
-       ep_provider_callback_data_init_copy (provider_callback_data, value);
+       ep_provider_callback_data_init_move (provider_callback_data, value);
        ep_provider_callback_data_free (value);
 
        return true;