Skip to content

Commit

Permalink
Fix memory leak in enqueue/dequeue of EventPipe callback data.
Browse files Browse the repository at this point in the history 
dotnet#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.
  • Loading branch information
@lateralusX
lateralusX committed Aug 26, 2021
1 parent 8455a5e commit 1ae4bf2
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 18 deletions.
27 changes: 15 additions & 12 deletions src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/eventpipe/test/ep-tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions src/native/eventpipe/ep-config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 (
Expand All @@ -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);
}
}
}
Expand Down
46 changes: 41 additions & 5 deletions src/native/eventpipe/ep.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,26 @@ ep_provider_callback_data_alloc_copy (EventPipeProviderCallbackData *provider_ca
ep_exit_error_handler ();
}

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,
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand Down

0 comments on commit 1ae4bf2

Please sign in to comment.