[asan] Fix stack-use-after-free checks on non-main thread on Fuchsia

[zarvox]

Summary

While some platforms call AsanThread::Init() from the context of the
thread being started, others (like Fuchsia) call AsanThread::Init()
from the context of the thread spawning a child. Since
AsyncSignalSafeLazyInitFakeStack writes to a thread-local, we need to
avoid calling it from the spawning thread on Fuchsia. Skipping the call
here on Fuchsia is fine; it'll get called from the new thread lazily on first
attempted access.

[vitalybuka]

is possible to test this case, even if it's only Fuchsia?

[mcgrathr]

I don't really understand why the whole "lazy" logic exists. I can't tell what case it would ever be used in as the code stands. Every thread will pass through ASanThread::Init before it would ever just call fake_stack() and hit the "lazy" case. Is it intended that one could set __asan_option_detect_stack_use_after_return to false (either from startup or explicitly clear it), then create a thread, then set the flag explicitly on later?

What actually seems preferable here is that we always do the eager allocation, which can happen from any thread, but defer the setting of the TLS cache, which is the only thing that has to happen in the target thread.
So how about if AsyncSignalSafeLazyInitFakeStack calls SetTLSFakeStack either never or only if (tid() == CurrentTid), and then the GetFakeStack() slow path can call SetTLSFakeStack itself?

As to testing, note that we don't have a mechanism to run sanitizer runtime tests on Fuchsia at all so far. The best I can really think of is that just adding a DCHECK_EQ(GetCurrentThread(), this); before calling SetTLSFakeStack would trigger the assertion failure for a spawned thread on Fuchsia before this fix.

[vitalybuka]

In D89607#2350988, @mcgrathr wrote:

I don't really understand why the whole "lazy" logic exists. I can't tell what case it would ever be used in as the code stands. Every thread will pass through ASanThread::Init before it would ever just call fake_stack() and hit the "lazy" case. Is it intended that one could set __asan_option_detect_stack_use_after_return to false (either from startup or explicitly clear it), then create a thread, then set the flag explicitly on later?

What actually seems preferable here is that we always do the eager allocation, which can happen from any thread, but defer the setting of the TLS cache, which is the only thing that has to happen in the target thread.
So how about if AsyncSignalSafeLazyInitFakeStack calls SetTLSFakeStack either never or only if (tid() == CurrentTid), and then the GetFakeStack() slow path can call SetTLSFakeStack itself?

As to testing, note that we don't have a mechanism to run sanitizer runtime tests on Fuchsia at all so far. The best I can really think of is that just adding a DCHECK_EQ(GetCurrentThread(), this); before calling SetTLSFakeStack would trigger the assertion failure for a spawned thread on Fuchsia before this fix.

Maybe https://reviews.llvm.org/D2553

compiler-rt/lib/asan/asan_thread.cpp

237

This looks like very inconvenient difference, it can be easily missed by some making changes for other platforms.
How hard to make fuchsia to run AsanThread::Init on new thread?

[vitalybuka]

In D89607#2350988, @mcgrathr wrote:

As to testing, note that we don't have a mechanism to run sanitizer runtime tests on Fuchsia at all so far. The best I can really think of is that just adding a DCHECK_EQ(GetCurrentThread(), this); before calling SetTLSFakeStack would trigger the assertion failure for a spawned thread on Fuchsia before this fix.

Why is that? Can you run lit on linux and push tests binaries like we do for android?

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

234

can you add comment that the Fuchsia is probably the only known example of platform which needs that?

[zarvox]

Updated comment and added DCHECK

compiler-rt/lib/asan/asan_thread.cpp

234

I suspect RTEMS might too, based on how the #if just below notes that it also doesn't use ThreadStart, but I'm not certain.

I updated the comment to make it clear that Fuchsia is one of the users of this approach.

237

My understanding is that that would be complicated for reasons that Roland can probably speak to better than I can.

compiler-rt/lib/asan/asan_thread.cpp

uptr AsanThread::stack_bottom() {
   return GetStackBounds().bottom;
 }
 
 uptr AsanThread::stack_size() {
   const auto bounds = GetStackBounds();
   return bounds.top - bounds.bottom;
 }
 
-// We want to create the FakeStack lazyly on the first use, but not eralier
+// We want to create the FakeStack lazily on the first use, but not earlier
 // than the stack size is known and the procedure has to be async-signal safe.
 FakeStack *AsanThread::AsyncSignalSafeLazyInitFakeStack() {
   uptr stack_size = this->stack_size();
   if (stack_size == 0)  // stack_size is not yet available, don't use FakeStack.
     return nullptr;
   uptr old_val = 0;
   // fake_stack_ has 3 states:
   // 0   -- not initialized
   // 1   -- being initialized
   // ptr -- initialized
   // This CAS checks if the state was 0 and if so changes it to state 1,
   // if that was successful, it initializes the pointer.
   if (atomic_compare_exchange_strong(
       reinterpret_cast<atomic_uintptr_t *>(&fake_stack_), &old_val, 1UL,
       memory_order_relaxed)) {
     uptr stack_size_log = Log2(RoundUpToPowerOfTwo(stack_size));
     CHECK_LE(flags()->min_uar_stack_size_log, flags()->max_uar_stack_size_log);
     stack_size_log =
         Min(stack_size_log, static_cast<uptr>(flags()->max_uar_stack_size_log));
     stack_size_log =
         Max(stack_size_log, static_cast<uptr>(flags()->min_uar_stack_size_log));
     fake_stack_ = FakeStack::Create(stack_size_log);
+    DCHECK_EQ(GetCurrentThread(), this);
     SetTLSFakeStack(fake_stack_);
     return fake_stack_;
   }
   return nullptr;
 }
 
 void AsanThread::Init(const InitOptions *options) {
   DCHECK_NE(tid(), ThreadRegistry::kUnknownTid);
   next_stack_top_ = next_stack_bottom_ = 0;
   atomic_store(&stack_switching_, false, memory_order_release);
   CHECK_EQ(this->stack_size(), 0U);
   SetThreadStackAndTls(options);
   if (stack_top_ != stack_bottom_) {
     CHECK_GT(this->stack_size(), 0U);
     CHECK(AddrIsInMem(stack_bottom_));
     CHECK(AddrIsInMem(stack_top_ - 1));
   }
   ClearShadowForThreadStackAndTLS();
   fake_stack_ = nullptr;
-  if (__asan_option_detect_stack_use_after_return)
+  if (__asan_option_detect_stack_use_after_return &&
+      tid() == GetCurrentTidOrInvalid()) {
+    // AsyncSignalSafeLazyInitFakeStack makes use of threadlocals and must be
+    // called from the context of the thread it is initializing, not its parent.
+    // Most platforms call AsanThread::Init on the newly-spawned thread, but
+    // Fuchsia calls this function from the parent thread.  To support that
+    // approach, we avoid calling AsyncSignalSafeLazyInitFakeStack here; it will
+    // be called by the new thread when it first attempts to access the fake
+    // stack.
     AsyncSignalSafeLazyInitFakeStack();
+  }
   int local = 0;
   VReport(1, "T%d: stack [%p,%p) size 0x%zx; local=%p\n", tid(),
           (void *)stack_bottom_, (void *)stack_top_, stack_top_ - stack_bottom_,
           &local);
 }
 
 // Fuchsia and RTEMS don't use ThreadStart.
 // asan_fuchsia.c/asan_rtems.c define CreateMainThread and