From 1c4df0352d5f75464acdb27377ccd10d1e0ba10a Mon Sep 17 00:00:00 2001
From: Xavier Leroy <xavierleroy@users.noreply.github.com>
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