Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent Segfault in EventPipe on disable #56104

Merged
merged 4 commits into from
Jul 27, 2021
Merged

Prevent Segfault in EventPipe on disable #56104

merged 4 commits into from
Jul 27, 2021

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Jul 21, 2021

During EventPipe disable, we call perform some work under a lock and queue some data for use after releasing the lock.

runtime/src/native/eventpipe/ep.c

Lines 607 to 621 in 52262b8

EventPipeProviderCallbackDataQueue callback_data_queue;
EventPipeProviderCallbackData provider_callback_data;
EventPipeProviderCallbackDataQueue *provider_callback_data_queue = ep_provider_callback_data_queue_init (&callback_data_queue);
EP_LOCK_ENTER (section1)
if (ep_volatile_load_number_of_sessions () > 0)
disable_holding_lock (id, provider_callback_data_queue);
EP_LOCK_EXIT (section1)
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_queue_fini (provider_callback_data_queue);

The issue is that while under the lock we also call ep_session_disable, which clears the list of providers we used to generate the callback queue in the above snippet. Most of the fields on the structs are primitives, but filter_data is a utf8 string, so we end up freeing the string and then immediately attempt to use it afterwards. This patch changes ownership rules and dups the string for the ep_provider_callback_data struct.

@josalem josalem added this to the 6.0.0 milestone Jul 21, 2021
@josalem josalem requested review from lateralusX and a team July 21, 2021 17:33
@josalem josalem self-assigned this Jul 21, 2021
@davmason
Copy link
Member

Deja vu? #42307

lateralusX
lateralusX reviewed Jul 22, 2021
View reviewed changes
src/native/eventpipe/ep-provider.c Outdated Show resolved Hide resolved
src/native/eventpipe/ep-types.h Outdated Show resolved Hide resolved
src/native/eventpipe/ep.c Outdated Show resolved Hide resolved
src/native/eventpipe/ep.c Outdated Show resolved Hide resolved
src/native/eventpipe/ep-types.h Outdated Show resolved Hide resolved
src/native/eventpipe/ep-types.h Outdated Show resolved Hide resolved
src/native/eventpipe/ep.c Outdated Show resolved Hide resolved
src/native/eventpipe/ep.c Outdated Show resolved Hide resolved
@josalem
Copy link
Contributor Author

josalem commented Jul 23, 2021

Deja vu? #42307

I knew I had seen this issue before. Looks like this slipped through when we transferred to the C impl.

@josalem josalem merged commit 598c2da into dotnet:main Jul 27, 2021
@josalem josalem deleted the dev/josalem/provider-string-uaf branch July 27, 2021 22:55
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into improve_hot_reload_de…
1930315 
…bug_tests

* origin/main: (274 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_js_tests
3f83a08 
* origin/main: (95 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
@josalem josalem mentioned this pull request Aug 2, 2021
Closed
lateralusX added a commit to lateralusX/runtime that referenced this pull request Aug 26, 2021
@lateralusX
Fix memory leak in enqueue/dequeue of EventPipe callback data.
1ae4bf2 
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.
@lateralusX lateralusX mentioned this pull request Aug 26, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hoyosjs hoyosjs hoyosjs approved these changes

@davmason davmason davmason approved these changes

@lateralusX lateralusX lateralusX approved these changes

Assignees

@josalem josalem

Labels
area-Tracing-coreclr EventPipe
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@josalem @davmason @lateralusX @hoyosjs