[asan] Fix pthread_create interceptor

[vitalybuka]

Summary

AsanThread::Destroy implementation expected to be called on
child thread.

I missed authors concern regarding this reviewing D95184.

[delcypher]

Thanks for catching this. Did this break something somewhere? I didn't see any emails about breaking CI.

I've left some minor comments in the review but overall the approach seems good to me.

compiler-rt/lib/asan/asan_thread.cpp

103

Should we have something like this to document the assumption and catch if it's being violated?

// Must be called on owning thread if `create_failed` is false.
AsanThread::Destroy(bool create_failed) {
...

bool called_by_owning_thread = tid == GetCurrentTidOrInvalid();

...
if (!create_failed) {
  CHECK(called_by_owning_thread);
  ...
}

compiler-rt/test/sanitizer_common/TestCases/Posix/create_thread_fail.cpp

22

I'm guessing you're trying to zero the 16 least significant bits in sz which makes the size 64KiB aligned. Maybe add a comment explaining why? Perhaps you need to do this to make pthread_attr_setstacksize() to succeed?

I presume the idea here is that the requested stack size is supposed to be too large to succeed? If that's the case could you add a comment explaining this?

[vitalybuka]

update

[vitalybuka]

In D95731#2533103, @delcypher wrote:

Thanks for catching this. Did this break something somewhere? I didn't see any emails about breaking CI.

It fails many_threads_detach.cpp on my workstation. Probably bots have higher thread limits.

[delcypher]

compiler-rt/lib/asan/asan_thread.cpp

103

@vitalybuka Do you want to add the checks I suggested to AsanThread::Destroy()? It's fine to not do it I'd just like to understand why.

131

Maybe add a comment here about keeping this in sync with AsanThread::Destroy()?

compiler-rt/lib/asan/asan_thread.h

67 ↗

(On Diff #320413)

How about?

// Destroy this object. Should be called from the corresponding thread when it terminates.
void Destroy();
// Destroy this object. Should be called if the thread fails to start. May be called from any thread
void DestroyFailed();

Perhaps we should pick clearer names. E.g. DestroyOnThreadTermination() and DestroyOnFailedCreate()?

[vitalybuka]

update

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

103

I guess we we have enough info to perform appropriate version cleanup here.

[vitalybuka]

fix typo

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

109

I changing order of UnsetAlternateSignalStack/FinishThread but I don't see a reason for particular order.

[delcypher]

compiler-rt/lib/asan/asan_thread.cpp

102–103

@vitalybuka Should malloc_storage().CommitBack(); be guarded by `was_running too?

I'm not very familiar with this part of ASan's allocator but it looks like malloc_storage() is returning some kind of thread local cache for the quarantine and the primary allocator. I'm guessing that this cache would not be used if was_running is false because we never started the thread and so no allocations could have occurred. That would imply that technically calling malloc_storage().CommitBack(); when was_running is false is unnecessary.

However, for simplicity and to avoid creating an avenue for future bugs calling malloc_storage().CommitBack(); unguarded is probably better provided we are sure it's safe to do this for storage for a thread that was never started. Are we sure it's safe?

109

Seems reasonable. I think the only impact would be if a signal was raised in this function. Previously if a signal was raised in asanThreadRegistry().FinishThread(tid); then I think it would not use the alt signal stack. In the new patch if a signal is raised in asanThreadRegistry().FinishThread(tid) then it will use the alt signal stack.

We should not be raising a signal from asanThreadRegistry().FinishThread(tid); so I think this change should be okay.

[delcypher]

Thanks for working on this. The new approach LGTM apart from calling malloc_storage().CommitBack();. I've left a comment on this. I suspect it's probably okay but I wanted to bring this up before we land this.

[vitalybuka]

update

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

102–103

I think it should be safe either way.
"was_running" is hot case so if we wrong about running it after FinishThread() we should notice that soon.

Alternative to add getter for thread state, but it will require additional lock and so fare it looks unnecessary

[delcypher]

LGTM. Thanks for working on this.

compiler-rt/lib/asan/asan_thread.cpp

void AsanThread::TSDDtor(void *tsd) {
   VReport(1, "T%d TSDDtor\n", context->tid);
   if (context->thread)
     context->thread->Destroy();
 }
 
 void AsanThread::Destroy() {
   int tid = this->tid();
   VReport(1, "T%d exited\n", tid);
 
+  bool was_running =
+      (asanThreadRegistry().FinishThread(tid) == ThreadStatusRunning);
+  if (was_running) {
+    if (AsanThread *thread = GetCurrentThread())
+      CHECK_EQ(this, thread);
     malloc_storage().CommitBack();
-  if (common_flags()->use_sigaltstack) UnsetAlternateSignalStack();
-  asanThreadRegistry().FinishThread(tid);
+    if (common_flags()->use_sigaltstack)
+      UnsetAlternateSignalStack();
     FlushToDeadThreadStats(&stats_);
     // We also clear the shadow on thread destruction because
     // some code may still be executing in later TSD destructors
     // and we don't want it to have any poisoned stack.
     ClearShadowForThreadStackAndTLS();
     DeleteFakeStack(tid);
+  } else {
+    CHECK_NE(this, GetCurrentThread());
+  }
   uptr size = RoundUpTo(sizeof(AsanThread), GetPageSizeCached());
   UnmapOrDie(this, size);
+  if (was_running)
     DTLS_Destroy();
 }
 
 void AsanThread::StartSwitchFiber(FakeStack **fake_stack_save, uptr bottom,
                                   uptr size) {
   if (atomic_load(&stack_switching_, memory_order_relaxed)) {
     Report("ERROR: starting fiber switch while in fiber switch\n");
     Die();
   }
 
   next_stack_bottom_ = bottom;
   next_stack_top_ = bottom + size;
   atomic_store(&stack_switching_, 1, memory_order_release);
 
   FakeStack *current_fake_stack = fake_stack_;
   if (fake_stack_save)
     *fake_stack_save = fake_stack_;

compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h

public:
   ThreadContextBase *FindThreadContextLocked(FindThreadCallback cb,
                                              void *arg);
   ThreadContextBase *FindThreadContextByOsIDLocked(tid_t os_id);
 
   void SetThreadName(u32 tid, const char *name);
   void SetThreadNameByUserId(uptr user_id, const char *name);
   void DetachThread(u32 tid, void *arg);
   void JoinThread(u32 tid, void *arg);
-  void FinishThread(u32 tid);
+  // Finishes thread and returns previous status.
+  ThreadStatus FinishThread(u32 tid);
   void StartThread(u32 tid, tid_t os_id, ThreadType thread_type, void *arg);
   void SetThreadUserId(u32 tid, uptr user_id);
 
  private:
   const ThreadContextFactory context_factory_;
   const u32 max_threads_;
   const u32 thread_quarantine_size_;
   const u32 max_reuse_;

compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp

void ThreadRegistry::JoinThread(u32 tid, void *arg) {
   } while (!destroyed);
 }
 
 // Normally this is called when the thread is about to exit.  If
 // called in ThreadStatusCreated state, then this thread was never
 // really started.  We just did CreateThread for a prospective new
 // thread before trying to create it, and then failed to actually
 // create it, and so never called StartThread.
-void ThreadRegistry::FinishThread(u32 tid) {
+ThreadStatus ThreadRegistry::FinishThread(u32 tid) {
   BlockingMutexLock l(&mtx_);
   CHECK_GT(alive_threads_, 0);
   alive_threads_--;
   CHECK_LT(tid, n_contexts_);
   ThreadContextBase *tctx = threads_[tid];
   CHECK_NE(tctx, 0);
   bool dead = tctx->detached;
+  ThreadStatus prev_status = tctx->status;
   if (tctx->status == ThreadStatusRunning) {
     CHECK_GT(running_threads_, 0);
     running_threads_--;
   } else {
     // The thread never really existed.
     CHECK_EQ(tctx->status, ThreadStatusCreated);
     dead = true;
   }
   tctx->SetFinished();
   if (dead) {
     tctx->SetDead();
     QuarantinePush(tctx);
   }
   tctx->SetDestroyed();
+  return prev_status;
 }
 
 void ThreadRegistry::StartThread(u32 tid, tid_t os_id, ThreadType thread_type,
                                  void *arg) {
   BlockingMutexLock l(&mtx_);
   running_threads_++;
   CHECK_LT(tid, n_contexts_);
   ThreadContextBase *tctx = threads_[tid];

compiler-rt/test/sanitizer_common/TestCases/Posix/create_thread_fail.cpp

+// Sanitizer should not crash if pthread_create fails.
+// RUN: %clangxx -pthread %s -o %t && %run %t
+
+// pthread_create with lsan i386 does not fail here.
+// UNSUPPORTED: i386-linux && lsan
+
+#include <cassert>
+#include <pthread.h>
+#include <stdlib.h>
+
+void *null_func(void *args) {
+  return NULL;
+}
+
+int main(void) {
+  pthread_t thread;
+  pthread_attr_t attrs;
+  pthread_attr_init(&attrs);
+  // Set size huge enough to fail pthread_create.
+  size_t sz = ~0;
+  // Align the size just in case.
+  sz >>= 16;
+  sz <<= 16;
+  int res = pthread_attr_setstacksize(&attrs, sz);
+  assert(res == 0);
+  for (size_t i = 0; i < 10; ++i) {
+    res = pthread_create(&thread, &attrs, null_func, NULL);
+    assert(res != 0);
+  }
+  return 0;
+}