Skip to content

Commit

Permalink
[release/6.0] Fix memory leak in enqueue/dequeue of EventPipe callbac…
Browse files Browse the repository at this point in the history 
…k data. (#58244)

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

#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 <[email protected]>
  • Loading branch information
@github-actions @lateralusX
github-actions[bot] and lateralusX authored Aug 27, 2021
1 parent a021027 commit 59f5157
Show file tree
Hide file tree
Showing 5 changed files with 70 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
8 changes: 8 additions & 0 deletions src/native/eventpipe/ep-types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ ep_provider_callback_data_alloc (
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,
Expand All @@ -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);

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 59f5157

Please sign in to comment.