[tsan] Update dispatch_group support to avoid using a disposed group object

[kubamracek]

Summary

We're using the dispatch group itself to synchronize (to call Release() and Acquire() on it), but in dispatch group notifications, the group can already be disposed/deallocated. This causes a later assertion failure at DCHECK_EQ(*meta, 0); in MetaMap::AllocBlock when the same memory is reused (note that the failure only happens in debug builds).

Fixing this by retaining the group and releasing it in the notification. Adding a stress test case that reproduces this.

[kubamracek]

Also, shouldn't we have some CHECK/DCHECK to make sure Acquire() and Release() aren't called on deallocated memory?

[dvyukov]

lib/tsan/rtl/tsan_libdispatch_mac.cc

100

Looks like use-after-free. You've just freed the context line above.

100

Please add a comment somewhere here explaining why we need this additional lifetime management.

100

I don't understand the exact scenario that leads to the problem. The group is context->object_to_acquire, which we touch in the very beginning of this function. Then we call user callback orig_work. In your tests orig_work calls dispatch_group_leave on the group. So either (1) test code is also buggy, or (2) we don't have use-after-free in this function and don't need this change, or (3) I am missing something.

[kubamracek]

lib/tsan/rtl/tsan_libdispatch_mac.cc

100

In your tests orig_work calls dispatch_group_leave

No, orig_work here is the empty block (or empty function) passed as argument to dispatch_group_notify (or dispatch_group_notify_f). The issue is that the API doesn't guarantee that the group itself will still be alive, when the notification is invoked.

Looks like use-after-free. You've just freed the context line above.

Oops, you're right.

[dvyukov]

lib/tsan/rtl/tsan_libdispatch_mac.cc

100

I've missed that it is only for dispatch_group_notify.
Then, I think you need to move the release right below the Acquire and add a comment.

As a side note, it can make sense to detect such bugs in user code. If you add MemoryRead/Write in necessary places, tsan will be able to detect such racy-use-after-free usages. You can see tsan_rtl_mutex.cc for an example.

[kubamracek]

Updating patch.

[kubamracek]

lib/tsan/rtl/tsan_libdispatch_mac.cc

100

To detect racy-use-after-free, do we need to annotate all uses of the object (a dispatch group in this case) or is it okay to miss some? Will that produce false positives? Because here, the group is deallocated either by the explicit dispatch_retain on the main thread, or implicitly when the async block finishes. I'm not sure how to write a reliable test that would catch the bug here.

[dvyukov]

LGTM

lib/tsan/rtl/tsan_libdispatch_mac.cc

100

It is OK to miss some. As for reliable test, check out e.g. race_on_mutex2.c.
I can't say about implicit/explicit deallocation, you will need to experiment to see how we can detect some misuses without false positives.

lib/tsan/rtl/tsan_libdispatch_mac.cc

 
 namespace __tsan {
 
 typedef struct {
   dispatch_queue_t queue;
   void *orig_context;
   dispatch_function_t orig_work;
   uptr object_to_acquire;
+  dispatch_object_t object_to_release;
 } 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,
                                           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->object_to_release = nullptr;
   return new_context;
 }
 
 static void dispatch_callback_wrap_acquire(void *param) {
   SCOPED_INTERCEPTOR_RAW(dispatch_async_f_callback_wrap);
   tsan_block_context_t *context = (tsan_block_context_t *)param;
   Acquire(thr, pc, context->object_to_acquire);
+
+  // 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);
   context->orig_work(context->orig_context);
   if (IsQueueSerial(context->queue)) Release(thr, pc, (uptr)context->queue);
   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, dispatch_group_notify, dispatch_group_t group,
                  dispatch_queue_t q, dispatch_block_t block) {
   SCOPED_TSAN_INTERCEPTOR(dispatch_group_notify, group, q, block);
   dispatch_block_t heap_block = Block_copy(block);
   tsan_block_context_t *new_context =
       AllocContext(thr, pc, q, heap_block, &invoke_and_release_block);
   new_context->object_to_acquire = (uptr)group;
+
+  // Will be released in dispatch_callback_wrap_acquire.
+  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);
 }
 
 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;
+
+  // Will be released in dispatch_callback_wrap_acquire.
+  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);
 }
 
 }  // namespace __tsan
 
 #endif  // SANITIZER_MAC

test/tsan/Darwin/gcd-groups-stress.mm

+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %run %t 2>&1
+
+#import <Foundation/Foundation.h>
+
+void notify_callback(void *context) {
+  // Do nothing.
+}
+
+int main() {
+  NSLog(@"Hello world.");
+
+  dispatch_queue_t q = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+  
+  for (int i = 0; i < 300000; i++) {
+    dispatch_group_t g = dispatch_group_create();
+    dispatch_group_enter(g);
+    dispatch_async(q, ^{
+      dispatch_group_leave(g);
+    });
+    dispatch_group_notify(g, q, ^{
+      // Do nothing.
+    });
+    dispatch_release(g);
+  }
+
+  for (int i = 0; i < 300000; i++) {
+    dispatch_group_t g = dispatch_group_create();
+    dispatch_group_enter(g);
+    dispatch_async(q, ^{
+      dispatch_group_leave(g);
+    });
+    dispatch_group_notify_f(g, q, nullptr, &notify_callback);
+    dispatch_release(g);
+  }
+
+  NSLog(@"Done.");
+}
+
+// CHECK: Hello world.
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer
+// CHECK-NOT: CHECK failed