[tsan] Fix synchronization in dispatch_sync

[kubamracek]

Summary

In the interceptor for dispatch_sync, we're currently missing synchronization between the callback and the code *after* the call to dispatch_sync. So we'll false report a race in:

// now executing on a background queue/thread
dispatch_sync(main_queue, ^{ // "main_queue" forces the callback to be executed on a different thread
  x = 42;
});
print(x); // back on the background_thread

This patch fixes this by adding an extra release+acquire pair to dispatch_sync() and similar APIs. Added a testcase.

compiler-rt/trunk/lib/tsan/rtl/tsan_libdispatch_mac.cc

 typedef long long_t;  // NOLINT
 
 namespace __tsan {
 
 typedef struct {
   dispatch_queue_t queue;
   void *orig_context;
   dispatch_function_t orig_work;
-  uptr object_to_acquire;
+  uptr sync_object;
   dispatch_object_t object_to_release;
+  bool free_context_in_callback;
+  bool release_sync_object_in_callback;
 } tsan_block_context_t;
 
 // The offsets of different fields of the dispatch_queue_t structure, exported
 // by libdispatch.dylib.
 extern "C" struct dispatch_queue_offsets_s {
   const uint16_t dqo_version;
   const uint16_t dqo_label;
   const uint16_t dqo_label_size;
static tsan_block_context_t *AllocContext(ThreadState *thr, uptr pc,
                                           dispatch_queue_t queue,
                                           void *orig_context,
                                           dispatch_function_t orig_work) {
   tsan_block_context_t *new_context =
       (tsan_block_context_t *)user_alloc(thr, pc, sizeof(tsan_block_context_t));
   new_context->queue = queue;
   new_context->orig_context = orig_context;
   new_context->orig_work = orig_work;
-  new_context->object_to_acquire = (uptr)new_context;
+  new_context->sync_object = (uptr)new_context;
   new_context->object_to_release = nullptr;
+  new_context->free_context_in_callback = true;
+  new_context->release_sync_object_in_callback = false;
   return new_context;
 }
 
-static void dispatch_callback_wrap_acquire(void *param) {
-  SCOPED_INTERCEPTOR_RAW(dispatch_async_f_callback_wrap);
+static void dispatch_callback_wrap(void *param) {
+  SCOPED_INTERCEPTOR_RAW(dispatch_callback_wrap);
   tsan_block_context_t *context = (tsan_block_context_t *)param;
-  Acquire(thr, pc, context->object_to_acquire);
+
+  Acquire(thr, pc, context->sync_object);
 
   // Extra retain/release is required for dispatch groups. We use the group
   // itself to synchronize, but in a notification (dispatch_group_notify
   // callback), it may be disposed already. To solve this, we retain the group
   // and release it here.
   if (context->object_to_release) dispatch_release(context->object_to_release);
 
   // In serial queues, work items can be executed on different threads, we need
   // to explicitly synchronize on the queue itself.
   if (IsQueueSerial(context->queue)) Acquire(thr, pc, (uptr)context->queue);
   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();
   context->orig_work(context->orig_context);
   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();
   if (IsQueueSerial(context->queue)) Release(thr, pc, (uptr)context->queue);
-  user_free(thr, pc, context);
+
+  if (context->release_sync_object_in_callback)
+    Release(thr, pc, context->sync_object);
+
+  if (context->free_context_in_callback) user_free(thr, pc, context);
 }
 
 static void invoke_and_release_block(void *param) {
   dispatch_block_t block = (dispatch_block_t)param;
   block();
   Block_release(block);
 }
 
 #define DISPATCH_INTERCEPT_B(name)                                           \
   TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, dispatch_block_t block) { \
     SCOPED_TSAN_INTERCEPTOR(name, q, block);                                 \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START(); \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                           \
     dispatch_block_t heap_block = Block_copy(block);                         \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END(); \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
     tsan_block_context_t *new_context =                                      \
         AllocContext(thr, pc, q, heap_block, &invoke_and_release_block);     \
     Release(thr, pc, (uptr)new_context);                                     \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START(); \
-    REAL(name##_f)(q, new_context, dispatch_callback_wrap_acquire);          \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END(); \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                           \
+    REAL(name##_f)(q, new_context, dispatch_callback_wrap);                  \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
   }
 
+#define DISPATCH_INTERCEPT_SYNC_B(name)                                      \
+  TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, dispatch_block_t block) { \
+    SCOPED_TSAN_INTERCEPTOR(name, q, block);                                 \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                           \
+    dispatch_block_t heap_block = Block_copy(block);                         \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
+    tsan_block_context_t new_context = {                                     \
+        q, heap_block, &invoke_and_release_block, 0, 0, false, true};        \
+    new_context.sync_object = (uptr)&new_context;                            \
+    Release(thr, pc, (uptr)&new_context);                                    \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                           \
+    REAL(name##_f)(q, &new_context, dispatch_callback_wrap);                 \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
+    Acquire(thr, pc, (uptr)&new_context);                                    \
+  }
+
 #define DISPATCH_INTERCEPT_F(name)                                \
   TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, void *context, \
                    dispatch_function_t work) {                    \
     SCOPED_TSAN_INTERCEPTOR(name, q, context, work);              \
     tsan_block_context_t *new_context =                           \
         AllocContext(thr, pc, q, context, work);                  \
     Release(thr, pc, (uptr)new_context);                          \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START(); \
-    REAL(name)(q, new_context, dispatch_callback_wrap_acquire);   \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END(); \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                \
+    REAL(name)(q, new_context, dispatch_callback_wrap);           \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                  \
+  }
+
+#define DISPATCH_INTERCEPT_SYNC_F(name)                                       \
+  TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, void *context,             \
+                   dispatch_function_t work) {                                \
+    SCOPED_TSAN_INTERCEPTOR(name, q, context, work);                          \
+    tsan_block_context_t new_context = {q, context, work, 0, 0, false, true}; \
+    new_context.sync_object = (uptr)&new_context;                             \
+    Release(thr, pc, (uptr)&new_context);                                     \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                            \
+    REAL(name)(q, &new_context, dispatch_callback_wrap);                      \
+    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                              \
+    Acquire(thr, pc, (uptr)&new_context);                                     \
   }
 
 // We wrap dispatch_async, dispatch_sync and friends where we allocate a new
 // context, which is used to synchronize (we release the context before
 // submitting, and the callback acquires it before executing the original
 // callback).
 DISPATCH_INTERCEPT_B(dispatch_async)
 DISPATCH_INTERCEPT_B(dispatch_barrier_async)
 DISPATCH_INTERCEPT_F(dispatch_async_f)
 DISPATCH_INTERCEPT_F(dispatch_barrier_async_f)
-DISPATCH_INTERCEPT_B(dispatch_sync)
-DISPATCH_INTERCEPT_B(dispatch_barrier_sync)
-DISPATCH_INTERCEPT_F(dispatch_sync_f)
-DISPATCH_INTERCEPT_F(dispatch_barrier_sync_f)
+DISPATCH_INTERCEPT_SYNC_B(dispatch_sync)
+DISPATCH_INTERCEPT_SYNC_B(dispatch_barrier_sync)
+DISPATCH_INTERCEPT_SYNC_F(dispatch_sync_f)
+DISPATCH_INTERCEPT_SYNC_F(dispatch_barrier_sync_f)
 
 // GCD's dispatch_once implementation has a fast path that contains a racy read
 // and it's inlined into user's code. Furthermore, this fast path doesn't
 // establish a proper happens-before relations between the initialization and
 // code following the call to dispatch_once. We could deal with this in
 // instrumented code, but there's not much we can do about it in system
 // libraries. Let's disable the fast path (by never storing the value ~0 to
 // predicate), so the interceptor is always called, and let's add proper release

 TSAN_INTERCEPTOR(void, dispatch_group_notify, dispatch_group_t group,
                  dispatch_queue_t q, dispatch_block_t block) {
   SCOPED_TSAN_INTERCEPTOR(dispatch_group_notify, group, q, block);
   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();
   dispatch_block_t heap_block = Block_copy(block);
   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();
   tsan_block_context_t *new_context =
       AllocContext(thr, pc, q, heap_block, &invoke_and_release_block);
-  new_context->object_to_acquire = (uptr)group;
+  new_context->sync_object = (uptr)group;
 
-  // Will be released in dispatch_callback_wrap_acquire.
+  // Will be released in dispatch_callback_wrap.
   new_context->object_to_release = group;
   dispatch_retain(group);
 
   Release(thr, pc, (uptr)group);
   REAL(dispatch_group_notify_f)(group, q, new_context,
-                                dispatch_callback_wrap_acquire);
+                                dispatch_callback_wrap);
 }
 
 TSAN_INTERCEPTOR(void, dispatch_group_notify_f, dispatch_group_t group,
                  dispatch_queue_t q, void *context, dispatch_function_t work) {
   SCOPED_TSAN_INTERCEPTOR(dispatch_group_notify_f, group, q, context, work);
   tsan_block_context_t *new_context = AllocContext(thr, pc, q, context, work);
-  new_context->object_to_acquire = (uptr)group;
+  new_context->sync_object = (uptr)group;
 
-  // Will be released in dispatch_callback_wrap_acquire.
+  // Will be released in dispatch_callback_wrap.
   new_context->object_to_release = group;
   dispatch_retain(group);
 
   Release(thr, pc, (uptr)group);
   REAL(dispatch_group_notify_f)(group, q, new_context,
-                                dispatch_callback_wrap_acquire);
+                                dispatch_callback_wrap);
 }
 
 }  // namespace __tsan
 
 #endif  // SANITIZER_MAC

compiler-rt/trunk/test/tsan/Darwin/gcd-blocks.mm

+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+int main() {
+  fprintf(stderr, "start\n");
+
+  dispatch_queue_t background_q = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+  dispatch_queue_t main_q = dispatch_get_main_queue();
+
+  dispatch_async(background_q, ^{
+    __block long block_var = 0;
+
+    dispatch_sync(main_q, ^{
+      block_var = 42;
+    });
+
+    fprintf(stderr, "block_var = %ld\n", block_var);
+
+    dispatch_sync(dispatch_get_main_queue(), ^{
+      CFRunLoopStop(CFRunLoopGetCurrent());
+    });
+  });
+  
+  CFRunLoopRun();
+  fprintf(stderr, "done\n");
+}
+
+// CHECK: start
+// CHECK: block_var = 42
+// CHECK: done
+// CHECK-NOT: WARNING: ThreadSanitizer
+// CHECK-NOT: CHECK failed