[LibOS] Make the signal disposition a per-process attribute

LibOS/shim/include/shim_signal.h

@@ -4,10 +4,7 @@
 #include <shim_types.h>
 #include <shim_defs.h>
 
-struct shim_signal_handle {
-    /* sigaction */
-    struct __kernel_sigaction * action;
-};
+void sigaction_make_defaults(struct __kernel_sigaction* sig_action);
 
 # define BITS_PER_WORD (8 * sizeof(unsigned long))
 /* The standard def of this macro is dumb */

LibOS/shim/include/shim_thread.h

@@ -28,6 +28,12 @@ struct wake_queue_head {
     struct wake_queue_node* first;
 };
 
+struct shim_signal_handles {
+    struct __kernel_sigaction actions[NUM_SIGS];
+    struct shim_lock lock;
+    REFTYPE ref_count;
+};
+
 DEFINE_LIST(shim_thread);
 DEFINE_LISTP(shim_thread);
 struct shim_thread {
@@ -67,7 +73,7 @@ struct shim_thread {
 
     /* signal handling */
     __sigset_t signal_mask;
-    struct shim_signal_handle signal_handles[NUM_SIGS];
+    struct shim_signal_handles* signal_handles;
     struct atomic_int has_signal;
     struct shim_signal_log * signal_logs;
     bool suspend_on_signal;
@@ -111,6 +117,9 @@ static inline bool is_internal(struct shim_thread *thread)
     return thread->tid >= INTERNAL_TID_BASE;
 }
 
+void get_signal_handles(struct shim_signal_handles* handles);
+void put_signal_handles(struct shim_signal_handles* handles);
+
 void get_thread (struct shim_thread * thread);
 void put_thread (struct shim_thread * thread);
 

LibOS/shim/src/bookkeep/shim_signal.c

@@ -42,6 +42,13 @@
 
 typedef void (*__rt_sighandler_t)(int, siginfo_t*, void*);
 
+void sigaction_make_defaults(struct __kernel_sigaction* sig_action) {
+    sig_action->k_sa_handler = (void*)SIG_DFL;
+    sig_action->sa_flags = 0;
+    sig_action->sa_restorer = NULL;
+    sig_action->sa_mask = 0;
+}
+
 static __rt_sighandler_t default_sighandler[NUM_SIGS];
 
 #define MAX_SIGNAL_LOG 32
@@ -677,33 +684,32 @@ __sigset_t * set_sig_mask (struct shim_thread * thread,
     return &thread->signal_mask;
 }
 
-static __rt_sighandler_t __get_sighandler(struct shim_thread* thread, int sig, bool allow_reset) {
-    assert(locked(&thread->lock));
+static __rt_sighandler_t get_sighandler(struct shim_thread* thread, int sig, bool allow_reset) {
+    lock(&thread->signal_handles->lock);
+    struct __kernel_sigaction* sig_action = &thread->signal_handles->actions[sig - 1];
 
-    struct shim_signal_handle* sighdl = &thread->signal_handles[sig - 1];
-    __rt_sighandler_t handler = NULL;
-
-    if (sighdl->action) {
-        struct __kernel_sigaction * act = sighdl->action;
-        /*
-         * on amd64, sa_handler can be treated as sa_sigaction
-         * because 1-3 arguments are passed by register and
-         * sa_handler simply ignores 2nd and 3rd argument.
-         */
-#ifdef __i386__
-# error "x86-32 support is heavily broken."
+    /*
+     * on amd64, sa_handler can be treated as sa_sigaction
+     * because 1-3 arguments are passed by register and
+     * sa_handler simply ignores 2nd and 3rd argument.
+     */
+#ifndef __x86_64__
+# error "get_sighandler: see the comment above"
 #endif
-        handler = (void*)act->k_sa_handler;
-        if (allow_reset && act->sa_flags & SA_RESETHAND) {
-            sighdl->action = NULL;
-            free(act);
-        }
+
+    __rt_sighandler_t handler = (void*)sig_action->k_sa_handler;
+    if (allow_reset && sig_action->sa_flags & SA_RESETHAND) {
+        sigaction_make_defaults(sig_action);
     }
 
-    if ((void*)handler == SIG_IGN)
-        return NULL;
+    if ((void*)handler == (void*)SIG_IGN) {
+        handler = NULL;
+    } else if ((void*)handler == (void*)SIG_DFL) {
+        handler = default_sighandler[sig - 1];
+    }
 
-    return handler ? : default_sighandler[sig - 1];
+    unlock(&thread->signal_handles->lock);
+    return handler;
 }
 
 static void
@@ -711,9 +717,7 @@ __handle_one_signal(shim_tcb_t* tcb, int sig, struct shim_signal* signal) {
     struct shim_thread* thread = (struct shim_thread*)tcb->tp;
     __rt_sighandler_t handler = NULL;
 
-    lock(&thread->lock);
-    handler = __get_sighandler(thread, sig, /*allow_reset=*/true);
-    unlock(&thread->lock);
+    handler = get_sighandler(thread, sig, /*allow_reset=*/true);
 
     if (!handler)
         return;
@@ -805,7 +809,7 @@ void append_signal(struct shim_thread* thread, int sig, siginfo_t* info, bool ne
 
     /* only want to check if sighandler exists without actual invocation, so don't
      * reset even if SA_RESETHAND */
-    __rt_sighandler_t handler = __get_sighandler(thread, sig, /*allow_reset=*/false);
+    __rt_sighandler_t handler = get_sighandler(thread, sig, /*allow_reset=*/false);
 
     if (!handler) {
         // SIGSTOP and SIGKILL cannot be ignored

LibOS/shim/src/bookkeep/shim_thread.c

@@ -154,6 +154,24 @@ struct shim_thread * alloc_new_thread (void)
     return thread;
 }
 
+static struct shim_signal_handles* alloc_default_signal_handles(void) {
+        struct shim_signal_handles* handles = malloc(sizeof(*handles));
+        if (!handles) {
+            return NULL;
+        }
+
+        if (!create_lock(&handles->lock)) {
+            free(handles);
+            return NULL;
+        }
+        REF_SET(handles->ref_count, 1);
+        for (size_t i = 0; i < ARRAY_SIZE(handles->actions); i++) {
+            sigaction_make_defaults(&handles->actions[i]);
+        }
+
+        return handles;
+}
+
 struct shim_thread * get_new_thread (IDTYPE new_tid)
 {
     if (!new_tid) {
@@ -198,17 +216,8 @@ struct shim_thread * get_new_thread (IDTYPE new_tid)
         thread->exec        = cur_thread->exec;
         get_handle(cur_thread->exec);
 
-        for (int i = 0 ; i < NUM_SIGS ; i++) {
-            if (!cur_thread->signal_handles[i].action)
-                continue;
-
-            thread->signal_handles[i].action =
-                    malloc_copy(cur_thread->signal_handles[i].action,
-                                sizeof(*thread->signal_handles[i].action));
-            if (!thread->signal_handles[i].action) {
-                goto out_error;
-            }
-        }
+        thread->signal_handles = cur_thread->signal_handles;
+        get_signal_handles(thread->signal_handles);
 
         memcpy(&thread->signal_mask, &cur_thread->signal_mask,
                sizeof(sigset_t));
@@ -234,6 +243,11 @@ struct shim_thread * get_new_thread (IDTYPE new_tid)
             get_dentry(thread->root);
             thread->cwd = thread->root;
         }
+
+        thread->signal_handles = alloc_default_signal_handles();
+        if (!thread->signal_handles) {
+            goto out_error;
+        }
     }
 
     if (!create_lock(&thread->lock)) {
@@ -259,8 +273,8 @@ out_error:
     if (thread->cwd) {
         put_dentry(thread->cwd);
     }
-    for (int i = 0; i < NUM_SIGS; i++) {
-        free(thread->signal_handles[i].action);
+    if (thread->signal_handles) {
+        put_signal_handles(thread->signal_handles);
     }
     if (thread->exec) {
         put_handle(thread->exec);
@@ -292,6 +306,28 @@ struct shim_thread * get_new_internal_thread (void)
     return thread;
 }
 
+void get_signal_handles(struct shim_signal_handles* handles) {
+    int ref_count = REF_INC(handles->ref_count);
+#ifdef DEBUG_REF
+    debug("get_signal_handles %p ref_count = %d\n", handles, ref_count);
+#else
+    __UNUSED(ref_count);
+#endif
+}
+
+void put_signal_handles(struct shim_signal_handles* handles) {
+    int ref_count = REF_DEC(handles->ref_count);
+
+#ifdef DEBUG_REF
+    debug("put_signal_handles %p ref_count = %d\n", handles, ref_count);
+#endif
+
+    if (!ref_count) {
+        destroy_lock(&handles->lock);
+        free(handles);
+    }
+}
+
 void get_thread (struct shim_thread * thread)
 {
 #ifdef DEBUG_REF
@@ -338,8 +374,8 @@ void put_thread (struct shim_thread * thread)
             put_dentry(thread->cwd);
         }
 
-        for (int i = 0; i < NUM_SIGS; i++) {
-            free(thread->signal_handles[i].action);
+        if (thread->signal_handles) {
+            put_signal_handles(thread->signal_handles);
         }
 
         if (thread->exec)
@@ -554,6 +590,53 @@ void switch_dummy_thread (struct shim_thread * thread)
 }
 #endif
 
+BEGIN_CP_FUNC(signal_handles)
+{
+    __UNUSED(size);
+    assert(size == sizeof(struct shim_signal_handles));
+
+    struct shim_signal_handles* handles = (struct shim_signal_handles*)obj;
+    struct shim_signal_handles* new_handles = NULL;
+
+    ptr_t off = GET_FROM_CP_MAP(obj);
+
+    if (!off) {
+        off = ADD_CP_OFFSET(sizeof(struct shim_signal_handles));
+        ADD_TO_CP_MAP(obj, off);
+        new_handles = (struct shim_signal_handles*)(base + off);
+
+        lock(&handles->lock);
+
+        memcpy(new_handles, handles, sizeof(*handles));
+        clear_lock(&new_handles->lock);
+        REF_SET(new_handles->ref_count, 0);
+
+        unlock(&handles->lock);
+
+        ADD_CP_FUNC_ENTRY(off);
+    } else {
+        new_handles = (struct shim_signal_handles*)(base + off);
+    }
+
+    if (objp) {
+        *objp = (void*)new_handles;
+    }
+
+}
+END_CP_FUNC(signal_handles)
+
+BEGIN_RS_FUNC(signal_handles)
+{
+    __UNUSED(offset);
+    __UNUSED(rebase);
+    struct shim_signal_handles* handles = (void*)(base + GET_CP_FUNC_ENTRY());
+
+    if (!create_lock(&handles->lock)) {
+        return -ENOMEM;
+    }
+}
+END_RS_FUNC(signal_handles)
+
 BEGIN_CP_FUNC(thread)
 {
     __UNUSED(size);
@@ -587,15 +670,7 @@ BEGIN_CP_FUNC(thread)
         new_thread->robust_list = NULL;
         REF_SET(new_thread->ref_count, 0);
 
-        for (int i = 0 ; i < NUM_SIGS ; i++)
-            if (thread->signal_handles[i].action) {
-                ptr_t soff = ADD_CP_OFFSET(sizeof(struct __kernel_sigaction));
-                new_thread->signal_handles[i].action
-                        = (struct __kernel_sigaction *) (base + soff);
-                memcpy(new_thread->signal_handles[i].action,
-                       thread->signal_handles[i].action,
-                       sizeof(struct __kernel_sigaction));
-            }
+        DO_CP_MEMBER(signal_handles, thread, new_thread, signal_handles);
 
         DO_CP_MEMBER(handle, thread, new_thread, exec);
         DO_CP_MEMBER(handle_map, thread, new_thread, handle_map);
@@ -647,6 +722,10 @@ BEGIN_RS_FUNC(thread)
     if (thread->cwd)
         get_dentry(thread->cwd);
 
+    if (thread->signal_handles) {
+        get_signal_handles(thread->signal_handles);
+    }
+
     DEBUG_RS("tid=%d,tgid=%d,parent=%d,stack=%p,frameptr=%p,tcb=%p,shim_tcb=%p",
              thread->tid, thread->tgid,
              thread->parent ? thread->parent->tid : thread->tid,

LibOS/shim/src/sys/shim_futex.c

@@ -816,6 +816,7 @@ int shim_do_get_robust_list(pid_t pid, struct robust_list_head** head, size_t* l
     if (pid) {
         thread = lookup_thread(pid);
         if (!thread) {
+            /* We only support get_robust_list on threads in the same thread group. */
             return -ESRCH;
         }
     } else {

LibOS/shim/src/sys/shim_sigaction.c

@@ -49,39 +49,21 @@ int shim_do_sigaction(int signum, const struct __kernel_sigaction* act,
         return -EFAULT;
 
     struct shim_thread* cur = get_cur_thread();
-    int err = 0;
 
-    assert(!act || (void*)act->k_sa_handler != (void*)0x11);
+    lock(&cur->signal_handles->lock);
 
-    struct shim_signal_handle* sighdl = &cur->signal_handles[signum - 1];
-
-    lock(&cur->lock);
+    struct __kernel_sigaction* sigaction = &cur->signal_handles->actions[signum - 1];
 
     if (oldact) {
-        if (sighdl->action) {
-            memcpy(oldact, sighdl->action, sizeof(struct __kernel_sigaction));
-        } else {
-            memset(oldact, 0, sizeof(struct __kernel_sigaction));
-            oldact->k_sa_handler = SIG_DFL;
-        }
+        memcpy(oldact, sigaction, sizeof(*oldact));
     }
 
     if (act) {
-        if (!(sighdl->action))
-            sighdl->action = malloc(sizeof(struct __kernel_sigaction));
-
-        if (!(sighdl->action)) {
-            err = -ENOMEM;
-            goto out;
-        }
-
-        memcpy(sighdl->action, act, sizeof(struct __kernel_sigaction));
+        memcpy(sigaction, act, sizeof(*sigaction));
     }
 
-    err = 0;
-out:
-    unlock(&cur->lock);
-    return err;
+    unlock(&cur->signal_handles->lock);
+    return 0;
 }
 
 int shim_do_sigreturn(int __unused) {

LibOS/shim/test/regression/.gitignore

@@ -57,6 +57,7 @@
 /sched
 /select
 /shared_object
+/sigaction_per_process
 /sigaltstack
 /sighandler_reset
 /sigprocmask

LibOS/shim/test/regression/Makefile

@@ -49,6 +49,7 @@ c_executables = \
 	sched \
 	select \
 	shared_object \
+	sigaction_per_process \
 	sigaltstack \
 	sighandler_reset \
 	sigprocmask \
@@ -126,6 +127,7 @@ CFLAGS-futex_requeue = -pthread
 CFLAGS-futex_wake_op = -pthread
 CFLAGS-proc = -pthread
 CFLAGS-spinlock += -I$(PALDIR)/../include/lib -pthread
+CFLAGS-sigaction_per_process += -pthread
 CFLAGS-sigprocmask += -pthread
 
 CFLAGS-attestation += -I$(PALDIR)/../lib/crypto/mbedtls/crypto/include \

LibOS/shim/test/regression/sigaction_per_process.c

@@ -0,0 +1,107 @@
+/*
+ * Test that signal disposition is per-process: set SIGTERM handler in a child thread, but send
+ * SIGTERM signal to the main thread specifically. Verify that signal handler was called in the
+ * main thread and it was called only once.
+ */
+
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+static pid_t gettid(void) {
+    return syscall(SYS_gettid);
+}
+
+static int tkill(pid_t tid, int sig) {
+    return syscall(SYS_tkill, tid, sig);
+}
+
+static pid_t who1 = 0;
+static pid_t who2 = 0;
+
+static void sigterm_handler(int signum) {
+    pid_t v = 0;
+    pid_t my_tid = gettid();
+    if (!__atomic_compare_exchange_n(&who1, &v, my_tid, /*weak=*/0,
+                                     __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)) {
+        __atomic_store_n(&who2, my_tid, __ATOMIC_SEQ_CST);
+    }
+    printf("sigterm_handler called in: %d\n", my_tid);
+}
+
+static int sync_var = 0;
+
+static void set(int x) {
+    __atomic_store_n(&sync_var, x, __ATOMIC_SEQ_CST);
+}
+
+static void wait_for(int x) {
+    while (__atomic_load_n(&sync_var, __ATOMIC_SEQ_CST) != x) {
+        __asm__ volatile("pause");
+    }
+}
+
+static void* f(void* x) {
+    printf("thread id: %d\n", gettid());
+
+    struct sigaction action = { 0 };
+    action.sa_handler = sigterm_handler;
+
+    int ret = sigaction(SIGTERM, &action, NULL);
+    if (ret < 0) {
+        fprintf(stderr, "sigaction failed\n");
+        exit(1);
+    }
+
+    set(1);
+    wait_for(2);
+
+    return x;
+}
+
+int main() {
+    setbuf(stdout, NULL);
+    setbuf(stderr, NULL);
+
+    pthread_t th;
+
+    if (pthread_create(&th, NULL, f, NULL)) {
+        fprintf(stderr, "pthread_create failed: %m\n");
+        return 1;
+    }
+
+    wait_for(1);
+
+    pid_t tid = gettid();
+
+    printf("parent tid: %d\n", tid);
+
+    if (tkill(tid, SIGTERM)) {
+        fprintf(stderr, "tkill failed: %m\n");
+        return 1;
+    }
+
+    set(2);
+
+    if (pthread_join(th, NULL)) {
+        fprintf(stderr, "pthread_join failed: %m\n");
+        return 1;
+    }
+
+    pid_t w1 = __atomic_load_n(&who1, __ATOMIC_SEQ_CST);
+    pid_t w2 = __atomic_load_n(&who2, __ATOMIC_SEQ_CST);
+
+    if (w1 != tid || w2 != 0) {
+        fprintf(stderr, "test failed: (%d, %d)\n", w1, w2);
+        return 1;
+    }
+
+    puts("TEST OK!");
+
+    return 0;
+}

LibOS/shim/test/regression/test_libos.py

@@ -390,6 +390,10 @@ class TC_30_Syscall(RegressionTestCase):
         self.assertIn('Got signal 17', stdout)
         self.assertIn('Handler was invoked 1 time(s).', stdout)
 
+    def test_091_sigaction_per_process(self):
+        stdout, _ = self.run_binary(['sigaction_per_process'])
+        self.assertIn('TEST OK', stdout)
+
 @unittest.skipUnless(HAS_SGX,
     'This test is only meaningful on SGX PAL because only SGX catches raw '
     'syscalls and redirects to Graphene\'s LibOS. If we will add seccomp to '