diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -81,6 +81,8 @@ extern "C" int pthread_attr_destroy(void *attr); DECLARE_REAL(int, pthread_attr_getdetachstate, void *, void *) extern "C" int pthread_attr_setstacksize(void *attr, uptr stacksize); +extern "C" int pthread_atfork(void (*prepare)(void), void (*parent)(void), + void (*child)(void)); extern "C" int pthread_key_create(unsigned *key, void (*destructor)(void* v)); extern "C" int pthread_setspecific(unsigned key, const void *v); DECLARE_REAL(int, pthread_mutexattr_gettype, void *, void *) @@ -1976,7 +1978,8 @@ // because in async signal processing case (when handler is called directly // from rtl_generic_sighandler) we have not yet received the reraised // signal; and it looks too fragile to intercept all ways to reraise a signal. - if (flags()->report_bugs && !sync && sig != SIGTERM && errno != 99) { + if (ShouldReport(thr, ReportTypeErrnoInSignal) && !sync && sig != SIGTERM && + errno != 99) { VarSizeStackTrace stack; // StackTrace::GetNestInstructionPc(pc) is used because return address is // expected, OutputReport() will undo this. @@ -2146,26 +2149,32 @@ if (in_symbolizer()) return REAL(fork)(fake); SCOPED_INTERCEPTOR_RAW(fork, fake); + return REAL(fork)(fake); +} + +void atfork_prepare() { + if (in_symbolizer()) + return; + ThreadState *thr = cur_thread(); + const uptr pc = StackTrace::GetCurrentPc(); ForkBefore(thr, pc); - int pid; - { - // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and - // we'll assert in CheckNoLocks() unless we ignore interceptors. - ScopedIgnoreInterceptors ignore; - pid = REAL(fork)(fake); - } - if (pid == 0) { - // child - ForkChildAfter(thr, pc); - FdOnFork(thr, pc); - } else if (pid > 0) { - // parent - ForkParentAfter(thr, pc); - } else { - // error - ForkParentAfter(thr, pc); - } - return pid; +} + +void atfork_parent() { + if (in_symbolizer()) + return; + ThreadState *thr = cur_thread(); + const uptr pc = StackTrace::GetCurrentPc(); + ForkParentAfter(thr, pc); +} + +void atfork_child() { + if (in_symbolizer()) + return; + ThreadState *thr = cur_thread(); + const uptr pc = StackTrace::GetCurrentPc(); + ForkChildAfter(thr, pc); + FdOnFork(thr, pc); } TSAN_INTERCEPTOR(int, vfork, int fake) { @@ -2518,13 +2527,10 @@ FdRelease(thr, pc, fd); } -static void syscall_pre_fork(uptr pc) { - TSAN_SYSCALL(); - ForkBefore(thr, pc); -} +static void syscall_pre_fork(uptr pc) { ForkBefore(cur_thread(), pc); } static void syscall_post_fork(uptr pc, int pid) { - TSAN_SYSCALL(); + ThreadState *thr = cur_thread(); if (pid == 0) { // child ForkChildAfter(thr, pc); @@ -2840,6 +2846,10 @@ Printf("ThreadSanitizer: failed to setup atexit callback\n"); Die(); } + if (pthread_atfork(atfork_prepare, atfork_parent, atfork_child)) { + Printf("ThreadSanitizer: failed to setup atfork callbacks\n"); + Die(); + } #if !SANITIZER_MAC && !SANITIZER_NETBSD && !SANITIZER_FREEBSD if (pthread_key_create(&interceptor_ctx()->finalize_key, &thread_finalize)) { diff --git a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp @@ -145,7 +145,7 @@ static void SignalUnsafeCall(ThreadState *thr, uptr pc) { if (atomic_load_relaxed(&thr->in_signal_handler) == 0 || - !flags()->report_signal_unsafe) + !ShouldReport(thr, ReportTypeSignalUnsafe)) return; VarSizeStackTrace stack; ObtainCurrentStack(thr, pc, &stack); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -624,6 +624,7 @@ ScopedErrorReportLock lock_; }; +bool ShouldReport(ThreadState *thr, ReportType typ); ThreadContext *IsThreadStackOrTls(uptr addr, bool *is_stack); void RestoreStack(int tid, const u64 epoch, VarSizeStackTrace *stk, MutexSet *mset, uptr *tag = nullptr); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp @@ -519,23 +519,27 @@ void ForkBefore(ThreadState *thr, uptr pc) { ctx->thread_registry->Lock(); ctx->report_mtx.Lock(); - // Ignore memory accesses in the pthread_atfork callbacks. - // If any of them triggers a data race we will deadlock - // on the report_mtx. - // We could ignore interceptors and sync operations as well, + // Suppress all reports in the pthread_atfork callbacks. + // Reports will deadlock on the report_mtx. + // We could ignore sync operations as well, // but so far it's unclear if it will do more good or harm. // Unnecessarily ignoring things can lead to false positives later. - ThreadIgnoreBegin(thr, pc); + thr->suppress_reports++; + // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and + // we'll assert in CheckNoLocks() unless we ignore interceptors. + thr->ignore_interceptors++; } void ForkParentAfter(ThreadState *thr, uptr pc) { - ThreadIgnoreEnd(thr, pc); // Begin is in ForkBefore. + thr->suppress_reports--; // Enabled in ForkBefore. + thr->ignore_interceptors--; ctx->report_mtx.Unlock(); ctx->thread_registry->Unlock(); } void ForkChildAfter(ThreadState *thr, uptr pc) { - ThreadIgnoreEnd(thr, pc); // Begin is in ForkBefore. + thr->suppress_reports--; // Enabled in ForkBefore. + thr->ignore_interceptors--; ctx->report_mtx.Unlock(); ctx->thread_registry->Unlock(); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp @@ -51,6 +51,8 @@ // or false positives (e.g. unlock in a different thread). if (SANITIZER_GO) return; + if (!ShouldReport(thr, typ)) + return; ThreadRegistryLock l(ctx->thread_registry); ScopedReport rep(typ); rep.AddMutex(mid); @@ -107,7 +109,7 @@ if (!unlock_locked) s->Reset(thr->proc()); // must not reset it before the report is printed s->mtx.Unlock(); - if (unlock_locked) { + if (unlock_locked && ShouldReport(thr, ReportTypeMutexDestroyLocked)) { ThreadRegistryLock l(ctx->thread_registry); ScopedReport rep(ReportTypeMutexDestroyLocked); rep.AddMutex(mid); @@ -534,7 +536,7 @@ } void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) { - if (r == 0) + if (r == 0 || !ShouldReport(thr, ReportTypeDeadlock)) return; ThreadRegistryLock l(ctx->thread_registry); ScopedReport rep(ReportTypeDeadlock); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -142,6 +142,34 @@ return stack; } +bool ShouldReport(ThreadState *thr, ReportType typ) { + // We set thr->suppress_reports in the fork context. + // Taking any locking in the fork context can lead to deadlocks. + // If any locks are already taken, it's too late to do this check. + CheckNoLocks(thr); + // For the same reason check we didn't lock thread_registry yet. + if (SANITIZER_DEBUG) + ThreadRegistryLock l(ctx->thread_registry); + if (!flags()->report_bugs || thr->suppress_reports) + return false; + switch (typ) { + case ReportTypeSignalUnsafe: + return flags()->report_signal_unsafe; + case ReportTypeThreadLeak: +#if !SANITIZER_GO + // It's impossible to join phantom threads + // in the child after fork. + if (ctx->after_multithreaded_fork) + return false; +#endif + return flags()->report_thread_leaks; + case ReportTypeMutexDestroyLocked: + return flags()->report_destroy_locked; + default: + return true; + } +} + ScopedReportBase::ScopedReportBase(ReportType typ, uptr tag) { ctx->thread_registry->CheckLocked(); void *mem = internal_alloc(MBlockReport, sizeof(ReportDesc)); @@ -497,8 +525,10 @@ } bool OutputReport(ThreadState *thr, const ScopedReport &srep) { - if (!flags()->report_bugs || thr->suppress_reports) - return false; + // These should have been checked in ShouldReport. + // It's too late to check them here, we have already taken locks. + CHECK(flags()->report_bugs); + CHECK(!thr->suppress_reports); atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime()); const ReportDesc *rep = srep.GetReport(); CHECK_EQ(thr->current_report, nullptr); @@ -589,7 +619,7 @@ // at best it will cause deadlocks on internal mutexes. ScopedIgnoreInterceptors ignore; - if (!flags()->report_bugs) + if (!ShouldReport(thr, ReportTypeRace)) return; if (!flags()->report_atomic_races && !RaceBetweenAtomicAndFree(thr)) return; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -210,7 +210,7 @@ void ThreadFinalize(ThreadState *thr) { ThreadCheckIgnore(thr); #if !SANITIZER_GO - if (!flags()->report_thread_leaks) + if (!ShouldReport(thr, ReportTypeThreadLeak)) return; ThreadRegistryLock l(ctx->thread_registry); Vector leaks; diff --git a/compiler-rt/test/tsan/Linux/fork_syscall.cpp b/compiler-rt/test/tsan/Linux/fork_syscall.cpp new file mode 100644 --- /dev/null +++ b/compiler-rt/test/tsan/Linux/fork_syscall.cpp @@ -0,0 +1,47 @@ +// RUN: %clangxx_tsan -O1 %s -o %t && %env_tsan_opts=atexit_sleep_ms=50 %run %t 2>&1 | FileCheck %s +#include "../test.h" +#include +#include +#include +#include +#include + +int counter; + +static void *incrementer(void *p) { + for (;;) + __sync_fetch_and_add(&counter, 1); + return 0; +} + +int myfork() { + __sanitizer_syscall_pre_fork(); + int res = syscall(SYS_fork); + __sanitizer_syscall_post_fork(res); + return res; +} + +int main() { + barrier_init(&barrier, 2); + pthread_t th1; + pthread_create(&th1, 0, incrementer, 0); + for (int i = 0; i < 10; i++) { + switch (myfork()) { + default: // parent + while (wait(0) < 0) { + } + fprintf(stderr, "."); + break; + case 0: // child + __sync_fetch_and_add(&counter, 1); + exit(0); + break; + case -1: // error + fprintf(stderr, "failed to fork (%d)\n", errno); + exit(1); + } + } + fprintf(stderr, "OK\n"); +} + +// CHECK: OK diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock.c b/compiler-rt/test/tsan/pthread_atfork_deadlock.c --- a/compiler-rt/test/tsan/pthread_atfork_deadlock.c +++ b/compiler-rt/test/tsan/pthread_atfork_deadlock.c @@ -21,7 +21,7 @@ int main() { barrier_init(&barrier, 2); - pthread_atfork(atfork, NULL, NULL); + pthread_atfork(atfork, atfork, atfork); pthread_t t; pthread_create(&t, NULL, worker, NULL); glob++; diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock2.c b/compiler-rt/test/tsan/pthread_atfork_deadlock2.c --- a/compiler-rt/test/tsan/pthread_atfork_deadlock2.c +++ b/compiler-rt/test/tsan/pthread_atfork_deadlock2.c @@ -1,4 +1,4 @@ -// RUN: %clang_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clang_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s // Regression test for // https://groups.google.com/d/msg/thread-sanitizer/e_zB9gYqFHM/DmAiTsrLAwAJ // pthread_atfork() callback triggers a data race and we deadlocked @@ -22,7 +22,7 @@ int main() { barrier_init(&barrier, 2); - pthread_atfork(atfork, NULL, NULL); + pthread_atfork(atfork, atfork, atfork); pthread_t t; pthread_create(&t, NULL, worker, NULL); barrier_wait(&barrier); @@ -44,6 +44,10 @@ return 0; } -// CHECK-NOT: ThreadSanitizer: data race +// CHECK: ThreadSanitizer: data race +// CHECK: Write of size 4 +// CHECK: #0 atfork +// CHECK: Previous write of size 4 +// CHECK: #0 worker // CHECK: CHILD // CHECK: PARENT diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock3.c b/compiler-rt/test/tsan/pthread_atfork_deadlock3.c new file mode 100644 --- /dev/null +++ b/compiler-rt/test/tsan/pthread_atfork_deadlock3.c @@ -0,0 +1,98 @@ +// RUN: %clang_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s +// Regression test for +// https://groups.google.com/g/thread-sanitizer/c/TQrr4-9PRYo/m/HFR4FMi6AQAJ +#include "test.h" +#include +#include +#include +#include +#include + +long glob = 0; + +void *worker(void *main) { + glob++; + // synchronize with main + barrier_wait(&barrier); + // synchronize with atfork + barrier_wait(&barrier); + pthread_kill((pthread_t)main, SIGPROF); + barrier_wait(&barrier); + // synchronize with afterfork + barrier_wait(&barrier); + pthread_kill((pthread_t)main, SIGPROF); + barrier_wait(&barrier); + return NULL; +} + +void atfork() { + barrier_wait(&barrier); + barrier_wait(&barrier); + write(2, "in atfork\n", strlen("in atfork\n")); + static volatile long a; + __atomic_fetch_add(&a, 1, __ATOMIC_RELEASE); +} + +void afterfork() { + barrier_wait(&barrier); + barrier_wait(&barrier); + write(2, "in afterfork\n", strlen("in afterfork\n")); + static volatile long a; + __atomic_fetch_add(&a, 1, __ATOMIC_RELEASE); +} + +void afterfork_child() { + // Can't synchronize with barriers because we are + // in the new process, but want consistent output. + sleep(1); + write(2, "in afterfork_child\n", strlen("in afterfork_child\n")); + glob++; +} + +void handler(int sig) { + write(2, "in handler\n", strlen("in handler\n")); + glob++; +} + +int main() { + barrier_init(&barrier, 2); + struct sigaction act = {}; + act.sa_handler = &handler; + if (sigaction(SIGPROF, &act, 0)) { + perror("sigaction"); + exit(1); + } + pthread_atfork(atfork, afterfork, afterfork_child); + pthread_t t; + pthread_create(&t, NULL, worker, (void*)pthread_self()); + barrier_wait(&barrier); + pid_t pid = fork(); + if (pid < 0) { + fprintf(stderr, "fork failed: %d\n", errno); + return 1; + } + if (pid == 0) { + fprintf(stderr, "CHILD\n"); + return 0; + } + if (pid != waitpid(pid, NULL, 0)) { + fprintf(stderr, "waitpid failed: %d\n", errno); + return 1; + } + pthread_join(t, NULL); + fprintf(stderr, "PARENT\n"); + return 0; +} + +// CHECK: in atfork +// CHECK: in handler +// CHECK: ThreadSanitizer: data race +// CHECK: Write of size 8 +// CHECK: #0 handler +// CHECK: Previous write of size 8 +// CHECK: #0 worker +// CHECK: afterfork +// CHECK: in handler +// CHECK: afterfork_child +// CHECK: CHILD +// CHECK: PARENT