[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

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

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

72

starting_sigset_ or something like that

[vitalybuka]

update

[vitalybuka]

rebase

[vitalybuka]

rebase

[eugenis]

LGTM

compiler-rt/lib/msan/msan_interceptors.cpp

1055 ↗

(On Diff #385688)

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_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();
 
private:
   StackBounds stack_;
   StackBounds next_stack_;
 
   uptr tls_begin_;
   uptr tls_end_;
 
   unsigned in_signal_handler_;
 
+  __sanitizer_sigset_t sigset_;
+
   MsanThreadLocalMallocStorage malloc_storage_;
 };
 
 MsanThread *GetCurrentThread();
 void SetCurrentThread(MsanThread *t);
 
 } // namespace __msan
 
 #endif // MSAN_THREAD_H

compiler-rt/lib/msan/msan_thread.cpp

 
-#include "msan.h"
 #include "msan_thread.h"
-#include "msan_interface_internal.h"
 
+#include <signal.h>
+
+#include "msan.h"
+#include "msan_interface_internal.h"
+#include "sanitizer_common/sanitizer_linux.h"
+#include "sanitizer_common/sanitizer_posix.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 
 namespace __msan {
 
 MsanThread *MsanThread::Create(thread_callback_t start_routine,
                                void *arg) {
   uptr PageSize = GetPageSizeCached();
   uptr size = RoundUpTo(sizeof(MsanThread), PageSize);
   MsanThread *thread = (MsanThread*)MmapOrDie(size, __func__);
   thread->start_routine_ = start_routine;
   thread->arg_ = arg;
   thread->destructor_iterations_ = GetPthreadDestructorIterations();
 
+  __sanitizer_sigset_t set;
+  internal_sigfillset(&set);
+  CHECK_EQ(0, internal_sigprocmask(SIG_SETMASK, &set, &thread->sigset_));
+
   return thread;
 }
 
 void MsanThread::SetThreadStackAndTls() {
   uptr tls_size = 0;
   uptr stack_size = 0;
   GetThreadStackAndTls(IsMainThread(), &stack_.bottom, &stack_size, &tls_begin_,
                        &tls_size);
void MsanThread::ClearShadowForThreadStackAndTLS() {
   });
 }
 
 void MsanThread::Init() {
   SetThreadStackAndTls();
   CHECK(MEM_IS_APP(stack_.bottom));
   CHECK(MEM_IS_APP(stack_.top - 1));
   ClearShadowForThreadStackAndTLS();
+
+  CHECK_EQ(0, internal_sigprocmask(SIG_SETMASK, &sigset_, nullptr));
 }
 
 void MsanThread::TSDDtor(void *tsd) {
   MsanThread *t = (MsanThread*)tsd;
   t->Destroy();
 }
 
 void MsanThread::Destroy() {