[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

26

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

26

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 ↗

(On Diff #385632)

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.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_platform_limits_posix.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();
 
   return thread;
 }
 
 void MsanThread::SetThreadStackAndTls() {
   uptr tls_size = 0;
   uptr stack_size = 0;
   GetThreadStackAndTls(IsMainThread(), &stack_.bottom, &stack_size, &tls_begin_,
                        &tls_size);
   stack_.top = stack_.bottom + stack_size;
   tls_end_ = tls_begin_ + tls_size;
 
   int local;
   CHECK(AddrIsInStack((uptr)&local));
 }
 
 void MsanThread::ClearShadowForThreadStackAndTLS() {
   __msan_unpoison((void *)stack_.bottom, stack_.top - stack_.bottom);
   if (tls_begin_ != tls_end_)
     __msan_unpoison((void *)tls_begin_, tls_end_ - tls_begin_);
   DTLS *dtls = DTLS_Get();
   CHECK_NE(dtls, 0);
   ForEachDVT(dtls, [](const DTLS::DTV &dtv, int id) {
     __msan_unpoison((void *)(dtv.beg), dtv.size);
   });
 }
 
 void MsanThread::Init() {
+  __sanitizer_sigset_t set, old;
+  internal_sigfillset(&set);
+  internal_sigprocmask(SIG_SETMASK, &set, &old);
+
   SetThreadStackAndTls();
   CHECK(MEM_IS_APP(stack_.bottom));
   CHECK(MEM_IS_APP(stack_.top - 1));
   ClearShadowForThreadStackAndTLS();
+
+  internal_sigprocmask(SIG_SETMASK, &old, nullptr);
 }
 
 void MsanThread::TSDDtor(void *tsd) {
   MsanThread *t = (MsanThread*)tsd;
   t->Destroy();
 }
 
 void MsanThread::Destroy() {