[tsan] Fix crash in objc_sync_enter/objc_sync_exit when using an Obj-C tagged pointer

[kubamracek]

Summary

Objective-C tagged pointers (either bottom-most or top-most bit is 1) are valid Obj-C objects but are not valid pointers. Make sure we don't crash on them when used in objc_sync_enter/objc_sync_exit. Instead, let's synchronize on a global object.

[george.karpenkov]

lib/tsan/rtl/tsan_interceptors_mac.cc

297 ↗

(On Diff #156925)

can we have a short docstring for both functions? Also, isTaggedObjCPointer?

299 ↗

(On Diff #156925)

return ((uptr)obj & kPossibleTaggedBits) != 0 ?

303 ↗

(On Diff #156925)

what's the point of taking in the argument then?

312 ↗

(On Diff #156925)

can we extract this line into a function with a name explaining it's semantics?

[kubamracek]

lib/tsan/rtl/tsan_interceptors_mac.cc

303 ↗

(On Diff #156925)

The point is that, ideally, the function would return a different sync address for different pointers. But for now, the implementation trivially returns the same address for all, which can cause false negatives.

[kubamracek]

lib/tsan/rtl/tsan_interceptors_mac.cc

297 ↗

(On Diff #156925)

Also, isTaggedObjCPointer?

In (almost) all of TSan, private functions starts with an uppercase letter. Let's not try to change style with this patch.

[george.karpenkov]

lib/tsan/rtl/tsan_interceptors_mac.cc

299 ↗

(On Diff #156932)

also all the helper functions should be static

[kubamracek]

add static

[delcypher]

Other than minor suggestions the basic idea seems okay for now.

lib/tsan/rtl/tsan_interceptors_mac.cc

299 ↗

(On Diff #156937)

Perhaps the name should be IsTaggedObjCPointer?

308 ↗

(On Diff #156937)

There probably should be a TODO(kubamracek) or FIXME in here.

Do we even want to do Acquire() and Release() for tagged pointers? Even if we had a perfect implementation of SyncAddressForTaggedPointer() that still seems wrong because we could have two independent instances of a tagged pointer
(e.g. create two separate NSString with same value) and those shouldn't really be considered as the same object, but they would
be considered the same because their tagged pointer value is the same.

[kubamracek]

lib/tsan/rtl/tsan_interceptors_mac.cc

308 ↗

(On Diff #156937)

Yes, the entire idea of using value types (strings, integeres, timestamps) as synchronization objects is weird and most likely not what you intended anyway. Therefore this patch is really just to avoid crashing and to mirror what libobjc is actually doing when using @synchronize on a tagged pointer (it does actually establish synchronization if the pointer bits are exactly the same).

[kubamracek]

Thanks for the fast review!

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

 TSAN_INTERCEPTOR(void, xpc_connection_cancel, xpc_connection_t connection) {
   SCOPED_TSAN_INTERCEPTOR(xpc_connection_cancel, connection);
   Release(thr, pc, (uptr)connection);
   REAL(xpc_connection_cancel)(connection);
 }
 
 #endif  // #if defined(__has_include) && __has_include(<xpc/xpc.h>)
 
+// Is the Obj-C object a tagged pointer (i.e. isn't really a valid pointer and
+// contains data in the pointers bits instead)?
+static bool IsTaggedObjCPointer(void *obj) {
+  const uptr kPossibleTaggedBits = 0x8000000000000001ull;
+  return ((uptr)obj & kPossibleTaggedBits) != 0;
+}
+
+// Return an address on which we can synchronize (Acquire and Release) for a
+// Obj-C tagged pointer (which is not a valid pointer). Ideally should be a
+// derived address from 'obj', but for now just return the same global address.
+// TODO(kubamracek): Return different address for different pointers.
+static uptr SyncAddressForTaggedPointer(void *obj) {
+  (void)obj;
+  static u64 addr;
+  return (uptr)&addr;
+}
+
+// Address on which we can synchronize for an Objective-C object. Supports
+// tagged pointers.
+static uptr SyncAddressForObjCObject(void *obj) {
+  if (IsTaggedObjCPointer(obj)) return SyncAddressForTaggedPointer(obj);
+  return (uptr)obj;
+}
+
 TSAN_INTERCEPTOR(int, objc_sync_enter, void *obj) {
   SCOPED_TSAN_INTERCEPTOR(objc_sync_enter, obj);
   int result = REAL(objc_sync_enter)(obj);
-  if (obj) Acquire(thr, pc, (uptr)obj);
+  if (obj) Acquire(thr, pc, SyncAddressForObjCObject(obj));
   return result;
 }
 
 TSAN_INTERCEPTOR(int, objc_sync_exit, void *obj) {
   SCOPED_TSAN_INTERCEPTOR(objc_sync_enter, obj);
-  if (obj) Release(thr, pc, (uptr)obj);
+  if (obj) Release(thr, pc, SyncAddressForObjCObject(obj));
   return REAL(objc_sync_exit)(obj);
 }
 
 // On macOS, libc++ is always linked dynamically, so intercepting works the
 // usual way.
 #define STDCXX_INTERCEPTOR TSAN_INTERCEPTOR
 
 namespace {

compiler-rt/trunk/test/tsan/Darwin/objc-synchronize-tagged.mm

+// RUN: %clangxx_tsan %s -o %t -framework Foundation -fobjc-arc %darwin_min_target_with_full_runtime_arc_support
+// RUN: %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+NSString *tagged_string = nil;
+
+@interface MyClass : NSObject {
+  long field;
+}
+@property(nonatomic, readonly) long value;
+@end
+
+dispatch_group_t group;
+
+@implementation MyClass
+
+- (void)start {
+  dispatch_queue_t q = dispatch_queue_create(NULL, NULL);
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    for (int i = 0; i < 10; i++) {
+      dispatch_async(q, ^{
+        @synchronized(tagged_string) {
+          self->field = i;
+        }
+      });
+    }
+  });
+}
+
+- (long)value {
+  @synchronized(tagged_string) {
+    return self->field;
+  }
+}
+
+- (void)dealloc {
+  dispatch_group_leave(group);
+}
+
+@end
+
+int main() {
+  tagged_string = [NSString stringWithFormat:@"%s", "abc"];
+  uintptr_t tagged_string_bits = (uintptr_t)tagged_string;
+  assert((tagged_string_bits & 0x8000000000000001ull) != 0);
+  group = dispatch_group_create();
+  @autoreleasepool {
+    for (int j = 0; j < 100; ++j) {
+      dispatch_group_enter(group);
+      MyClass *obj = [[MyClass alloc] init];
+      [obj start];
+      long x = obj.value;
+      (void)x;
+    }
+  }
+  dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+  NSLog(@"Hello world");
+}
+
+// CHECK: Hello world
+// CHECK-NOT: WARNING: ThreadSanitizer