[compiler-rt] r224530 - tsan: fix data races between signal handler and sigaction
Dmitry Vyukov
dvyukov at google.com
Thu Dec 18 10:31:47 PST 2014
Author: dvyukov
Date: Thu Dec 18 12:31:47 2014
New Revision: 224530
URL: http://llvm.org/viewvc/llvm-project?rev=224530&view=rev
Log:
tsan: fix data races between signal handler and sigaction
signal handler reads sa_sigaction when a concurrent sigaction call can modify it
as the result in could try to call SIG_DFL or a partially overwritten function pointer
Added:
compiler-rt/trunk/test/tsan/signal_reset.cc
Modified:
compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc
Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc?rev=224530&r1=224529&r2=224530&view=diff
==============================================================================
--- compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc (original)
+++ compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc Thu Dec 18 12:31:47 2014
@@ -101,14 +101,15 @@ typedef long long_t; // NOLINT
# define F_TLOCK 2 /* Test and lock a region for exclusive use. */
# define F_TEST 3 /* Test a region for other processes locks. */
-typedef void (*sighandler_t)(int sig);
-
#define errno (*__errno_location())
+typedef void (*sighandler_t)(int sig);
+typedef void (*sigactionhandler_t)(int sig, my_siginfo_t *siginfo, void *uctx);
+
struct sigaction_t {
union {
sighandler_t sa_handler;
- void (*sa_sigaction)(int sig, my_siginfo_t *siginfo, void *uctx);
+ sigactionhandler_t sa_sigaction;
};
#if SANITIZER_FREEBSD
int sa_flags;
@@ -1873,15 +1874,18 @@ static void CallUserSignalHandler(Thread
// Ensure that the handler does not spoil errno.
const int saved_errno = errno;
errno = 99;
- // Need to remember pc before the call, because the handler can reset it.
- uptr pc = sigact ?
+ // This code races with sigaction. Be careful to not read sa_sigaction twice.
+ // Also need to remember pc for reporting before the call,
+ // because the handler can reset it.
+ volatile uptr pc = sigact ?
(uptr)sigactions[sig].sa_sigaction :
(uptr)sigactions[sig].sa_handler;
- pc += 1; // return address is expected, OutputReport() will undo this
- if (sigact)
- sigactions[sig].sa_sigaction(sig, info, uctx);
- else
- sigactions[sig].sa_handler(sig);
+ if (pc != (uptr)SIG_DFL && pc != (uptr)SIG_IGN) {
+ if (sigact)
+ ((sigactionhandler_t)pc)(sig, info, uctx);
+ else
+ ((sighandler_t)pc)(sig);
+ }
// We do not detect errno spoiling for SIGTERM,
// because some SIGTERM handlers do spoil errno but reraise SIGTERM,
// tsan reports false positive in such case.
@@ -1891,7 +1895,9 @@ static void CallUserSignalHandler(Thread
// signal; and it looks too fragile to intercept all ways to reraise a signal.
if (flags()->report_bugs && !sync && sig != SIGTERM && errno != 99) {
VarSizeStackTrace stack;
- ObtainCurrentStack(thr, pc, &stack);
+ // Add 1 to pc because return address is expected,
+ // OutputReport() will undo this.
+ ObtainCurrentStack(thr, pc + 1, &stack);
ThreadRegistryLock l(ctx->thread_registry);
ScopedReport rep(ReportTypeErrnoInSignal);
if (!IsFiredSuppression(ctx, rep, stack)) {
@@ -1917,11 +1923,8 @@ void ProcessPendingSignals(ThreadState *
SignalDesc *signal = &sctx->pending_signals[sig];
if (signal->armed) {
signal->armed = false;
- if (sigactions[sig].sa_handler != SIG_DFL
- && sigactions[sig].sa_handler != SIG_IGN) {
- CallUserSignalHandler(thr, false, true, signal->sigaction,
- sig, &signal->siginfo, &signal->ctx);
- }
+ CallUserSignalHandler(thr, false, true, signal->sigaction, sig,
+ &signal->siginfo, &signal->ctx);
}
}
pthread_sigmask(SIG_SETMASK, &oldset, 0);
@@ -2003,7 +2006,19 @@ TSAN_INTERCEPTOR(int, sigaction, int sig
internal_memcpy(old, &sigactions[sig], sizeof(*old));
if (act == 0)
return 0;
- internal_memcpy(&sigactions[sig], act, sizeof(*act));
+ // Copy act into sigactions[sig].
+ // Can't use struct copy, because compiler can emit call to memcpy.
+ // Can't use internal_memcpy, because it copies byte-by-byte,
+ // and signal handler reads the sa_handler concurrently. It it can read
+ // some bytes from old value and some bytes from new value.
+ // Use volatile to prevent insertion of memcpy.
+ sigactions[sig].sa_handler = *(volatile sighandler_t*)&act->sa_handler;
+ sigactions[sig].sa_flags = *(volatile int*)&act->sa_flags;
+ internal_memcpy(&sigactions[sig].sa_mask, &act->sa_mask,
+ sizeof(sigactions[sig].sa_mask));
+#if !SANITIZER_FREEBSD
+ sigactions[sig].sa_restorer = act->sa_restorer;
+#endif
sigaction_t newact;
internal_memcpy(&newact, act, sizeof(newact));
REAL(sigfillset)(&newact.sa_mask);
Added: compiler-rt/trunk/test/tsan/signal_reset.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/signal_reset.cc?rev=224530&view=auto
==============================================================================
--- compiler-rt/trunk/test/tsan/signal_reset.cc (added)
+++ compiler-rt/trunk/test/tsan/signal_reset.cc Thu Dec 18 12:31:47 2014
@@ -0,0 +1,74 @@
+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <sys/types.h>
+#include <sys/time.h>
+#include <unistd.h>
+#include <errno.h>
+
+volatile int X;
+int stop;
+
+static void handler(int sig) {
+ (void)sig;
+ if (X != 0)
+ printf("bad");
+}
+
+static void* busy(void *p) {
+ while (__atomic_load_n(&stop, __ATOMIC_RELAXED) == 0) {
+ }
+ return 0;
+}
+
+static void* reset(void *p) {
+ struct sigaction act = {};
+ for (int i = 0; i < 1000000; i++) {
+ act.sa_handler = &handler;
+ if (sigaction(SIGPROF, &act, 0)) {
+ perror("sigaction");
+ exit(1);
+ }
+ act.sa_handler = SIG_IGN;
+ if (sigaction(SIGPROF, &act, 0)) {
+ perror("sigaction");
+ exit(1);
+ }
+ }
+ return 0;
+}
+
+int main() {
+ struct sigaction act = {};
+ act.sa_handler = SIG_IGN;
+ if (sigaction(SIGPROF, &act, 0)) {
+ perror("sigaction");
+ exit(1);
+ }
+
+ itimerval t;
+ t.it_value.tv_sec = 0;
+ t.it_value.tv_usec = 10;
+ t.it_interval = t.it_value;
+ if (setitimer(ITIMER_PROF, &t, 0)) {
+ perror("setitimer");
+ exit(1);
+ }
+
+ pthread_t th[2];
+ pthread_create(&th[0], 0, busy, 0);
+ pthread_create(&th[1], 0, reset, 0);
+
+ pthread_join(th[1], 0);
+ __atomic_store_n(&stop, 1, __ATOMIC_RELAXED);
+ pthread_join(th[0], 0);
+
+ fprintf(stderr, "DONE\n");
+ return 0;
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer:
+// CHECK: DONE
+// CHECK-NOT: WARNING: ThreadSanitizer:
More information about the llvm-commits
mailing list