From 1c4df0352d5f75464acdb27377ccd10d1e0ba10a Mon Sep 17 00:00:00 2001 From: Xavier Leroy Date: Mon, 22 Aug 2022 16:06:50 +0200 Subject: [PATCH 18/24] More prudent deallocation of alternate signal stack (#11496) After `caml_setup_stack_overflow_detection` is called, a C library can install its own alternate stack for signal handling. Therefore, `caml_stop_stack_overflow_detection` must not free the alternate signal stack block, only the block that `caml_setup_stack_overflow_detection` allocated. Fixes: #11489 --- Changes | 4 +++ otherlibs/systhreads/st_stubs.c | 5 ++-- runtime/caml/signals.h | 4 +-- runtime/signals_byt.c | 4 +-- runtime/signals_nat.c | 47 +++++++++++++++++++++------------ 5 files changed, 41 insertions(+), 23 deletions(-) diff --git a/Changes b/Changes index 6b537edca9..4d1bb10435 100644 --- a/Changes +++ b/Changes @@ -41,6 +41,10 @@ OCaml 4.14 maintenance branch mingw-w64 i686 port. (David Allsopp, review by Xavier Leroy and Sébastien Hinderer) +- #11489, #11496: More prudent deallocation of alternate signal stack + (Xavier Leroy, report by @rajdakin, review by Florian Angeletti) + + OCaml 4.14.0 (28 March 2022) ---------------------------- diff --git a/otherlibs/systhreads/st_stubs.c b/otherlibs/systhreads/st_stubs.c index b7a6a9a6bb..043e07031e 100644 --- a/otherlibs/systhreads/st_stubs.c +++ b/otherlibs/systhreads/st_stubs.c @@ -524,6 +524,7 @@ static ST_THREAD_FUNCTION caml_thread_start(void * arg) { caml_thread_t th = (caml_thread_t) arg; value clos; + void * signal_stack; #ifdef NATIVE_CODE struct longjmp_buffer termination_buf; char tos; @@ -536,7 +537,7 @@ static ST_THREAD_FUNCTION caml_thread_start(void * arg) /* Acquire the global mutex */ caml_leave_blocking_section(); st_thread_set_id(Ident(th->descr)); - caml_setup_stack_overflow_detection(); + signal_stack = caml_setup_stack_overflow_detection(); #ifdef NATIVE_CODE /* Setup termination handler (for caml_thread_exit) */ if (sigsetjmp(termination_buf.buf, 0) == 0) { @@ -550,7 +551,7 @@ static ST_THREAD_FUNCTION caml_thread_start(void * arg) #ifdef NATIVE_CODE } #endif - caml_stop_stack_overflow_detection(); + caml_stop_stack_overflow_detection(signal_stack); /* The thread now stops running */ return 0; } diff --git a/runtime/caml/signals.h b/runtime/caml/signals.h index c6aeebfc78..62b0e7fafa 100644 --- a/runtime/caml/signals.h +++ b/runtime/caml/signals.h @@ -87,8 +87,8 @@ value caml_do_pending_actions_exn (void); value caml_process_pending_actions_with_root (value extra_root); // raises value caml_process_pending_actions_with_root_exn (value extra_root); int caml_set_signal_action(int signo, int action); -CAMLextern int caml_setup_stack_overflow_detection(void); -CAMLextern int caml_stop_stack_overflow_detection(void); +CAMLextern void * caml_setup_stack_overflow_detection(void); +CAMLextern int caml_stop_stack_overflow_detection(void *); CAMLextern void caml_init_signals(void); CAMLextern void caml_terminate_signals(void); CAMLextern void (*caml_enter_blocking_section_hook)(void); diff --git a/runtime/signals_byt.c b/runtime/signals_byt.c index 439fb56404..7cb461ac4d 100644 --- a/runtime/signals_byt.c +++ b/runtime/signals_byt.c @@ -81,7 +81,7 @@ int caml_set_signal_action(int signo, int action) return 0; } -CAMLexport int caml_setup_stack_overflow_detection(void) { return 0; } -CAMLexport int caml_stop_stack_overflow_detection(void) { return 0; } +CAMLexport void * caml_setup_stack_overflow_detection(void) { return NULL; } +CAMLexport int caml_stop_stack_overflow_detection(void * p) { return 0; } CAMLexport void caml_init_signals(void) { } CAMLexport void caml_terminate_signals(void) { } diff --git a/runtime/signals_nat.c b/runtime/signals_nat.c index 443f5d53b6..1dd8289c12 100644 --- a/runtime/signals_nat.c +++ b/runtime/signals_nat.c @@ -254,6 +254,10 @@ DECLARE_SIGNAL_HANDLER(segv_handler) /* Initialization of signal stuff */ +#ifdef HAS_STACK_OVERFLOW_DETECTION +static void * caml_signal_stack = NULL; +#endif + void caml_init_signals(void) { /* Bound-check trap handling */ @@ -278,7 +282,8 @@ void caml_init_signals(void) #endif #ifdef HAS_STACK_OVERFLOW_DETECTION - if (caml_setup_stack_overflow_detection() != -1) { + caml_signal_stack = caml_setup_stack_overflow_detection(); + if (caml_signal_stack != NULL) { struct sigaction act; SET_SIGACT(act, segv_handler); act.sa_flags |= SA_ONSTACK | SA_NODEFER; @@ -314,7 +319,8 @@ void caml_terminate_signals(void) #ifdef HAS_STACK_OVERFLOW_DETECTION set_signal_default(SIGSEGV); - caml_stop_stack_overflow_detection(); + caml_stop_stack_overflow_detection(caml_signal_stack); + caml_signal_stack = NULL; #endif } @@ -323,37 +329,44 @@ void caml_terminate_signals(void) Each thread needs its own alternate stack. The alternate stack used to be statically-allocated for the main thread, but this is incompatible with Glibc 2.34 and newer, where SIGSTKSZ - may not be a compile-time constant (issue #10250). */ + may not be a compile-time constant (issue #10250). + Return the dynamically-allocated alternate signal stack, or NULL + if an error occurred. + The returned pointer must be passed to [caml_stop_stack_overflow_detection]. +*/ -CAMLexport int caml_setup_stack_overflow_detection(void) +CAMLexport void * caml_setup_stack_overflow_detection(void) { #ifdef HAS_STACK_OVERFLOW_DETECTION stack_t stk; - stk.ss_sp = malloc(SIGSTKSZ); - if (stk.ss_sp == NULL) return -1; stk.ss_size = SIGSTKSZ; + stk.ss_sp = malloc(stk.ss_size); + if (stk.ss_sp == NULL) return NULL; stk.ss_flags = 0; if (sigaltstack(&stk, NULL) == -1) { free(stk.ss_sp); - return -1; + return NULL; } + return stk.ss_sp; +#else + return NULL; #endif - /* Success (or stack overflow detection not available) */ - return 0; } -CAMLexport int caml_stop_stack_overflow_detection(void) +CAMLexport int caml_stop_stack_overflow_detection(void * signal_stack) { #ifdef HAS_STACK_OVERFLOW_DETECTION stack_t oldstk, stk; stk.ss_flags = SS_DISABLE; + stk.ss_sp = NULL; /* not required but avoids a valgrind false alarm */ + stk.ss_size = SIGSTKSZ; /* macOS wants a valid size here */ if (sigaltstack(&stk, &oldstk) == -1) return -1; - /* If caml_setup_stack_overflow_detection failed, we are not using - an alternate signal stack. SS_DISABLE will be set in oldstk, - and there is nothing to free in this case. */ - if (! (oldstk.ss_flags & SS_DISABLE)) free(oldstk.ss_sp); - return 0; -#else - return 0; + /* Check whether someone else installed their own signal stack */ + if (!(oldstk.ss_flags & SS_DISABLE) && oldstk.ss_sp != signal_stack) { + /* Re-activate their signal stack. */ + sigaltstack(&oldstk, NULL); + } + free(signal_stack); #endif + return 0; } -- 2.37.0.rc2