From 46195e5f673a93fa597b03962bb87f0806d03ce2 Mon Sep 17 00:00:00 2001 From: borysp Date: Wed, 13 May 2020 13:56:47 +0200 Subject: [PATCH] [LibOS] Make the signal disposition a per-process attribute --- LibOS/shim/include/shim_signal.h | 5 +- LibOS/shim/include/shim_thread.h | 11 +- LibOS/shim/src/bookkeep/shim_signal.c | 56 ++++---- LibOS/shim/src/bookkeep/shim_thread.c | 127 ++++++++++++++---- LibOS/shim/src/sys/shim_futex.c | 1 + LibOS/shim/src/sys/shim_sigaction.c | 30 +---- LibOS/shim/test/regression/.gitignore | 1 + LibOS/shim/test/regression/Makefile | 2 + .../test/regression/sigaction_per_process.c | 107 +++++++++++++++ LibOS/shim/test/regression/test_libos.py | 4 + 10 files changed, 265 insertions(+), 79 deletions(-) create mode 100644 LibOS/shim/test/regression/sigaction_per_process.c diff --git a/LibOS/shim/include/shim_signal.h b/LibOS/shim/include/shim_signal.h index 9e42af2c..df81046a 100644 --- a/LibOS/shim/include/shim_signal.h +++ b/LibOS/shim/include/shim_signal.h @@ -4,10 +4,7 @@ #include #include -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 */ diff --git a/LibOS/shim/include/shim_thread.h b/LibOS/shim/include/shim_thread.h index 5796e7fd..6210c301 100644 --- a/LibOS/shim/include/shim_thread.h +++ b/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); diff --git a/LibOS/shim/src/bookkeep/shim_signal.c b/LibOS/shim/src/bookkeep/shim_signal.c index 5c9463c6..6cd9cc0f 100644 --- a/LibOS/shim/src/bookkeep/shim_signal.c +++ b/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 diff --git a/LibOS/shim/src/bookkeep/shim_thread.c b/LibOS/shim/src/bookkeep/shim_thread.c index d1eae450..e18fc647 100644 --- a/LibOS/shim/src/bookkeep/shim_thread.c +++ b/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, diff --git a/LibOS/shim/src/sys/shim_futex.c b/LibOS/shim/src/sys/shim_futex.c index 41796e81..b750db6e 100644 --- a/LibOS/shim/src/sys/shim_futex.c +++ b/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 { diff --git a/LibOS/shim/src/sys/shim_sigaction.c b/LibOS/shim/src/sys/shim_sigaction.c index 5f7184e8..03ac8240 100644 --- a/LibOS/shim/src/sys/shim_sigaction.c +++ b/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) { diff --git a/LibOS/shim/test/regression/.gitignore b/LibOS/shim/test/regression/.gitignore index 96826e4d..1f003aaf 100644 --- a/LibOS/shim/test/regression/.gitignore +++ b/LibOS/shim/test/regression/.gitignore @@ -57,6 +57,7 @@ /sched /select /shared_object +/sigaction_per_process /sigaltstack /sighandler_reset /sigprocmask diff --git a/LibOS/shim/test/regression/Makefile b/LibOS/shim/test/regression/Makefile index aa2e6382..82f9c406 100644 --- a/LibOS/shim/test/regression/Makefile +++ b/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 \ diff --git a/LibOS/shim/test/regression/sigaction_per_process.c b/LibOS/shim/test/regression/sigaction_per_process.c new file mode 100644 index 00000000..1f812684 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include + +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; +} diff --git a/LibOS/shim/test/regression/test_libos.py b/LibOS/shim/test/regression/test_libos.py index 4fbe4433..9404f2c8 100644 --- a/LibOS/shim/test/regression/test_libos.py +++ b/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 '