Bug 58323 - Race conditions with mono_thread_detach_internal (thread->state)
Summary: Race conditions with mono_thread_detach_internal (thread->state)
Alias: None
Product: Runtime
Classification: Mono
Component: General ()
Version: unspecified
Hardware: PC Linux
: --- normal
Target Milestone: ---
Assignee: Bugzilla
Depends on:
Reported: 2017-07-23 17:05 UTC by Armin
Modified: 2017-07-28 18:03 UTC (History)
4 users (show)

Is this bug a regression?: ---
Last known good build:

Stack traces (16.63 KB, text/plain)
2017-07-23 17:05 UTC, Armin

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Description Armin 2017-07-23 17:05:25 UTC
Created attachment 23739 [details]
Stack traces

With the help of TSan (please find the corresponding stack traces attached), I found two different race conditions that are both tied to `mono_thread_detach_internal ()`. More precisely they are connected to `thread->state`:

void mono_thread_detach_internal (MonoInternalThread *thread) {
  // ...
  LOCK_THREAD (thread);
  thread->state |= ThreadState_Stopped;
! thread->state &= ~ThreadState_Background;
  // ...
  UNLOCK_THREAD (thread);
  // ...

While writing `thread->state` is mutex protected, reading it is not; e.g. in:

static gboolean remove_and_abort_threads (gpointer key, gpointer value, gpointer user) {
  // ...
! if ((thread->state & ThreadState_Background) && !(thread->flags & MONO_THREAD_FLAG_DONT_MANAGE)) {
  // ...

... or in:

static void build_wait_tids (gpointer key, gpointer value, gpointer user) {
  // ...
  /* Do not lock here since it is not needed and the caller holds threads_lock */
! if (thread->state & ThreadState_Background) {
  // ...

Throughout the file threads.c, some reads of `thread->state` (also talking about simple ones like in `ves_icall_System_Threading_Thread_Join_internal ()`) are protected with `LOCK_THREAD (thread)` while others are not.

Shouldn't / couldn't `LOCK_THREAD (thread);` be used for all reads of `thread->state` since these functions are only called a handful of times (at least `remove_and_abort_threads ()` and `build_wait_tids ()`)?

Furthermore, the comment in `build_wait_tids ()` states that threads_lock is held by the caller. I am not sure if this is also meant to comment the read of `thread->state` as, as far as I understand it, `LOCK_THREAD (thread)` of `mono_thread_detach_internal ()` uses a different mutex.

(commit 2a3006438d48796def3c20743a891b97d31cd6cb)
Comment 1 Ludovic Henry 2017-07-28 18:03:47 UTC
For the cases of `remove_and_abort_threads` and `build_wait_tids`, reading `thread->state` out of a lock is acceptable since we only read it, and do not write it concurrently. We could switch all the read and write to InterlockExchange + InterlockedRead, but that would be too much overhead for no gain.

For the case of the comment in `build_wait_tids`, the lock that is held is not the per-thread lock, but the `threads_mutex` lock which is to synchronise access to the static `threads` variable which contains all the threads.