[tsan] Intercept __tls_get_addr_earlier

[arichardson]

This can be useful because dlsym() may call malloc on failure which could
result in other interposed functions being called that could eventually
make use of TLS. While the crash that I experienced originally has been
fixed differently (by not using global-dynamic TLS accesses in the mutex
deadlock detector, see #83890),
moving this interception earlier is still a good since it makes the code
a bit more robust against initialization order problems.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Alexander Richardson (arichardson)

Changes

We have to interpose __tls_get_addr before the common interposers. This
is needed because dlsym() may call malloc on failure. And then
the wrapped malloc appears to access thread local storage, thus calling
accesses ___interceptor___tls_get_addr, before REAL(__tls_get_addr) has
ben set so we get a crash inside ___interceptor___tls_get_addr. For
example, this can happen when looking up __isoc23_scanf which might not
exist in some libcs.

I also believe __sanitizer::CheckedMutex::LockImpl() should not be
calling __tls_get_addr but rather use tls_model("initial-exec")?

This fixes a different crash but is related to #46204

Backtrace:

#<!-- -->0  0x0000000000000000 in ?? ()
#<!-- -->1  0x00007ffff6a9d89e in ___interceptor___tls_get_addr (arg=0x7ffff6b27be8) at /path/to/llvm/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:2759
#<!-- -->2  0x00007ffff6a46bc6 in __sanitizer::CheckedMutex::LockImpl (this=0x7ffff6b27be8, pc=140737331846066) at /path/to/llvm/compiler-rt/lib/sanitizer_common/sanitizer_mutex.cpp:218
#<!-- -->3  0x00007ffff6a448b2 in __sanitizer::CheckedMutex::Lock (this=0x7ffff6b27be8, this@<!-- -->entry=0x730000000580) at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_mutex.h:129
#<!-- -->4  __sanitizer::Mutex::Lock (this=0x7ffff6b27be8, this@<!-- -->entry=0x730000000580) at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_mutex.h:167
#<!-- -->5  0x00007ffff6abdbb2 in __sanitizer::GenericScopedLock&lt;__sanitizer::Mutex&gt;::GenericScopedLock (mu=0x730000000580, this=&lt;optimized out&gt;) at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_mutex.h:383
#<!-- -->6  __sanitizer::SizeClassAllocator64&lt;__tsan::AP64&gt;::GetFromAllocator (this=0x7ffff7487dc0 &lt;__tsan::allocator_placeholder&gt;, stat=stat@<!-- -->entry=0x7ffff570db68, class_id=11, chunks=chunks@<!-- -->entry=0x7ffff5702cc8, n_chunks=n_chunks@<!-- -->entry=128)
    at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_allocator_primary64.h:207
#<!-- -->7  0x00007ffff6abdaa0 in __sanitizer::SizeClassAllocator64LocalCache&lt;__sanitizer::SizeClassAllocator64&lt;__tsan::AP64&gt; &gt;::Refill (this=&lt;optimized out&gt;, c=c@<!-- -->entry=0x7ffff5702cb8, allocator=&lt;optimized out&gt;, class_id=&lt;optimized out&gt;)
    at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_allocator_local_cache.h:103
#<!-- -->8  0x00007ffff6abd731 in __sanitizer::SizeClassAllocator64LocalCache&lt;__sanitizer::SizeClassAllocator64&lt;__tsan::AP64&gt; &gt;::Allocate (this=0x7ffff6b27be8, allocator=0x7ffff5702cc8, class_id=140737311157448)
    at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_allocator_local_cache.h:39
#<!-- -->9  0x00007ffff6abc397 in __sanitizer::CombinedAllocator&lt;__sanitizer::SizeClassAllocator64&lt;__tsan::AP64&gt;, __sanitizer::LargeMmapAllocatorPtrArrayDynamic&gt;::Allocate (this=0x7ffff5702cc8, cache=0x7ffff6b27be8, size=&lt;optimized out&gt;, size@<!-- -->entry=175, alignment=alignment@<!-- -->entry=16)
    at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_allocator_combined.h:69
#<!-- -->10 0x00007ffff6abaa6a in __tsan::user_alloc_internal (thr=0x7ffff7ebd980, pc=140737331499943, sz=sz@<!-- -->entry=175, align=align@<!-- -->entry=16, signal=true) at /path/to/llvm/compiler-rt/lib/tsan/rtl/tsan_mman.cpp:198
#<!-- -->11 0x00007ffff6abb0d1 in __tsan::user_alloc (thr=0x7ffff6b27be8, pc=140737331846066, sz=11, sz@<!-- -->entry=175) at /path/to/llvm/compiler-rt/lib/tsan/rtl/tsan_mman.cpp:223
#<!-- -->12 0x00007ffff6a693b5 in ___interceptor_malloc (size=175) at /path/to/llvm/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:666
#<!-- -->13 0x00007ffff7fce7f2 in malloc (size=175) at ../include/rtld-malloc.h:56
#<!-- -->14 __GI__dl_exception_create_format (exception=exception@<!-- -->entry=0x7fffffffd0d0, objname=0x7ffff7fc3550 "/path/to/llvm/compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.tsan-x86_64.so",
    fmt=fmt@<!-- -->entry=0x7ffff7ff2db9 "undefined symbol: %s%s%s") at ./elf/dl-exception.c:157
#<!-- -->15 0x00007ffff7fd50e8 in _dl_lookup_symbol_x (undef_name=0x7ffff6af868b "__isoc23_scanf", undef_map=&lt;optimized out&gt;, ref=0x7fffffffd148, symbol_scope=&lt;optimized out&gt;, version=&lt;optimized out&gt;, type_class=0, flags=2, skip_map=0x7ffff7fc35e0) at ./elf/dl-lookup.c:793
--Type &lt;RET&gt; for more, q to quit, c to continue without paging--
#<!-- -->16 0x00007ffff656d6ed in do_sym (handle=&lt;optimized out&gt;, name=0x7ffff6af868b "__isoc23_scanf", who=0x7ffff6a3bb84 &lt;__interception::InterceptFunction(char const*, unsigned long*, unsigned long, unsigned long)+36&gt;, vers=vers@<!-- -->entry=0x0, flags=flags@<!-- -->entry=2) at ./elf/dl-sym.c:146
#<!-- -->17 0x00007ffff656d9dd in _dl_sym (handle=&lt;optimized out&gt;, name=&lt;optimized out&gt;, who=&lt;optimized out&gt;) at ./elf/dl-sym.c:195
#<!-- -->18 0x00007ffff64a2854 in dlsym_doit (a=a@<!-- -->entry=0x7fffffffd3b0) at ./dlfcn/dlsym.c:40
#<!-- -->19 0x00007ffff7fcc489 in __GI__dl_catch_exception (exception=exception@<!-- -->entry=0x7fffffffd310, operate=0x7ffff64a2840 &lt;dlsym_doit&gt;, args=0x7fffffffd3b0) at ./elf/dl-catch.c:237
#<!-- -->20 0x00007ffff7fcc5af in _dl_catch_error (objname=0x7fffffffd368, errstring=0x7fffffffd370, mallocedp=0x7fffffffd367, operate=&lt;optimized out&gt;, args=&lt;optimized out&gt;) at ./elf/dl-catch.c:256
#<!-- -->21 0x00007ffff64a2257 in _dlerror_run (operate=operate@<!-- -->entry=0x7ffff64a2840 &lt;dlsym_doit&gt;, args=args@<!-- -->entry=0x7fffffffd3b0) at ./dlfcn/dlerror.c:138
#<!-- -->22 0x00007ffff64a28e5 in dlsym_implementation (dl_caller=&lt;optimized out&gt;, name=&lt;optimized out&gt;, handle=&lt;optimized out&gt;) at ./dlfcn/dlsym.c:54
#<!-- -->23 ___dlsym (handle=&lt;optimized out&gt;, name=&lt;optimized out&gt;) at ./dlfcn/dlsym.c:68
#<!-- -->24 0x00007ffff6a3bb84 in __interception::GetFuncAddr (name=0x7ffff6af868b "__isoc23_scanf", trampoline=140737311157448) at /path/to/llvm/compiler-rt/lib/interception/interception_linux.cpp:42
#<!-- -->25 __interception::InterceptFunction (name=0x7ffff6af868b "__isoc23_scanf", ptr_to_real=0x7ffff74850e8 &lt;__interception::real___isoc23_scanf&gt;, func=11, trampoline=140737311157448)
    at /path/to/llvm/compiler-rt/lib/interception/interception_linux.cpp:61
#<!-- -->26 0x00007ffff6a9f2d9 in InitializeCommonInterceptors () at /path/to/llvm/compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_common_interceptors.inc:10315

---

Full diff: https://github.com/llvm/llvm-project/pull/83886.diff

1 Files Affected:

• (modified) compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp (+14-9)

diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index a9f6673ac44e90..b8bfaf500c2769 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -2863,6 +2863,20 @@ void InitializeInterceptors() {
 
   new(interceptor_ctx()) InterceptorContext();
 
+  // Interpose __tls_get_addr before the common interposers. This is needed
+  // because dlsym() may call malloc on failure. And then ___interceptor_malloc
+  // can end up calling ___interceptor___tls_get_addr, which would not be
+  // initialized yet and therefore results in a crash when calling
+  // REAL(__tls_get_addr). For example, this can happen when looking up
+  // __isoc23_scanf which might not exist in some libcs.
+#ifdef NEED_TLS_GET_ADDR
+#  if !SANITIZER_S390
+  TSAN_INTERCEPT(__tls_get_addr);
+#  else
+  TSAN_INTERCEPT(__tls_get_addr_internal);
+  TSAN_INTERCEPT(__tls_get_offset);
+#  endif
+#endif
   InitializeCommonInterceptors();
   InitializeSignalInterceptors();
   InitializeLibdispatchInterceptors();
@@ -3010,15 +3024,6 @@ void InitializeInterceptors() {
   TSAN_INTERCEPT(__cxa_atexit);
   TSAN_INTERCEPT(_exit);
 
-#ifdef NEED_TLS_GET_ADDR
-#if !SANITIZER_S390
-  TSAN_INTERCEPT(__tls_get_addr);
-#else
-  TSAN_INTERCEPT(__tls_get_addr_internal);
-  TSAN_INTERCEPT(__tls_get_offset);
-#endif
-#endif
-
   TSAN_MAYBE_INTERCEPT__LWP_EXIT;
   TSAN_MAYBE_INTERCEPT_THR_EXIT;
 

[arichardson]

I think #83890 is the better approach, but since I'm not sure I've uploaded both versions.

[vitalybuka]

I guess we can land both

[MaskRay]

Commented on #83890. Yes, 83890 looks better than this one.

[vitalybuka]

Commented on #83890. Yes, 83890 looks better than this one.

Yes, but initializing these low level interceptors earlier also looks like improvement