Fix TSAN signal interceptor out-of-bound access

[scw]

Summary

signal(2) and sigaction(2) have defined behaviors for invalid signal number
(EINVAL) and some programs rely on it.

The added test case also reveals that MSAN is too strict in this regard.

Test case passed on x86_64 Linux and AArch64 Linux.

[vitalybuka]

Can you please add some basic test, ideally under compiler-rt/test/sanitizer_common/TestCases/Posix/ so we "ninja check-sanitizer" tests asan and msan as well

[scw]

Added test case, needed to update MSAN for it as well. Tested on x86_86 Linux and AArch64 Linux.

[scw]

Oops, that was the old file. This is the actual update.

[vitalybuka]

compiler-rt/lib/msan/msan_interceptors.cpp

1377

If so, why not on compiler-rt/lib/sanitizer_common/sanitizer_signal_interceptors.inc:54 before GetHandleSignalMode check?

compiler-rt/test/sanitizer_common/TestCases/Posix/signal.cpp

220

Thanks if it works or all platforms.
I expected something basic which just triggers both branches of "if (signo <= 0 || signo >= kMaxSignals)"

[scw]

compiler-rt/lib/msan/msan_interceptors.cpp

1377

sanitizer_signal_interceptors.inc is also included by asan_interceptors.cpp which does not intercept signals and does not define kMaxSignals.

compiler-rt/test/sanitizer_common/TestCases/Posix/signal.cpp

220

Actually missed a change on relaxing checking the results of SIGCHLD. I'll try this and simplify it if build bots for different platforms complain.

[scw]

Relaxed SIGCHLD handling.

[vitalybuka]

Thank you.
LGTM

compiler-rt/lib/msan/msan_interceptors.cpp

 
 #include "interception/interception.h"
 #include "msan.h"
 #include "msan_chained_origin_depot.h"
 #include "msan_origin.h"
 #include "msan_report.h"
 #include "msan_thread.h"
 #include "msan_poisoning.h"
+#include "sanitizer_common/sanitizer_errno_codes.h"
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
 #include "sanitizer_common/sanitizer_platform_limits_netbsd.h"
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_allocator_interface.h"
 #include "sanitizer_common/sanitizer_allocator_internal.h"
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_errno.h"
#define SIGNAL_INTERCEPTOR_SIGNAL_IMPL(func, signo, handler) \
     return REAL(func)(signo, handler);                       \
   }
 
 #include "sanitizer_common/sanitizer_signal_interceptors.inc"
 
 static int sigaction_impl(int signo, const __sanitizer_sigaction *act,
                           __sanitizer_sigaction *oldact) {
   ENSURE_MSAN_INITED();
+  if (signo <= 0 || signo >= kMaxSignals) {
+    errno = errno_EINVAL;
+    return -1;
+  }
   if (act) read_sigaction(act);
   int res;
   if (flags()->wrap_signals) {
     SpinMutexLock lock(&sigactions_mu);
-    CHECK_LT(signo, kMaxSignals);
     uptr old_cb = atomic_load(&sigactions[signo], memory_order_relaxed);
     __sanitizer_sigaction new_act;
     __sanitizer_sigaction *pnew_act = act ? &new_act : nullptr;
     if (act) {
       REAL(memcpy)(pnew_act, act, sizeof(__sanitizer_sigaction));
       uptr cb = (uptr)pnew_act->sigaction;
       uptr new_cb = (pnew_act->sa_flags & __sanitizer::sa_siginfo)
                         ? (uptr)SignalAction
static int sigaction_impl(int signo, const __sanitizer_sigaction *act,
   if (res == 0 && oldact) {
     __msan_unpoison(oldact, sizeof(__sanitizer_sigaction));
   }
   return res;
 }
 
 static uptr signal_impl(int signo, uptr cb) {
   ENSURE_MSAN_INITED();
+  if (signo <= 0 || signo >= kMaxSignals) {
+    errno = errno_EINVAL;
+    return -1;
+  }
   if (flags()->wrap_signals) {
-    CHECK_LT(signo, kMaxSignals);
     SpinMutexLock lock(&sigactions_mu);
     if (cb != __sanitizer::sig_ign && cb != __sanitizer::sig_dfl) {
       atomic_store(&sigactions[signo], cb, memory_order_relaxed);
       cb = (uptr)&SignalHandler;
     }
   }
   return cb;
 }

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

 #include "sanitizer_common/sanitizer_signal_interceptors.inc"
 
 int sigaction_impl(int sig, const __sanitizer_sigaction *act,
                    __sanitizer_sigaction *old) {
   // Note: if we call REAL(sigaction) directly for any reason without proxying
   // the signal handler through rtl_sigaction, very bad things will happen.
   // The handler will run synchronously and corrupt tsan per-thread state.
   SCOPED_INTERCEPTOR_RAW(sigaction, sig, act, old);
+  if (sig <= 0 || sig >= kSigCount) {
+    errno = errno_EINVAL;
+    return -1;
+  }
   __sanitizer_sigaction *sigactions = interceptor_ctx()->sigactions;
   __sanitizer_sigaction old_stored;
   if (old) internal_memcpy(&old_stored, &sigactions[sig], sizeof(old_stored));
   __sanitizer_sigaction newact;
   if (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,

compiler-rt/test/sanitizer_common/TestCases/Posix/signal.cpp

+// RUN: %clangxx -O0 -g %s -o %t && %run %t 2>&1 | FileCheck %s
+
+#include <assert.h>
+#include <climits>
+#include <errno.h>
+#include <stdio.h>
+#include <signal.h>
+
+#include <initializer_list>
+
+constexpr int std_signals[] = {
+  SIGHUP,
+  SIGINT,
+  SIGQUIT,
+  SIGILL,
+  SIGTRAP,
+  SIGABRT,
+  SIGIOT,
+  SIGBUS,
+  SIGFPE,
+  SIGUSR1,
+  SIGSEGV,
+  SIGUSR2,
+  SIGPIPE,
+  SIGALRM,
+  SIGTERM,
+  SIGCHLD,
+  SIGCONT,
+  SIGTSTP,
+  SIGTTIN,
+  SIGTTOU,
+  SIGURG,
+  SIGXCPU,
+  SIGXFSZ,
+  SIGVTALRM,
+  SIGPROF,
+  SIGWINCH,
+  SIGIO,
+  SIGSYS,
+};
+
+constexpr int no_change_act_signals[] = {
+  SIGKILL,
+  SIGSTOP,
+};
+
+void signal_handler(int) {}
+void signal_action_handler(int, siginfo_t*, void*) {}
+
+void test_signal_custom() {
+  for (int signum : std_signals) {
+    sighandler_t ret = signal(signum, &signal_handler);
+    assert(ret != SIG_ERR);
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    sighandler_t ret = signal(signum, &signal_handler);
+    assert(ret != SIG_ERR);
+  }
+  for (int signum : no_change_act_signals) {
+    sighandler_t ret = signal(signum, &signal_handler);
+    int err = errno;
+    assert(ret == SIG_ERR);
+    assert(err == EINVAL);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    sighandler_t ret = signal(signum, &signal_handler);
+    int err = errno;
+    assert(ret == SIG_ERR);
+    assert(err == EINVAL);
+  }
+}
+
+void test_signal_ignore() {
+  for (int signum : std_signals) {
+    sighandler_t ret = signal(signum, SIG_IGN);
+    if (signum != SIGCHLD) {
+      // POSIX.1-1990 disallowed setting the action for SIGCHLD to SIG_IGN
+      // though POSIX.1-2001 and later allow this possibility.
+      assert(ret != SIG_ERR);
+    }
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    sighandler_t ret = signal(signum, SIG_IGN);
+    assert(ret != SIG_ERR);
+  }
+  for (int signum : no_change_act_signals) {
+    sighandler_t ret = signal(signum, SIG_IGN);
+    int err = errno;
+    assert(ret == SIG_ERR);
+    assert(err == EINVAL);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    sighandler_t ret = signal(signum, SIG_IGN);
+    int err = errno;
+    assert(ret == SIG_ERR);
+    assert(err == EINVAL);
+  }
+}
+
+void test_signal_default() {
+  for (int signum : std_signals) {
+    sighandler_t ret = signal(signum, SIG_DFL);
+    assert(ret != SIG_ERR);
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    sighandler_t ret = signal(signum, SIG_DFL);
+    assert(ret != SIG_ERR);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    sighandler_t ret = signal(signum, SIG_DFL);
+    int err = errno;
+    assert(ret == SIG_ERR);
+    assert(err == EINVAL);
+  }
+}
+
+void test_sigaction_custom() {
+  struct sigaction act = {}, oldact;
+
+  act.sa_handler = &signal_handler;
+  sigemptyset(&act.sa_mask);
+  act.sa_flags = 0;
+
+  for (int signum : std_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum : no_change_act_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+
+  act.sa_handler = nullptr;
+  act.sa_sigaction = &signal_action_handler;
+  act.sa_flags = SA_SIGINFO;
+
+  for (int signum : std_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum : no_change_act_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+}
+
+void test_sigaction_ignore() {
+  struct sigaction act = {}, oldact;
+
+  act.sa_handler = SIG_IGN;
+  sigemptyset(&act.sa_mask);
+  act.sa_flags = 0;
+
+  for (int signum : std_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    if (signum != SIGCHLD) {
+      // POSIX.1-1990 disallowed setting the action for SIGCHLD to SIG_IGN
+      // though POSIX.1-2001 and later allow this possibility.
+      assert(ret == 0);
+    }
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum : no_change_act_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+}
+
+void test_sigaction_default() {
+  struct sigaction act = {}, oldact;
+
+  act.sa_handler = SIG_DFL;
+  sigemptyset(&act.sa_mask);
+  act.sa_flags = 0;
+
+  for (int signum : std_signals) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum = SIGRTMIN; signum <= SIGRTMAX; ++signum) {
+    int ret = sigaction(signum, &act, &oldact);
+    assert(ret == 0);
+  }
+  for (int signum : { 0, SIGRTMAX + 1, INT_MAX}) {
+    int ret = sigaction(signum, &act, &oldact);
+    int err = errno;
+    assert(ret == -1);
+    assert(err == EINVAL);
+  }
+}
+
+int main(void) {
+  printf("sigaction\n");
+
+  test_signal_custom();
+  test_signal_ignore();
+  test_signal_default();
+
+  test_sigaction_custom();
+  test_sigaction_ignore();
+  test_sigaction_default();
+
+  // CHECK: sigaction
+
+  return 0;
+}