i#5256 sigprocmask: enable linux.sigaction on MacOS

core/arch/x86/x86.asm

@@ -641,6 +641,10 @@ cat_have_lock:
         mov      REG_XDI, REG_XAX    /* esp to use */
 #endif
         mov      REG_XSI, [2*ARG_SZ + REG_XBP]  /* sysnum */
+#ifdef MACOS64
+        /* For now we assume a BSD syscall */
+        or       REG_XSI, SYSCALL_NUM_MARKER_BSD
+#endif
         pop      REG_XAX             /* syscall */
         pop      REG_XCX             /* dstack */
 #if defined(UNIX) && !defined(X64)
@@ -1261,7 +1265,7 @@ dynamorio_sys_exit_next:
         mov      ARG2, REG_XAX /* kernel port, which we just acquired */
         mov      ARG1, 0 /* join semaphore: SEMAPHORE_NULL */
         mov      eax, SYS_bsdthread_terminate
-        or       eax, HEX(2000000) /* 2<<24 for BSD syscall */
+        or       eax, SYSCALL_NUM_MARKER_BSD
         mov      r10, rcx
         syscall
 # else

core/drlibc/drlibc_x86.asm

@@ -43,7 +43,7 @@
 #include "../arch/asm_defines.asm"
 #include "../arch/x86/x86_asm_defines.asm" /* PUSHGPR, POPGPR, etc. */
 #ifdef MACOS
-# include "include/syscall_mach.h" /* SYSCALL_NUM_MARKER_MACH */
+# include "include/syscall_mach.h" /* SYSCALL_NUM_MARKER_* */
 #endif
 START_FILE
 
@@ -67,8 +67,8 @@ GLOBAL_LABEL(dynamorio_syscall:)
         mov      REG_XBX, ARG2 /* put num_args where we can reference it longer */
         mov      rax, ARG1 /* sysnum: only need eax, but need rax for ARG1 (or movzx) */
 #  ifdef MACOS
-        /* for now we assume a BSD syscall */
-        or       rax, 0x2000000
+        /* For now we assume a BSD syscall */
+        or       rax, SYSCALL_NUM_MARKER_BSD
 #  endif
         cmp      REG_XBX, 0
         je       syscall_ready

core/unix/include/syscall_mach.h

@@ -38,6 +38,7 @@
 #define _SYSCALL_MACH_H_ 1
 
 #define SYSCALL_NUM_MARKER_MACH 0x1000000
+#define SYSCALL_NUM_MARKER_BSD 0x2000000
 #define SYSCALL_NUM_MARKER_MACHDEP 0x3000000
 
 #define MACH__kernelrpc_mach_vm_allocate_trap 10

core/unix/os.c

@@ -4880,7 +4880,7 @@ os_normalized_sysnum(int num_raw, instr_t *gateway, dcontext_t *dcontext)
         }
     }
 #    ifdef X64
-    if (num_raw >> 24 == 0x2)
+    if (TEST(SYSCALL_NUM_MARKER_BSD, num_raw))
         return (int)(num_raw & 0xffffff); /* Drop BSD bit */
     else
         num = (int)num_raw; /* Keep Mach and Machdep bits */
@@ -7331,7 +7331,6 @@ pre_system_call(dcontext_t *dcontext)
         break;
     }
     case IF_MACOS_ELSE(SYS_sigprocmask, SYS_rt_sigprocmask): { /* 175 */
-        /* TODO i#5256: Fx this path and enable linux.sigaction on MacOS. */
         /* in /usr/src/linux/kernel/signal.c:
            asmlinkage long
            sys_rt_sigprocmask(int how, sigset_t *set, sigset_t *oset,
@@ -8927,9 +8926,12 @@ post_system_call(dcontext_t *dcontext)
              size_t sigsetsize)
          */
         /* FIXME i#148: Handle syscall failure. */
-        handle_post_sigprocmask(
+        int status = handle_post_sigprocmask(
             dcontext, (int)dcontext->sys_param0, (kernel_sigset_t *)dcontext->sys_param1,
             (kernel_sigset_t *)dcontext->sys_param2, (size_t)dcontext->sys_param3);
+        if (status != 0) {
+            set_failure_return_val(dcontext, status);
+        }
         break;
     }
 #if defined(LINUX) && !defined(X64)

core/unix/os_private.h

@@ -351,7 +351,7 @@ handle_sigaltstack(dcontext_t *dcontext, const stack_t *stack, stack_t *old_stac
 bool
 handle_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *set,
                    kernel_sigset_t *oset, size_t sigsetsize, uint *error_code);
-void
+int
 handle_post_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *set,
                         kernel_sigset_t *oset, size_t sigsetsize);
 void

core/unix/signal.c

@@ -87,6 +87,9 @@
 #    include "include/syscall.h"
 #else
 #    include <sys/syscall.h>
+#    ifdef MACOS
+#        include "include/syscall_mach.h"
+#    endif
 #endif
 
 #include "instrument.h"
@@ -2297,16 +2300,11 @@ handle_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *app_set,
 {
     thread_sig_info_t *info = (thread_sig_info_t *)dcontext->signal_field;
     int i;
-    kernel_sigset_t safe_set, safe_old_set;
+    kernel_sigset_t safe_set;
     /* Some code uses this syscall to check whether the given address is
      * readable. E.g.
      * github.com/abseil/abseil-cpp/blob/master/absl/debugging/internal/
      * address_is_readable.cc#L85
-     * Those uses as well as our checks below don't guarantee that the given
-     * address will _remain_ readable or writable, even if they succeed now.
-     * TODO i#5255: DR can at least pass the safe_set to the actual syscall
-     * (in cases where it is not skipped) to avoid a second read of app_set,
-     * by the kernel.
      */
     if (sigsetsize != sizeof(kernel_sigset_t)) {
         if (error_code != NULL)
@@ -2323,15 +2321,15 @@ handle_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *app_set,
             *error_code = EINVAL;
         return false;
     }
-    if (oset != NULL) {
-        /* Old sigset should be writable too. */
-        if (!d_r_safe_read(oset, sizeof(safe_old_set), &safe_old_set) ||
-            !safe_write_ex(oset, sizeof(safe_old_set), &safe_old_set, NULL)) {
-            if (error_code != NULL)
-                *error_code = EFAULT;
-            return false;
-        }
-    }
+    /* We intentionally do not check validity of oset here. This is because,
+     * for an unwriteable oset, the native sigprocmask syscall shows a weird
+     * non-atomic behavior: it returns EFAULT, but also updates the app signal
+     * mask. To replicate the same behavior, we allow the following code to
+     * update the app signal masks, which happens either by making the actual
+     * syscall (for -no_intercept_all_signals) or by updating app_sigblocked
+     * (for -intercept_all_signals). oset is checked in handle_post_sigprocmask
+     * and any failures are returned to the app.
+     */
     /* If we're intercepting all, we emulate the whole thing */
     bool execute_syscall = !DYNAMO_OPTION(intercept_all_signals);
     LOG(THREAD, LOG_ASYNCH, 2, "handle_sigprocmask\n");
@@ -2345,7 +2343,9 @@ handle_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *app_set,
              * XXX i#1187: we could crash here touching app memory -- could
              * use TRY, but the app could pass read-only memory and it
              * would work natively!  Better to swap in our own
-             * allocated data struct.  There's a transparency issue w/
+             * allocated data struct. This would prevent a second read (by
+             * the kernel) too, which might fail if it does not _remain_
+             * readable anymore.  There's a transparency issue w/
              * races too if another thread looks at this memory.  This
              * won't happen by default b/c -intercept_all_signals is
              * on by default so we don't try to solve all these
@@ -2404,44 +2404,76 @@ handle_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *app_set,
         check_signals_pending(dcontext, info);
     }
     if (!execute_syscall) {
-        handle_post_sigprocmask(dcontext, how, app_set, oset, sigsetsize);
+        int status = handle_post_sigprocmask(dcontext, how, app_set, oset, sigsetsize);
+        if (status != 0 && error_code != NULL) {
+            *error_code = status;
+        }
         return false; /* skip syscall */
     } else
         return true;
 }
 
 /* need to add in our signals that the app thinks are blocked */
-void
+int
 handle_post_sigprocmask(dcontext_t *dcontext, int how, kernel_sigset_t *app_set,
                         kernel_sigset_t *oset, size_t sigsetsize)
 {
     thread_sig_info_t *info = (thread_sig_info_t *)dcontext->signal_field;
     int i;
-    /* TODO i#5255: Handle the case where the old sigset was writable in
-     * handle_sigprocmask but not now. Also, for the case where app_set is only
-     * readable but not writable.
-     */
     if (!DYNAMO_OPTION(intercept_all_signals)) {
-        /* Restore app memory */
+        /* Restore app memory.
+         * XXX i#1187: See comment in handle_sigprocmask about read-only
+         * app_set. Ideally, we would replace app_set with our own copy
+         * pre-syscall so that we only need to restore a pointer to the original
+         * app copy post-syscall.
+         * In case of a write failure below, we do not return an EFAULT as it would
+         * be incorrect to expect app_set to be writeable. We try continuing and
+         * return a cloberred app_set to the app. This is low priority as
+         * -intercept_all_signals is true by default.
+         */
         safe_write_ex(app_set, sizeof(*app_set), &info->pre_syscall_app_sigprocmask,
                       NULL);
     }
     if (oset != NULL) {
-        if (DYNAMO_OPTION(intercept_all_signals))
-            safe_write_ex(oset, sizeof(*oset), &info->pre_syscall_app_sigblocked, NULL);
-        else {
-            /* the syscall wrote to oset already, so just add any additional */
-            for (i = 1; i <= MAX_SIGNUM; i++) {
-                if (EMULATE_SIGMASK(info, i) &&
-                    /* use the pre-syscall value: do not take into account changes
-                     * from this syscall itself! (PR 523394)
-                     */
-                    kernel_sigismember(&info->pre_syscall_app_sigblocked, i)) {
-                    kernel_sigaddset(oset, i);
-                }
-            }
+        /* We want to handle the following cases where we are unable to fix oset
+         * because it is not writeable (anymore). Note that handle_sigprocmask
+         * does the writeability check only for non-MacOS.
+         * - On non-Mac systems, if oset was initially writable in our
+         *   handle_sigprocmask check, but is unwriteable now, we want to
+         *   return EFAULT.
+         * - On Mac systems, where a bad oset does not cause EFAULT,
+         *   we want to fail the write silently.
+         */
+        if (DYNAMO_OPTION(intercept_all_signals)) {
+            if (!safe_write_ex(oset, sizeof(*oset), &info->pre_syscall_app_sigblocked,
+                               NULL))
+                return IF_MACOS_ELSE(0, EFAULT);
+        } else {
+            bool write_fault = true;
+            TRY_EXCEPT(
+                dcontext,
+                {
+                    /* the syscall wrote to oset already, so just add any additional */
+                    for (i = 1; i <= MAX_SIGNUM; i++) {
+                        if (EMULATE_SIGMASK(info, i) &&
+                            /* use the pre-syscall value: do not take into account changes
+                             * from this syscall itself! (PR 523394)
+                             */
+                            kernel_sigismember(&info->pre_syscall_app_sigblocked, i)) {
+                            kernel_sigaddset(oset, i);
+                        }
+                    }
+                    write_fault = false;
+                },
+                {
+                    /* EXCEPT */
+                    /* nothing: write_fault is already true */
+                });
+            if (write_fault)
+                return IF_MACOS_ELSE(0, EFAULT);
         }
     }
+    return 0;
 }
 
 void
@@ -4843,7 +4875,7 @@ record_pending_signal(dcontext_t *dcontext, int sig, kernel_ucontext_t *ucxt,
 static bool
 is_sys_kill(dcontext_t *dcontext, byte *pc, byte *xsp, kernel_siginfo_t *info)
 {
-#if !defined(VMX86_SERVER) && !defined(MACOS) /* does not use SI_KERNEL */
+#if !defined(VMX86_SERVER) /* does not use SI_KERNEL */
     /* i#133: use si_code to distinguish user-sent signals.
      * Even 2.2 Linux kernel supports <=0 meaning user-sent (except
      * SIGIO) so we assume we can rely on it.

suite/tests/CMakeLists.txt

@@ -99,6 +99,9 @@ endif (UNIX)
 set(main_run_list
   # our main configuration
   "SHORT::-code_api"
+  # no_intercept_all_signals tests.
+  # TODO i#5261: Remove this suite when -intercept_all_signals is deprecated.
+  "SHORT::LIN::ONLY::sigaction$::-no_code_api -no_intercept_all_signals"
   # sanity check: run single app to make sure these options aren't totally broken
   # i#1575: ARM doesn't yet support -coarse_units
   "SHORT::X86::ONLY::client.events$::-code_api -opt_memory"
@@ -4142,6 +4145,9 @@ if (UNIX)
   tobuild(linux.sigplain110 linux/sigplain110.c)
   tobuild(linux.sigplain111 linux/sigplain111.c)
   tobuild(linux.sigaction   linux/sigaction.c)
+  # We want a native run to verify our assumptions of kernel behaviour.
+  torunonly_native(code_api|linux.sigaction.native linux.sigaction
+    "" linux/sigaction.c "")
   if (LINUX)
     tobuild(linux.syscall_pwait linux/syscall_pwait.cpp)
     link_with_pthread(linux.syscall_pwait)
@@ -4789,6 +4795,9 @@ if (APPLE)
     code_api|api.dis
     code_api|api.ir-static
     code_api|api.drdecode
+    code_api|linux.sigaction
+    no_code_api,no_intercept_all_signals|linux.sigaction
+    code_api|linux.sigaction.native
     PROPERTIES LABELS OSX)
   if (DEBUG) # FIXME i#1806: fails in release
     set_tests_properties(code_api|client.flush PROPERTIES LABELS OSX)