[sanitizer] Avoid calling a nullptr in MonotonicNanoTime if interceptors are not yet initialized

[kubamracek]

Summary

I'm seeing a TSan startup crash on Linux when used in Swift programs, where MonotonicNanoTime will try to call real_clock_gettime and then jump to NULL because interceptors are not yet initialized. Attaching the backtrace of how that happens below. This is on Ubuntu 18.04. Looks like TSan's main Initialize() function is called at a point where __progname is already set, but interceptors aren't yet set up.

* thread #1, name = 'hello', stop reason = signal SIGSEGV: invalid address 
  * frame #0: 0x0000000000000000
    frame #1: 0x00005555555dbf0a ::real_clock_gettime
    frame #2: 0x00005555555b14f0 __sanitizer::MonotonicNanoTime
    frame #3: 0x000055555562d430 __sanitizer::SizeClassAllocator64<...>::PopulateFreeArray
    frame #4: 0x000055555562d161 __sanitizer::SizeClassAllocator64<...>::GetFromAllocator
    frame #5: 0x000055555562cf5d __sanitizer::SizeClassAllocator64LocalCache<...>::Refill
    frame #6: 0x000055555562c845 __sanitizer::SizeClassAllocator64LocalCache<...>::Allocate
    frame #7: 0x0000555555629e75 __sanitizer::CombinedAllocator<...>::Allocate
    frame #8: 0x0000555555628c02 __tsan::user_alloc_internal
    frame #9: 0x0000555555629168 __tsan::user_calloc
    frame #10: 0x00005555555c7a78 ::__interceptor_calloc
    frame #11: 0x00007ffff6ded7e5 libdl.so.2`___lldb_unnamed_symbol11$$libdl.so.2 + 277
    frame #12: 0x00007ffff6ded166 libdl.so.2`dlsym + 118
    frame #13: 0x00005555556317b8 __interception::GetRealFunctionAddress
    frame #14: 0x0000555555601c7c InitializeCommonInterceptors
    frame #15: 0x0000555555600b6a __tsan::InitializeInterceptors
    frame #16: 0x000055555555cc71 __tsan::Initialize
    frame #17: 0x00005555555c6340 __tsan::ScopedInterceptor::ScopedInterceptor
    frame #18: 0x00005555555c6f18 ::__interceptor___cxa_atexit
    frame #19: 0x00007ffff6af22e6 libstdc++.so.6`___lldb_unnamed_symbol241$$libstdc++.so.6 + 70
    frame #20: 0x00007ffff7de5733 ld-linux-x86-64.so.2`___lldb_unnamed_symbol50$$ld-linux-x86-64.so.2 + 259
    frame #21: 0x00007ffff7dd60ca ld-linux-x86-64.so.2`___lldb_unnamed_symbol4$$ld-linux-x86-64.so.2 + 105

[cryptoad]

There is a check for real_clock_gettime (as an extern "C", not part of the interception namespace) in MonotonicNanoTime, as defined by sanitizer_common_interceptors.inc, can this check be moved there? eg: if real_clock_gettime exists & the interception function is not null?

[kubamracek]

In D53528#1272580, @cryptoad wrote:

There is a check for real_clock_gettime (as an extern "C", not part of the interception namespace) in MonotonicNanoTime, as defined by sanitizer_common_interceptors.inc, can this check be moved there? eg: if real_clock_gettime exists & the interception function is not null?

Yes, but it would need to be on the if (CanUseVDSO) line otherwise we still get a crash. I'll update the patch.

[cryptoad]

In D53528#1272587, @kubamracek wrote:

Yes, but it would need to be on the if (CanUseVDSO) line otherwise we still get a crash. I'll update the patch.

My thought is that if extern "C" real_clock_gettime doesn't exist, it means that the function clock_gettime is not intercepted, hence we don't need to check for REAL(clock_gettime) eg __interception::real_clock_gettime to be not null.
Basically that would save us a memory read in case of non-interception. But I might be wrong.

[kubamracek]

The problem I'm seeing is when real_clock_gettime does exist, but it's not yet initialized, see the backtrace posted earlier. I'm not sure how performance sensitive this area is: Is it used elsewhere other than the memory allocator?

[cryptoad]

In D53528#1272592, @kubamracek wrote:

The problem I'm seeing is when real_clock_gettime does exist, but it's not yet initialized, see the backtrace posted earlier. I'm not sure how performance sensitive this area is: Is it used elsewhere other than the memory allocator?

MonotonicNanoTime is hot for Scudo (which is performance sensitive), which is the reason for the ugly vDSO check. But yeah that should work.

[kubamracek]

So, it's okay to land this?

[cryptoad]

In D53528#1272945, @kubamracek wrote:

So, it's okay to land this?

Yup sorry for the delay!

lib/sanitizer_common/sanitizer_linux_libcdep.cc

if (SANITIZER_ANDROID)
     return true;
   if (&__progname && __progname && *__progname)
     return true;
   return false;
 }
 
 // MonotonicNanoTime is a timing function that can leverage the vDSO by calling
 // clock_gettime. real_clock_gettime only exists if clock_gettime is
-// intercepted, so define it weakly and use it if available.
+// intercepted, so define it weakly and use it if available. MonotonicNanoTime
+// might also be called when interceptors are not yet initialized, so check for
+// that as well.
 extern "C" SANITIZER_WEAK_ATTRIBUTE
 int real_clock_gettime(u32 clk_id, void *tp);
+namespace __interception { int (*real_clock_gettime)(u32 clk_id, void *tp); }
 u64 MonotonicNanoTime() {
   timespec ts;
-  if (CanUseVDSO()) {
+  if (CanUseVDSO() && __interception::real_clock_gettime) {
     if (&real_clock_gettime)
       real_clock_gettime(CLOCK_MONOTONIC, &ts);
     else
       clock_gettime(CLOCK_MONOTONIC, &ts);
   } else {
     internal_clock_gettime(CLOCK_MONOTONIC, &ts);
   }
   return (u64)ts.tv_sec * (1000ULL * 1000 * 1000) + ts.tv_nsec;