tsan: fix false positives in dynamic libs with static tls

[dvyukov]

Summary

The added test demonstrates loading a dynamic library with static TLS.
Such static TLS is a hack that allows a dynamic library to have faster TLS,
but it can be loaded only iff all threads happened to allocate some excess
of static TLS space for whatever reason. If it's not the case loading fails with:

dlopen: cannot load any more object with static TLS

We used to produce a false positive because dlopen will write into TLS
of all existing threads to initialize/zero TLS region for the loaded library.
And this appears to be racing with initialization of TLS in the thread
since we model a write into the whole static TLS region (we don't what part
of it is currently unused):

WARNING: ThreadSanitizer: data race (pid=2317365) Write of size 1 at 0x7f1fa9bfcdd7 by main thread: 0 memset 1 init_one_static_tls 2 pthread_init_static_tls [[ this is where main calls dlopen ]] 3 main Previous write of size 8 at 0x7f1fa9bfcdd0 by thread T1: 0 tsan_tls_initialization

Fix this by ignoring accesses during dlopen.

[melver]

compiler-rt/test/tsan/Linux/dlopen_static_tls.cpp

13

"we don't know what.."

But on a whole this makes sense.
Is the reason for not reporting this hack that it's pretty intentional and whoever wants to do this knows the risks?

[dvyukov]

compiler-rt/test/tsan/Linux/dlopen_static_tls.cpp

13

Is the reason for not reporting this hack that it's pretty intentional and whoever wants to do this knows the risks?

It is synchronized inside of the dynamic linker (hopefully), we just don't see that synchronization.
Plus we imitate write to the whole TLS range on thread start in __tsan_tls_initialization, which is formally not happening.
Overall what this test is doing legal as far as I understand.

[dvyukov]

add missing word in the comment

[dvyukov]

compiler-rt/test/tsan/Linux/dlopen_static_tls.cpp

13

"we don't know what.."

Done

compiler-rt/lib/asan/asan_interceptors.cpp

 // and remembers all ever existed threads, so the linear search by UserId
 // can be slow.
 #define COMMON_INTERCEPTOR_SET_PTHREAD_NAME(ctx, thread, name) \
   do {                                                         \
   } while (false)
 #define COMMON_INTERCEPTOR_BLOCK_REAL(name) REAL(name)
 // Strict init-order checking is dlopen-hostile:
 // https://github.com/google/sanitizers/issues/178
-#define COMMON_INTERCEPTOR_ON_DLOPEN(filename, flag)                           \
-  do {                                                                         \
-    if (flags()->strict_init_order)                                            \
-      StopInitOrderChecking();                                                 \
-    CheckNoDeepBind(filename, flag);                                           \
-  } while (false)
-#define COMMON_INTERCEPTOR_ON_EXIT(ctx) OnExit()
-#define COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, handle)
-#define COMMON_INTERCEPTOR_LIBRARY_UNLOADED()
-#define COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED (!asan_inited)
-#define COMMON_INTERCEPTOR_GET_TLS_RANGE(begin, end)                           \
-  if (AsanThread *t = GetCurrentThread()) {                                    \
-    *begin = t->tls_begin();                                                   \
-    *end = t->tls_end();                                                       \
-  } else {                                                                     \
-    *begin = *end = 0;                                                         \
+#  define COMMON_INTERCEPTOR_DLOPEN(filename, flag) \
+    ({                                              \
+      if (flags()->strict_init_order)               \
+        StopInitOrderChecking();                    \
+      CheckNoDeepBind(filename, flag);              \
+      REAL(dlopen)(filename, flag);                 \
+    })
+#  define COMMON_INTERCEPTOR_ON_EXIT(ctx) OnExit()
+#  define COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, handle)
+#  define COMMON_INTERCEPTOR_LIBRARY_UNLOADED()
+#  define COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED (!asan_inited)
+#  define COMMON_INTERCEPTOR_GET_TLS_RANGE(begin, end) \
+    if (AsanThread *t = GetCurrentThread()) {          \
+      *begin = t->tls_begin();                         \
+      *end = t->tls_end();                             \
+    } else {                                           \
+      *begin = *end = 0;                               \
     }
 
 #define COMMON_INTERCEPTOR_MEMMOVE_IMPL(ctx, to, from, size) \
   do {                                                       \
     ASAN_INTERCEPTOR_ENTER(ctx, memmove);                    \
     ASAN_MEMMOVE_IMPL(ctx, to, from, size);                  \
   } while (false)
 
 #define COMMON_INTERCEPTOR_MEMCPY_IMPL(ctx, to, from, size) \

compiler-rt/lib/memprof/memprof_interceptors.cpp

 // Should be memprofThreadRegistry().SetThreadNameByUserId(thread, name)
 // But memprof does not remember UserId's for threads (pthread_t);
 // and remembers all ever existed threads, so the linear search by UserId
 // can be slow.
 #define COMMON_INTERCEPTOR_SET_PTHREAD_NAME(ctx, thread, name)                 \
   do {                                                                         \
   } while (false)
 #define COMMON_INTERCEPTOR_BLOCK_REAL(name) REAL(name)
-#define COMMON_INTERCEPTOR_ON_DLOPEN(filename, flag)                           \
-  do {                                                                         \
-    CheckNoDeepBind(filename, flag);                                           \
-  } while (false)
 #define COMMON_INTERCEPTOR_ON_EXIT(ctx) OnExit()
 #define COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, handle)
 #define COMMON_INTERCEPTOR_LIBRARY_UNLOADED()
 #define COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED (!memprof_inited)
 #define COMMON_INTERCEPTOR_GET_TLS_RANGE(begin, end)                           \
   if (MemprofThread *t = GetCurrentThread()) {                                 \
     *begin = t->tls_begin();                                                   \
     *end = t->tls_end();                                                       \

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 //   COMMON_INTERCEPTOR_READ_RANGE
 //   COMMON_INTERCEPTOR_WRITE_RANGE
 //   COMMON_INTERCEPTOR_INITIALIZE_RANGE
 //   COMMON_INTERCEPTOR_DIR_ACQUIRE
 //   COMMON_INTERCEPTOR_FD_ACQUIRE
 //   COMMON_INTERCEPTOR_FD_RELEASE
 //   COMMON_INTERCEPTOR_FD_ACCESS
 //   COMMON_INTERCEPTOR_SET_THREAD_NAME
-//   COMMON_INTERCEPTOR_ON_DLOPEN
+//   COMMON_INTERCEPTOR_DLOPEN
 //   COMMON_INTERCEPTOR_ON_EXIT
 //   COMMON_INTERCEPTOR_MUTEX_PRE_LOCK
 //   COMMON_INTERCEPTOR_MUTEX_POST_LOCK
 //   COMMON_INTERCEPTOR_MUTEX_UNLOCK
 //   COMMON_INTERCEPTOR_MUTEX_REPAIR
 //   COMMON_INTERCEPTOR_SET_PTHREAD_NAME
 //   COMMON_INTERCEPTOR_HANDLE_RECVMSG
 //   COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED

 #ifndef COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED
 #define COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED (0)
 #endif
 
 #define COMMON_INTERCEPTOR_READ_STRING(ctx, s, n)                   \
     COMMON_INTERCEPTOR_READ_RANGE((ctx), (s),                       \
       common_flags()->strict_string_checks ? (internal_strlen(s)) + 1 : (n) )
 
-#ifndef COMMON_INTERCEPTOR_ON_DLOPEN
-#define COMMON_INTERCEPTOR_ON_DLOPEN(filename, flag) \
-  CheckNoDeepBind(filename, flag);
+#ifndef COMMON_INTERCEPTOR_DLOPEN
+#define COMMON_INTERCEPTOR_DLOPEN(filename, flag) \
+  ({ CheckNoDeepBind(filename, flag); REAL(dlopen)(filename, flag); })
 #endif
 
 #ifndef COMMON_INTERCEPTOR_GET_TLS_RANGE
 #define COMMON_INTERCEPTOR_GET_TLS_RANGE(begin, end) *begin = *end = 0;
 #endif
 
 #ifndef COMMON_INTERCEPTOR_ACQUIRE
 #define COMMON_INTERCEPTOR_ACQUIRE(ctx, u) {}

 #define INIT_FCLOSE
 #endif
 
 #if SANITIZER_INTERCEPT_DLOPEN_DLCLOSE
 INTERCEPTOR(void*, dlopen, const char *filename, int flag) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER_NOIGNORE(ctx, dlopen, filename, flag);
   if (filename) COMMON_INTERCEPTOR_READ_STRING(ctx, filename, 0);
-  COMMON_INTERCEPTOR_ON_DLOPEN(filename, flag);
-  void *res = REAL(dlopen)(filename, flag);
+  void *res = COMMON_INTERCEPTOR_DLOPEN(filename, flag);
   Symbolizer::GetOrInit()->InvalidateModuleList();
   COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, res);
   return res;
 }
 
 INTERCEPTOR(int, dlclose, void *handle) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER_NOIGNORE(ctx, dlclose, handle);

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

#define COMMON_INTERCEPTOR_FILE_OPEN(ctx, file, path) \
   }
 
 #define COMMON_INTERCEPTOR_FILE_CLOSE(ctx, file) \
   if (file) {                                    \
     int fd = fileno_unlocked(file);              \
     if (fd >= 0) FdClose(thr, pc, fd);           \
   }
 
+#define COMMON_INTERCEPTOR_DLOPEN(filename, flag) \
+  ({                                              \
+    CheckNoDeepBind(filename, flag);              \
+    ThreadIgnoreBegin(thr, 0);                    \
+    void *res = REAL(dlopen)(filename, flag);     \
+    ThreadIgnoreEnd(thr);                         \
+    res;                                          \
+  })
+
 #define COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, handle) \
   libignore()->OnLibraryLoaded(filename)
 
 #define COMMON_INTERCEPTOR_LIBRARY_UNLOADED() \
   libignore()->OnLibraryUnloaded()
 
 #define COMMON_INTERCEPTOR_ACQUIRE(ctx, u) \
   Acquire(((TsanInterceptorContext *) ctx)->thr, pc, u)

compiler-rt/test/tsan/Linux/dlopen_static_tls.cpp

+// RUN: %clangxx_tsan -O1 %s -DBUILD_SO -fPIC -shared -o %t-so.so
+// RUN: %clangxx_tsan -O1 %s %link_libcxx_tsan -o %t && %run %t 2>&1 | FileCheck %s
+
+// A test for loading a dynamic library with static TLS.
+// Such static TLS is a hack that allows a dynamic library to have faster TLS,
+// but it can be loaded only iff all threads happened to allocate some excess
+// of static TLS space for whatever reason. If it's not the case loading fails with:
+// dlopen: cannot load any more object with static TLS
+// We used to produce a false positive because dlopen will write into TLS
+// of all existing threads to initialize/zero TLS region for the loaded library.
+// And this appears to be racing with initialization of TLS in the thread
+// since we model a write into the whole static TLS region (we don't know what part
+// of it is currently unused):
+// WARNING: ThreadSanitizer: data race (pid=2317365)
+//   Write of size 1 at 0x7f1fa9bfcdd7 by main thread:
+//     #0 memset
+//     #1 init_one_static_tls
+//     #2 __pthread_init_static_tls
+//     [[ this is where main calls dlopen ]]
+//     #3 main
+//   Previous write of size 8 at 0x7f1fa9bfcdd0 by thread T1:
+//     #0 __tsan_tls_initialization
+
+#ifdef BUILD_SO
+
+__attribute__((tls_model("initial-exec"))) __thread char x = 42;
+__attribute__((tls_model("initial-exec"))) __thread char y;
+
+extern "C" int sofunc() { return ++x + ++y; }
+
+#else // BUILD_SO
+
+#  include "../test.h"
+#  include <dlfcn.h>
+#  include <string>
+
+__thread int x[1023];
+
+void *lib;
+void (*func)();
+int ready;
+
+void *thread(void *arg) {
+  barrier_wait(&barrier);
+  if (__atomic_load_n(&ready, __ATOMIC_ACQUIRE))
+    func();
+  if (dlclose(lib)) {
+    printf("error in dlclose: %s\n", dlerror());
+    exit(1);
+  }
+  return 0;
+}
+
+int main(int argc, char *argv[]) {
+  barrier_init(&barrier, 2);
+  pthread_t th;
+  pthread_create(&th, 0, thread, 0);
+  lib = dlopen((std::string(argv[0]) + "-so.so").c_str(), RTLD_NOW);
+  if (lib == 0) {
+    printf("error in dlopen: %s\n", dlerror());
+    return 1;
+  }
+  func = (void (*)())dlsym(lib, "sofunc");
+  if (func == 0) {
+    printf("error in dlsym: %s\n", dlerror());
+    return 1;
+  }
+  __atomic_store_n(&ready, 1, __ATOMIC_RELEASE);
+  barrier_wait(&barrier);
+  func();
+  pthread_join(th, 0);
+  fprintf(stderr, "DONE\n");
+  return 0;
+}
+
+#endif // BUILD_SO
+
+// CHECK: DONE