From f5a503c86299b657a708890a7660930f0df9f2cc Mon Sep 17 00:00:00 2001 From: Marius Gerbershagen Date: Sat, 6 Jan 2018 17:58:59 +0100 Subject: [PATCH] fix segmentation faults when a signal is queued for a thread whose environment is write protected If a thread is interrupted while interrupts are disabled by C, then the signal is queued and the environment is write protected by mprotect. If another thread then calls queue_signal, it will try to write in the protected environment, leading to a segmentation fault. Since mprotect can only protect whole memory pages, we need to allocate the pending interrupts and the signal queue in a separate struct. --- src/c/main.d | 9 +++++---- src/c/unixint.d | 28 ++++++++++++++-------------- src/h/external.h | 13 ++++++++++--- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/c/main.d b/src/c/main.d index 684b16f17..b8437dadf 100755 --- a/src/c/main.d +++ b/src/c/main.d @@ -162,10 +162,10 @@ ecl_init_env(cl_env_ptr env) env->method_cache = ecl_make_cache(64, 4096); env->slot_cache = ecl_make_cache(3, 4096); - env->pending_interrupt = ECL_NIL; + env->interrupt_struct->pending_interrupt = ECL_NIL; { int size = ecl_option_values[ECL_OPT_SIGNAL_QUEUE_SIZE]; - env->signal_queue = cl_make_list(1, ecl_make_fixnum(size)); + env->interrupt_struct->signal_queue = cl_make_list(1, ecl_make_fixnum(size)); } init_stacks(env); @@ -239,6 +239,7 @@ _ecl_alloc_env(cl_env_ptr parent) } # endif #endif + output->interrupt_struct = ecl_alloc_unprotected(sizeof(*output->interrupt_struct)); { size_t bytes = cl_core.default_sigmask_bytes; if (bytes == 0) { @@ -257,8 +258,8 @@ _ecl_alloc_env(cl_env_ptr parent) * are activated later on by the thread entry point or init_unixint(). */ output->disable_interrupts = 1; - output->pending_interrupt = ECL_NIL; - output->signal_queue_spinlock = ECL_NIL; + output->interrupt_struct->pending_interrupt = ECL_NIL; + output->interrupt_struct->signal_queue_spinlock = ECL_NIL; return output; } diff --git a/src/c/unixint.d b/src/c/unixint.d index 0a3b470d2..b7c0eb9fa 100644 --- a/src/c/unixint.d +++ b/src/c/unixint.d @@ -367,7 +367,7 @@ si_handle_signal(cl_object signal_code, cl_object process) static void handle_all_queued(cl_env_ptr env) { - while (env->pending_interrupt != ECL_NIL) { + while (env->interrupt_struct->pending_interrupt != ECL_NIL) { handle_signal_now(pop_signal(env), env->own_process); } } @@ -375,20 +375,20 @@ handle_all_queued(cl_env_ptr env) static void queue_signal(cl_env_ptr env, cl_object code, int allocate) { - ECL_WITH_SPINLOCK_BEGIN(env, &env->signal_queue_spinlock) { + ECL_WITH_SPINLOCK_BEGIN(env, &env->interrupt_struct->signal_queue_spinlock) { cl_object record; if (allocate) { record = ecl_list1(ECL_NIL); } else { - record = env->signal_queue; + record = env->interrupt_struct->signal_queue; if (record != ECL_NIL) { - env->signal_queue = ECL_CONS_CDR(record); + env->interrupt_struct->signal_queue = ECL_CONS_CDR(record); } } if (record != ECL_NIL) { ECL_RPLACA(record, code); - env->pending_interrupt = - ecl_nconc(env->pending_interrupt, + env->interrupt_struct->pending_interrupt = + ecl_nconc(env->interrupt_struct->pending_interrupt, record); } } ECL_WITH_SPINLOCK_END; @@ -398,17 +398,17 @@ static cl_object pop_signal(cl_env_ptr env) { cl_object record, value; - if (env->pending_interrupt == ECL_NIL) { + if (env->interrupt_struct->pending_interrupt == ECL_NIL) { return ECL_NIL; } - ECL_WITH_SPINLOCK_BEGIN(env, &env->signal_queue_spinlock) { - record = env->pending_interrupt; + ECL_WITH_SPINLOCK_BEGIN(env, &env->interrupt_struct->signal_queue_spinlock) { + record = env->interrupt_struct->pending_interrupt; value = ECL_CONS_CAR(record); - env->pending_interrupt = ECL_CONS_CDR(record); + env->interrupt_struct->pending_interrupt = ECL_CONS_CDR(record); /* Save some conses for future use, to avoid allocating */ if (ECL_SYMBOLP(value) || ECL_FIXNUMP(value)) { - ECL_RPLACD(record, env->signal_queue); - env->signal_queue = record; + ECL_RPLACD(record, env->interrupt_struct->signal_queue); + env->interrupt_struct->signal_queue = record; } } ECL_WITH_SPINLOCK_END; return value; @@ -599,7 +599,7 @@ handler_fn_prototype(process_interrupt_handler, int sig, siginfo_t *siginfo, voi the_env = ecl_process_env(); if (zombie_process(the_env)) return; - if (!Null(the_env->pending_interrupt)) { + if (!Null(the_env->interrupt_struct->pending_interrupt)) { if (interrupts_disabled_by_C(the_env)) { set_guard_page(the_env); } else if (!interrupts_disabled_by_lisp(the_env)) { @@ -726,7 +726,7 @@ handler_fn_prototype(sigsegv_handler, int sig, siginfo_t *info, void *aux) /* We access the environment when it was protected. That * means there was a pending signal. */ if (((char*)the_env <= (char*)info->si_addr) && - ((char*)info->si_addr <= (char*)(the_env+1))) + ((char*)info->si_addr <= (char*)(the_env+sizeof(*the_env)+1))) { mprotect(the_env, sizeof(*the_env), PROT_READ | PROT_WRITE); the_env->disable_interrupts = 0; diff --git a/src/h/external.h b/src/h/external.h index 79e96a0a3..845eb74ab 100755 --- a/src/h/external.h +++ b/src/h/external.h @@ -102,9 +102,10 @@ struct cl_env_struct { cl_object big_register[3]; cl_object own_process; - cl_object pending_interrupt; - cl_object signal_queue; - cl_object signal_queue_spinlock; + /* The objects in this struct need to be writeable from a + different thread, if environment is write-protected by + mprotect. Hence they have to be allocated seperately. */ + struct ecl_interrupt_struct *interrupt_struct; void *default_sigmask; /* The following is a hash table for caching invocations of @@ -145,6 +146,12 @@ struct cl_env_struct { #endif }; +struct ecl_interrupt_struct { + cl_object pending_interrupt; + cl_object signal_queue; + cl_object signal_queue_spinlock; +}; + #ifndef __GNUC__ #define __attribute__(x) #endif