[msan] Block signals in MsanThread::Init

[vitalybuka]

Summary

If async signal handler called when we MsanThread::Init
signal handler may trigger false reports.
I failed to reproduce this locally for a test.

[eugenis]

What if the signal arrives before MsanThread::Init?
This is what bionic does for hwasan: https://android-review.googlesource.com/c/platform/bionic/+/1134990/

[vitalybuka]

better version

[vitalybuka]

In D113328#3116305, @eugenis wrote:

What if the signal arrives before MsanThread::Init?
This is what bionic does for hwasan: https://android-review.googlesource.com/c/platform/bionic/+/1134990/

thanks, done!

[browneee]

compiler-rt/lib/msan/msan_thread.cpp

25 ↗

(On Diff #385632)

Would the same fix be applicable for DFSan?
https://github.com/llvm/llvm-project/blob/443820179a849eac6ddb5cbf0ba5cc7bbc42e2d2/compiler-rt/lib/dfsan/dfsan_thread.cpp#L21

[eugenis]

compiler-rt/lib/msan/msan_thread.cpp

25 ↗

(On Diff #385632)

I think so. We need to prevent signals from running on the new thread stack and tls before they are ready. Another option would be to always allocate stacks for threads ourselves in the parent thread, but the current approach is simpler.

Not sure about including <signal.h> here. It should go in msan_linux.cpp, or even in sanitizer_common_posix.cpp if it was reused in dfsan.

compiler-rt/lib/msan/msan_thread.h

73

starting_sigset_ or something like that

[vitalybuka]

update

[vitalybuka]

rebase

[vitalybuka]

rebase

[eugenis]

LGTM

compiler-rt/lib/msan/msan_interceptors.cpp

1055

I'd prefer an API where the save location is passed as a construction argument.

ScopedBlockSignals block(&t->starting_sigset_);

Or change the API to explicit block() and release() and move the entire object to MsanThread (call release in the parent and the child explicitly). That way MSanThread does not need to know about __sanitizer_sigset_t.

Anyway, it's just an aesthetic preference. Up to you.

[vitalybuka]

rebase

[thakis]

FYI, this broke running tests in sandboxes processes with msan for us (https://crbug.com/1276113). We're still looking into options, but here's an early heads-up.

(…mostly in case others bisect something to this change.)

[hans]

In D113328#3169901, @thakis wrote:

FYI, this broke running tests in sandboxes processes with msan for us (https://crbug.com/1276113). We're still looking into options, but here's an early heads-up.

(…mostly in case others bisect something to this change.)

Here's a fix: https://reviews.llvm.org/D115057

compiler-rt/lib/msan/msan_interceptors.cpp

 
 extern "C" int pthread_attr_init(void *attr);
 extern "C" int pthread_attr_destroy(void *attr);
 
 static void *MsanThreadStartFunc(void *arg) {
   MsanThread *t = (MsanThread *)arg;
   SetCurrentThread(t);
   t->Init();
+  SetSigProcMask(&t->starting_sigset_, nullptr);
   return t->ThreadStart();
 }
 
 INTERCEPTOR(int, pthread_create, void *th, void *attr, void *(*callback)(void*),
             void * param) {
   ENSURE_MSAN_INITED(); // for GetTlsSize()
   __sanitizer_pthread_attr_t myattr;
   if (!attr) {
     pthread_attr_init(&myattr);
     attr = &myattr;
   }
 
   AdjustStackSize(attr);
 
   MsanThread *t = MsanThread::Create(callback, param);
-
+  ScopedBlockSignals block(&t->starting_sigset_);
   int res = REAL(pthread_create)(th, attr, MsanThreadStartFunc, t);
 
   if (attr == &myattr)
     pthread_attr_destroy(&myattr);
   if (!res) {
     __msan_unpoison(th, __sanitizer::pthread_t_sz);
   }
   return res;
 }
 

compiler-rt/lib/msan/msan_thread.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MSAN_THREAD_H
 #define MSAN_THREAD_H
 
 #include "msan_allocator.h"
 #include "sanitizer_common/sanitizer_common.h"
-
+#include "sanitizer_common/sanitizer_posix.h"
 namespace __msan {
 
 class MsanThread {
  public:
   static MsanThread *Create(thread_callback_t start_routine, void *arg);
   static void TSDDtor(void *tsd);
   void Destroy();
 
public:
   void LeaveSignalHandler() { in_signal_handler_--; }
 
   void StartSwitchFiber(uptr bottom, uptr size);
   void FinishSwitchFiber(uptr *bottom_old, uptr *size_old);
 
   MsanThreadLocalMallocStorage &malloc_storage() { return malloc_storage_; }
 
   int destructor_iterations_;
+  __sanitizer_sigset_t starting_sigset_;
 
  private:
   // NOTE: There is no MsanThread constructor. It is allocated
   // via mmap() and *must* be valid in zero-initialized state.
   void SetThreadStackAndTls();
   void ClearShadowForThreadStackAndTLS();
   struct StackBounds {
     uptr bottom;
     uptr top;
   };
   StackBounds GetStackBounds() const;
   thread_callback_t start_routine_;
   void *arg_;
 
   bool stack_switching_;
 
   StackBounds stack_;
   StackBounds next_stack_;
 
   uptr tls_begin_;
   uptr tls_end_;
 
   unsigned in_signal_handler_;
 
   MsanThreadLocalMallocStorage malloc_storage_;
 };
 
 MsanThread *GetCurrentThread();
 void SetCurrentThread(MsanThread *t);
 
 } // namespace __msan
 
 #endif // MSAN_THREAD_H