Fix couple of races in EventPipe C library. (#46193)
authorJohan Lorensson <lateralusx.github@gmail.com>
Tue, 12 Jan 2021 10:37:07 +0000 (11:37 +0100)
committerGitHub <noreply@github.com>
Tue, 12 Jan 2021 10:37:07 +0000 (11:37 +0100)
commit2b54e45f0c6e8fe929d4cd40823c0fb1e76b5a15
tree811ec9359a286a9031da9dfa386cea9e65317a44
parent7ffe282af85c6d5ab3f98e6959df39aec9d5ed0f
Fix couple of races in EventPipe C library. (#46193)

Fix primarily two main issues triggering races related to ref counting on
EventPipeThread.

First was an incorrect mapping of hash map add method in CoreClr shim.
Intent of this method was a add_replace, but CoreClr shim ended up with
Add implementation. That could result in unreferenced threads being added
into thread_sequence_number_map, that on free would release the ref count
creating unbalance, triggering EventPipeThreads to be terminated to soon.

There is also a theoretical race (don't know if it has been hit) that
manifests when we end up adding threads into thread_sequence_number_map
instead of replacing an existing already ref counted item. If we add,
then we will hit the same race, so added a fix making sure we detect
this case and correctly makes an addref to the thread. This potential
issue exists in C++ version of EventPipe library as well.

The other issue triggering another race related to ref counted threads.
The streaming thread in C library used ep_thread_get_or_create to get a
reference to itself. This will create an EventPipeThread
instance in TLS, but it will also add thread into global thread list.
Since the streaming thread doesn’t do additional writes of events it
will not get a thread session state, meaning that it will keep its original
ref count. This opens up a race when the TLS destructor decrements
the reference count, and another thread disabling a session. The
thread that disables the session will get a copy of all running threads,
but if the thread running TLS destructor managed to be the first
hitting ref count down to 0, but other thread will beat it taking
locks protecting list, the thread with refcount == 0 will still
be in the list copy, meaning the other thread will end up with a
potentially freed object on its thread list copy.

I made two fixes for this issue, first, don’t use ep_thread_get_or_create
from streaming thread, since there is an ability to get hold of
runtime specific thread object, that will eliminate the race between
a thread added into EventPipe, but not writing events, so it won’t have
an thread session state holding an additional ref count. Second, since
the race is still there due to the spit between ref count and lock
protecting the list, I also made a change in the C library, moving the
register/unregister on the list into the ep_thread_get_or_create (register)
and TLS destructor (unregister), meaning that the list will track live
threads currently using EventPipe library, and the EventPipeThread
object is still ref counted, meaning that it still can outlive the
lifetime of the physical thread.

Doing this split close the potential race condition (and even fix the
scenario if we kept the streaming thread added into the global thread list).
This hypothetical race condition exists in C++ library as well but might
have been mitigated by ref counting thread session state and making sure
the TLS destructor won’t race with a thread doing disable since thread
session state ref count will be held past par of disable that could race.
17 files changed:
src/coreclr/inc/shash.h
src/coreclr/inc/shash.inl
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
src/mono/mono/eventpipe/ep-rt-mono.h
src/mono/mono/eventpipe/ep-rt-types-mono.h
src/mono/mono/eventpipe/test/ep-thread-tests.c
src/native/eventpipe/ep-buffer-manager.c
src/native/eventpipe/ep-config.c
src/native/eventpipe/ep-event-source.c
src/native/eventpipe/ep-provider-internals.h
src/native/eventpipe/ep-provider.c
src/native/eventpipe/ep-rt.h
src/native/eventpipe/ep-sample-profiler.c
src/native/eventpipe/ep-session.c
src/native/eventpipe/ep-session.h
src/native/eventpipe/ep-thread.c
src/native/eventpipe/ep-thread.h