[tsan] Avoid false positives with GCD data callbacks

[kubamracek]

Summary

This patch adds synchronization between the creation of the GCD data object and destructor’s execution. It’s far from perfect, because ideally we’d want to synchronize the destruction of the last reference (via dispatch_release) and the destructor’s execution, but intercepting objc_release is problematic.

[dvyukov]

It’s far from perfect, because ideally we’d want to synchronize the destruction of the last reference (via dispatch_release) and the destructor’s execution, but intercepting objc_release is problematic.

Why don't we intercept dispatch_release?

[dvyukov]

lib/tsan/rtl/tsan_libdispatch_mac.cc

488

Will it work with DISPATCH_DATA_DESTRUCTOR_FREE/DEFAULT?
DISPATCH_DATA_DESTRUCTOR_FREE seems to be initialized to some block, but at the same time runtime avoids calling it directly and instead calls free directly. Though, maybe it's just for performance reasons.

[kubamracek]

In D21990#474097, @dvyukov wrote:

It’s far from perfect, because ideally we’d want to synchronize the destruction of the last reference (via dispatch_release) and the destructor’s execution, but intercepting objc_release is problematic.

Why don't we intercept dispatch_release?

1. We’d need to intercept both dispatch_release and objc_release, in some cases they can be used interchangeably (toll-free bridging), which means we should add dispatch_retain and objc_retain as well. Theses functions are called *a lot* and I’m worried about performance, but more importantly about all the added synchronization on refcounts, which is going to cause false negatives.
2. We cannot tell if the release is going to destroy the object or not. We can’t take a look at the refcount, because that’s an internal implementation detail and it actually changes.
3. This will still not cover all the false positives with GCD objects, because there are shortcuts in GCD internals that release/destroy objects without going through dispatch_release.

Still, I’m probably going to try to add those interceptors at some point, but it sounds like quite a non-trivial task to do it right.

[kubamracek]

Updating the patch to handle DISPATCH_DATA_DESTRUCTOR_FREE and other special values. The original patch actually crashed when these were used (the block that this magic value is pointing to isn’t supposed to be called at all). Thanks!

Also checking when queue is NULL, it seems there is some code that is using it (although the docs isn’t clear what the behavior is then).

[dvyukov]

lib/tsan/rtl/tsan_libdispatch_mac.cc

484

Won't this cause false positives?
Tsan should intercept free in DISPATCH_DATA_DESTRUCTOR_FREE. If we don't synchronize dispatch_data_create with the free, free can race with allocation.

[kubamracek]

lib/tsan/rtl/tsan_libdispatch_mac.cc

484

I guess you’re right, but we’re currently still using ignore_interceptors_accesses=1 on Darwin to avoid false positives from system libraries, which also suppresses races with free.

Anyway, what would be the solution here? Release() on buffer?

[dvyukov]

lib/tsan/rtl/tsan_libdispatch_mac.cc

484

Anyway, what would be the solution here? Release() on buffer?

If destructor == DISPATCH_DATA_DESTRUCTOR_FREE, replace it with a block that calls free?

[kubamracek]

Updating patch.

lib/tsan/rtl/tsan_libdispatch_mac.cc

TSAN_INTERCEPTOR(void, dispatch_apply_f, size_t iterations,
                  void (*work)(void *, size_t)) {
   SCOPED_TSAN_INTERCEPTOR(dispatch_apply_f, iterations, queue, context, work);
   void (^new_block)(size_t) = ^(size_t iteration) {
     work(context, iteration);
   };
   WRAP(dispatch_apply)(iterations, queue, new_block);
 }
 
+TSAN_INTERCEPTOR(dispatch_data_t, dispatch_data_create, const void *buffer,
+                 size_t size, dispatch_queue_t q, dispatch_block_t destructor) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_data_create, buffer, size, q, destructor);
+  if ((q == nullptr) || (destructor == DISPATCH_DATA_DESTRUCTOR_DEFAULT ||
+                         destructor == DISPATCH_DATA_DESTRUCTOR_FREE ||
+                         destructor == DISPATCH_DATA_DESTRUCTOR_MUNMAP))
+    return REAL(dispatch_data_create)(buffer, size, q, destructor);
+  SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();
+  dispatch_block_t heap_block = Block_copy(destructor);
+  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);
+  return REAL(dispatch_data_create)(buffer, size, q, ^(void) {
+    dispatch_callback_wrap(new_context);
+  });
+}
+
 }  // namespace __tsan
 
 #endif  // SANITIZER_MAC

test/tsan/Darwin/gcd-data.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>
+
+long global;
+
+int main(int argc, const char *argv[]) {
+  fprintf(stderr, "Hello world.\n");
+
+  dispatch_queue_t q = dispatch_queue_create("my.queue", DISPATCH_QUEUE_SERIAL);
+  dispatch_semaphore_t sem = dispatch_semaphore_create(0);
+
+  global = 44;
+  dispatch_data_t data = dispatch_data_create("buffer", 6, q, ^{
+    fprintf(stderr, "Data destructor.\n");
+    global++;
+
+    dispatch_semaphore_signal(sem);
+  });
+  dispatch_release(data);
+  data = nil;
+
+  dispatch_semaphore_wait(sem, DISPATCH_TIME_FOREVER);
+
+  data = dispatch_data_create("buffer", 6, q, DISPATCH_DATA_DESTRUCTOR_DEFAULT);
+  dispatch_release(data);
+  data = nil;
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: Data destructor.
+// CHECK-NOT: WARNING: ThreadSanitizer
+// CHECK: Done.