Possible infinite signal loop with TSan under MacOS

[dustanddreams]

The following simple signal test case:

#include <err.h>
#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <unistd.h>

int r = 0;

void handler(int signo)
{
        r = 1;
}

int main()
{
        void *altstack = malloc(2 * MINSIGSTKSZ);
        if (altstack == NULL)
                perror("malloc");

        stack_t stk = { .ss_sp = altstack, .ss_size = 2 * MINSIGSTKSZ,
            .ss_flags = 0 };
        if (sigaltstack(&stk, NULL) != 0)
                perror("sigaltstack"); 

        // Mask SIGINT
        sigset_t set;
        sigemptyset(&set);
        sigaddset(&set, SIGINT);
        int ret = sigprocmask(SIG_BLOCK, &set, NULL);
        if (ret != 0)
                perror("sigprocmask");

        // Install handler for SIGINT
        struct sigaction act = { 0 };
        sigemptyset(&act.sa_mask); 
        act.sa_flags = SA_ONSTACK;
        act.sa_handler = &handler;
        if (sigaction(SIGINT, &act, NULL) != 0)
                perror("sigaction");

        // Send SIGINT to self
        ret = kill(getpid(), SIGINT);
        if (ret != 0)
                perror("kill");

        // Unmask SIGINT
        sigemptyset(&set);
        sigaddset(&set, SIGINT);
        ret = sigprocmask(SIG_UNBLOCK, &set, NULL);
        if (ret != 0)
                perror("sigprocmask");

        // Should print "r = 1"
        printf("r = %d\n", r);
        return EXIT_SUCCESS;
}

does not behave correctly under MacOS when compiled with TSan (clang -fsanitize=thread test.c && ./a.out)

The expected behaviour is to have the process exit after printing r = 1. Instead, the process becomes stuck, unsuspendable and unkillable (except with kill -9).

The reason for this behaviour is an out-of-bound memory access in the TSan wrapper for signals, which causes SIGSEGV to be sent, only for the signal handler to fault again, etc.

In compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp, wrapper sighandler ends up copying the siginfo_t and ucontext_t arguments of the signal handler. Unfortunately, the definitions used for these types, on MacOS, are larger than reality; when run on an alternate signal stack, this will cause an access beyond the end of said stack, which is very likely to be an unmapped page, and thus will cause a segmentation fault.

The following patch solves the problem for me (tested on x86_64, Darwin Kernel
Version 22.5.0).

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
index 58244c9944a0..97b0348170a5 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -601,6 +601,25 @@ union __sanitizer_siginfo {
   };
   __sanitizer_siginfo_pad pad;
 };
+#elif SANITIZER_APPLE
+struct __sanitizer_siginfo_macos {
+  // MacOS siginfo_t is 6 ints + 2 pointers + 8 longs, thus 64 bytes on
+  // 32-bit platforms and 104 on 64-bit platforms.
+#if SANITIZER_X32
+  u32 pad[64 / sizeof(u32)];
+#else
+  u64 pad[104 / sizeof(u64)];
+#endif
+};
+# define SANITIZER_HAS_SIGINFO 1
+union __sanitizer_siginfo {
+  struct {
+    int si_signo;
+    int si_errno;
+    int si_code;
+  };
+  __sanitizer_siginfo_macos pad;
+};
 #else
 # define SANITIZER_HAS_SIGINFO 0
 typedef __sanitizer_siginfo_pad __sanitizer_siginfo;
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 44d20e7ca690..9c3bff7c3ae9 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -63,6 +63,21 @@ const int kSigCount = 129;
 const int kSigCount = 65;
 #endif

+#if SANITIZER_APPLE
+struct ucontext_t {
+  // MacOS ucontext_t, without the mcontext_t storage, is 2 ints + 3 pointers +
+  // 2 long + 1 int padded to long, thus 32 bytes on 32-bit platforms and
+  // 56 bytes on 64-bit platforms.
+  // During signal delivery, the mcontext_t part is not contiguous with the
+  // ucontext_t part, so we can not afford blindly declaring an ucontext_t
+  // spanning the whole struct.
+#if SANITIZER_X32
+  u32 opaque[32 / sizeof(u32)];
+#else
+  u64 opaque[56 / sizeof(u64)];
+#endif
+};
+#else
 #ifdef __mips__
 struct ucontext_t {
   u64 opaque[768 / sizeof(u64) + 1];
@@ -73,6 +88,7 @@ struct ucontext_t {
   u64 opaque[936 / sizeof(u64) + 1];
 };
 #endif
+#endif

 #if defined(__x86_64__) || defined(__mips__) || SANITIZER_PPC64V1 || \
     defined(__s390x__)

Credits: the test case is based upon a test program written by @OlivierNicole.