[TSan][Darwin] Avoid potential crashes after unobserved operations

[yln]

Summary

Certain frameworks (e.g., IOSurface, IOKit) map and unmap VM ranges
through direct interaction with the kernel. So unfortunately, there is
no API that we can intercept to observe these operations and clear the
associated shadow memory. When such a previously-used VM region is
reused for a new thread, the shadow memory for the pthread_t struct may
have leftover, non-zero shadow bytes. Previously, we erroneously
interpreted these non-zero bytes as a pointer to the ThreadState
metadata and crashed.

In SignalSafeGetOrAllocate() we assumed all non-zero values to be
valid pointers, which led to crashes when a pthread_t struct later is
placed the the corresponding app memory and we interpret the shadow as
our pointer to the ThreadState metadata.

Now we always allocate this ThreadState metadata once and only once at
thread creation and remove the "get or allocate" semantics. We
therefore don't depend on the previous value anymore.

This requires that we don't call any interceptors before THREAD_CREATE
runs, which is a condition that currently holds. I added additional
asserts and tests to ensure we notice if this changes.

This change also fixes the following bug: previously we deallocated the
ThreadState metadata in THREAD_TERMINATE, however, some interceptors
still get called after that, which re-creates a new ThreadState
instance for the shutting down thread. Usually this worked "by
accident". The created metadata was either leaked or reused when
another thread was created in its place.

rdar://72010355

[kubamracek]

compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp

217

Let's please add a comment containing as much detailed explanation as possible of what exactly the problem is and how it's being addressed here.

[yln]

Update patch.

[yln]

CHECK -> DCHECK

[yln]

Improve interaction between THREAD_TERMINATE/DESTROY.

[delcypher]

compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp

234

The The current thread is a newly created GCD worker thread. comment got dropped. Is that intentional?

250

This used to be an if (condition), now it's a runtime check that needs to hold. Do we know why it was an if () condition before? Probably worth a comment if there are some subtleties here.

251

Why do we call ThreadFinish(cur_thread()) here rather than effectively letting the DestroyThreadState() do everything in the PTHREAD_INTROSPECTION_THREAD_DESTROY branch?

260

If we need to go with the approach of skipping the ThreadFinish() call, to avoid duplicating code (that may become out of date) how about we change DestroyThreadState() to take a bool parameter that is used to decided if ThreadFinish() should be called?

[yln]

Abandoning in favor of D110236

compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp

 //===-- tsan_platform_mac.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer (TSan), a race detector.
 //
 // Mac-specific code.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_platform.h"
 #if SANITIZER_MAC
 
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_posix.h"
 #include "sanitizer_common/sanitizer_procmaps.h"
 #include "sanitizer_common/sanitizer_ptrauth.h"
 #include "sanitizer_common/sanitizer_stackdepot.h"
+#include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "tsan_platform.h"
 #include "tsan_rtl.h"
 #include "tsan_flags.h"
 
 #include <mach/mach.h>
 #include <pthread.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <stdarg.h>
 #include <sys/mman.h>
 #include <sys/syscall.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <errno.h>
 #include <sched.h>
 
 namespace __tsan {
 
 #if !SANITIZER_GO
-static void *SignalSafeGetOrAllocate(uptr *dst, uptr size) {
+static void SignalSafeAllocate(uptr *dst, uptr size) {
   atomic_uintptr_t *a = (atomic_uintptr_t *)dst;
   void *val = (void *)atomic_load_relaxed(a);
   atomic_signal_fence(memory_order_acquire);  // Turns the previous load into
                                               // acquire wrt signals.
-  if (UNLIKELY(val == nullptr)) {
-    val = (void *)internal_mmap(nullptr, size, PROT_READ | PROT_WRITE,
-                                MAP_PRIVATE | MAP_ANON, -1, 0);
-    CHECK(val);
-    void *cmp = nullptr;
-    if (!atomic_compare_exchange_strong(a, (uintptr_t *)&cmp, (uintptr_t)val,
-                                        memory_order_acq_rel)) {
-      internal_munmap(val, size);
-      val = cmp;
-    }
-  }
-  return val;
-}
 
 // On OS X, accessing TLVs via __thread or manually by using pthread_key_* is
 // problematic, because there are several places where interceptors are called
 // when TLVs are not accessible (early process startup, thread cleanup, ...).
 // The following provides a "poor man's TLV" implementation, where we use the
 // shadow memory of the pointer returned by pthread_self() to store a pointer to
 // the ThreadState object. The main thread's ThreadState is stored separately
 // in a static variable, because we need to access it even before the
 // shadow memory is set up.
 static uptr main_thread_identity = 0;
 ALIGNED(64) static char main_thread_state[sizeof(ThreadState)];
 static ThreadState *main_thread_state_loc = (ThreadState *)main_thread_state;
 
+static ThreadState **thread_location(uptr thread_identity) {
+  if (thread_identity == main_thread_identity)
+    return &main_thread_state_loc;
+  return (ThreadState **)MemToShadow(thread_identity);
+}
+
 // We cannot use pthread_self() before libpthread has been initialized.  Our
 // current heuristic for guarding this is checking `main_thread_identity` which
 // is only assigned in `__tsan::InitializePlatform`.
 static ThreadState **cur_thread_location() {
   if (main_thread_identity == 0)
     return &main_thread_state_loc;
-  uptr thread_identity = (uptr)pthread_self();
-  if (thread_identity == main_thread_identity)
-    return &main_thread_state_loc;
-  return (ThreadState **)MemToShadow(thread_identity);
+  return thread_location((uptr)pthread_self());
 }
 
-ThreadState *cur_thread() {
-  return (ThreadState *)SignalSafeGetOrAllocate(
-      (uptr *)cur_thread_location(), sizeof(ThreadState));
-}
+ThreadState *cur_thread() { return *cur_thread_location(); }
 
 void set_cur_thread(ThreadState *thr) {
   *cur_thread_location() = thr;
 }
 
+// Do nothing here, we want to do this final step of cleanup in THREAD_DESTROY.
+void cur_thread_finalize() {}
+
 // TODO(kuba.brecka): This is not async-signal-safe. In particular, we call
 // munmap first and then clear `fake_tls`; if we receive a signal in between,
 // handler will try to access the unmapped ThreadState.
-void cur_thread_finalize() {
-  ThreadState **thr_state_loc = cur_thread_location();
-  if (thr_state_loc == &main_thread_state_loc) {
-    // Calling dispatch_main() or xpc_main() actually invokes pthread_exit to
-    // exit the main thread. Let's keep the main thread's ThreadState.
-    return;
-  }
+void thread_finalize(uptr thread_identity) {
+  // exit the main thread.  Ensure we keep the main thread's ThreadState.
+  CHECK_NE(thread_identity, main_thread_identity);
+  ThreadState **thr_state_loc = thread_location(thread_identity);
   internal_munmap(*thr_state_loc, sizeof(ThreadState));
   *thr_state_loc = nullptr;
 }
 #endif
 
 void FlushShadowMemory() {
 }
 
 static void RegionMemUsage(uptr start, uptr end, uptr *res, uptr *dirty) {
   vm_address_t address = start;
   vm_address_t end_address = end;
   uptr resident_pages = 0;

 
 // On OS X, GCD worker threads are created without a call to pthread_create. We
 // need to properly register these threads with ThreadCreate and ThreadStart.
 // These threads don't have a parent thread, as they are created "spuriously".
 // We're using a libpthread API that notifies us about a newly created thread.
 // The `thread == pthread_self()` check indicates this is actually a worker
 // thread. If it's just a regular thread, this hook is called on the parent
 // thread.
+enum {
+  PTHREAD_INTROSPECTION_THREAD_CREATE = 1,
+  PTHREAD_INTROSPECTION_THREAD_START,
+  PTHREAD_INTROSPECTION_THREAD_TERMINATE,
+  PTHREAD_INTROSPECTION_THREAD_DESTROY,
+};
 typedef void (*pthread_introspection_hook_t)(unsigned int event,
                                              pthread_t thread, void *addr,
                                              size_t size);
 extern "C" pthread_introspection_hook_t pthread_introspection_hook_install(
     pthread_introspection_hook_t hook);
-static const uptr PTHREAD_INTROSPECTION_THREAD_CREATE = 1;
-static const uptr PTHREAD_INTROSPECTION_THREAD_TERMINATE = 3;
+
 static pthread_introspection_hook_t prev_pthread_introspection_hook;
 static void my_pthread_introspection_hook(unsigned int event, pthread_t thread,
                                           void *addr, size_t size) {
-  if (event == PTHREAD_INTROSPECTION_THREAD_CREATE) {
-    if (thread == pthread_self()) {
-      // The current thread is a newly created GCD worker thread.
+  switch (event) {
+    case PTHREAD_INTROSPECTION_THREAD_CREATE: {
+      CHECK_NE(thread, main_thread_identity);
+      ThreadState **thr_loc = thread_location((uptr)thread);
+      DCHECK_EQ(*thr_loc, nullptr);
+      // Note: the above is a debug check (DCHECK) on purpose!
+      // Certain frameworks (e.g., IOSurface, IOKit) map and unmap VM ranges
+      // through direct interaction with the kernel.  So unfortunately, there is
+      // no API that we can intercept to observe these operations and clear the
+      // associated shadow memory.  When such a previously-used VM region is
+      // reused for a new thread, the shadow memory for the pthread_t struct may
+      // have leftover, non-zero shadow bytes.  Previously, we erroneously
+      // interpreted these non-zero bytes as a pointer to the ThreadState
+      // metadata and crashed.  Now we allocate this ThreadState metadata below
+      // so the current value does not matter.  However, the above failure
+      // situation is so rare that we still want to check this assertion in
+      // debug builds and during testing to avoid hiding other failures.
+      SignalSafeAllocate((uptr *)thr_loc, sizeof(ThreadState));
+
+      bool gcd_worker = (thread == pthread_self());
+      if (gcd_worker) {
         ThreadState *thr = cur_thread();
         Processor *proc = ProcCreate();
         ProcWire(proc, thr);
         ThreadState *parent_thread_state = nullptr;  // No parent.
         Tid tid = ThreadCreate(parent_thread_state, 0, (uptr)thread, true);
         CHECK_NE(tid, kMainTid);
         ThreadStart(thr, tid, GetTid(), ThreadType::Worker);
       }
-  } else if (event == PTHREAD_INTROSPECTION_THREAD_TERMINATE) {
-    if (thread == pthread_self()) {
-      ThreadState *thr = cur_thread();
-      if (thr->tctx) {
-        DestroyThreadState();
-      }
+      break;
+    case PTHREAD_INTROSPECTION_THREAD_START: {
+      CHECK_EQ(thread, pthread_self());
+      break;
+    }
+    case PTHREAD_INTROSPECTION_THREAD_TERMINATE: {
+      CHECK_EQ(thread, pthread_self());
+      ThreadFinish(cur_thread());
+      // Can't call full DestroyThreadState() here: interceptors in an "outer"
+      // THREAD_TERMINATE introspection hook might still get called on the
+      // terminating thread (e.g., libBacktraceRecording for Xcode "Queue
+      // Debugging" feature).
+      break;
+    }
+    case PTHREAD_INTROSPECTION_THREAD_DESTROY: {
+      CHECK_NE(thread, main_thread_identity);
+      // Remaining cleanup from DestroyThreadState() except ThreadFinish().
+      ThreadState *thr = *thread_location((uptr)thread);
+      Processor *proc = thr->proc();
+      ProcUnwire(proc, thr);
+      ProcDestroy(proc);
+      DTLS_Destroy();
+      thread_finalize((uptr)thread);  // cur_thread_finalize();
+      break;
     }
   }
 
   if (prev_pthread_introspection_hook != nullptr)
     prev_pthread_introspection_hook(event, thread, addr, size);
 }
 #endif