Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free the alternate signal stack when the main OCaml code / an OCaml thread stops #10726

Merged
merged 4 commits into from
Dec 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions Changes
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ OCaml 4.14.0
(Xavier Leroy and David Allsopp, review by Sébastien Hinderer and
Damien Doligez)

- #10698, #10726: Free the alternate signal stack when the main OCaml
code or an OCaml thread stops
(Xavier Leroy, review by David Allsopp and Damien Doligez)

- #10730, 10731: Fix bug in `Obj.reachable_words` causing a slowdown when called
multiple time (Alain Frisch, report by ygrek, review by Xavier Leroy)

Expand Down
10 changes: 8 additions & 2 deletions manual/src/cmds/intf-c.etex
Original file line number Diff line number Diff line change
Expand Up @@ -1727,8 +1727,8 @@ compilation of OCaml, as the variable "OC_LDFLAGS".
OCaml have been compiled with the "/MD" flag, and therefore
all other object files linked with it should also be compiled with
"/MD".
\item other systems: you may have to add one or more of "-lcurses",
"-lm", "-ldl", depending on your OS and C compiler.
\item other systems: you may have to add one or both of
"-lm" and "-ldl", depending on your OS and C compiler.
\end{itemize}

\paragraph{Stack backtraces.} When OCaml bytecode produced by
Expand Down Expand Up @@ -1778,6 +1778,12 @@ Once a runtime is unloaded, it cannot be started up again without reloading the
shared library and reinitializing its static data. Therefore, at the moment, the
facility is only useful for building reloadable shared libraries.

\paragraph{Unix signal handling.} Depending on the target platform and
operating system, the native-code runtime system may install signal
handlers for one or several of the "SIGSEGV", "SIGTRAP" and "SIGFPE"
signals when "caml_startup" is called, and reset these signals to
their default behaviors when "caml_shutdown" is called. The main
program written in~C should not try to handle these signals itself.

\section{s:c-advexample}{Advanced example with callbacks}

Expand Down
1 change: 1 addition & 0 deletions otherlibs/systhreads/st_stubs.c
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ static ST_THREAD_FUNCTION caml_thread_start(void * arg)
#ifdef NATIVE_CODE
}
#endif
caml_stop_stack_overflow_detection();
/* The thread now stops running */
return 0;
}
Expand Down
4 changes: 3 additions & 1 deletion runtime/caml/signals.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ 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_init_signals(void);
CAMLextern void caml_terminate_signals(void);
CAMLextern void (*caml_enter_blocking_section_hook)(void);
CAMLextern void (*caml_leave_blocking_section_hook)(void);
#ifdef POSIX_SIGNALS
Expand Down
6 changes: 5 additions & 1 deletion runtime/fail_nat.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "caml/stack.h"
#include "caml/roots.h"
#include "caml/callback.h"
#include "caml/signals.h"

/* The globals holding predefined exceptions */

Expand Down Expand Up @@ -70,7 +71,10 @@ void caml_raise(value v)
if (Is_exception_result(v))
v = Extract_exception(v);

if (Caml_state->exception_pointer == NULL) caml_fatal_uncaught_exception(v);
if (Caml_state->exception_pointer == NULL) {
caml_terminate_signals();
caml_fatal_uncaught_exception(v);
}

while (Caml_state->local_roots != NULL &&
(char *) Caml_state->local_roots < Caml_state->exception_pointer) {
Expand Down
3 changes: 3 additions & 0 deletions runtime/signals_byt.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,6 @@ int caml_set_signal_action(int signo, int action)
}

CAMLexport int caml_setup_stack_overflow_detection(void) { return 0; }
CAMLexport int caml_stop_stack_overflow_detection(void) { return 0; }
CAMLexport void caml_init_signals(void) { }
CAMLexport void caml_terminate_signals(void) { }
46 changes: 46 additions & 0 deletions runtime/signals_nat.c
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,36 @@ void caml_init_signals(void)
#endif
}

/* Termination of signal stuff */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the consistent use of the term "stuff" from caml_init_signals 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent use of precise terminology is important :-)


#if defined(TARGET_power) || defined(TARGET_s390x) \
|| defined(HAS_STACK_OVERFLOW_DETECTION)
static void set_signal_default(int signum)
{
struct sigaction act;
sigemptyset(&act.sa_mask);
act.sa_handler = SIG_DFL;
act.sa_flags = 0;
sigaction(signum, &act, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better here to be using the storing the old action from caml_init_signals and then restoring that here? Loosely thinking of library and pooled uses of libasmrun?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's because of library / pooled uses of libasmrun that it's important not to leave OCaml's signal handlers installed. However, I'm not sure about restoring the old action instead of going back to the default action.

If the application has SIGSEGV, etc, handlers installed before starting the OCaml runtime, I assume these handlers have a purpose, and this purpose is broken when the OCaml runtime is started.

So, I think it's pretty safe to assume that the special signals (SIGSEGV, etc) have default behavior before the OCaml runtime is started.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - I guess we could therefore document that in intf-c.etex (but it's of course not new behaviour to be "clobbering" those signal handlers)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of documenting what happens with signal handling. See commit df89f2a in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, modulo a pedantic nit (which I seem to have managed to comment on the commit, rather than the PR!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic nit addressed. I moved the corresponding change to a separate commit.

}
#endif

void caml_terminate_signals(void)
{
#if defined(TARGET_power)
set_signal_default(SIGTRAP);
#endif

#if defined(TARGET_s390x)
set_signal_default(SIGFPE);
#endif

#ifdef HAS_STACK_OVERFLOW_DETECTION
set_signal_default(SIGSEGV);
caml_stop_stack_overflow_detection();
#endif
}

/* Allocate and select an alternate stack for handling signals,
especially SIGSEGV signals.
Each thread needs its own alternate stack.
Expand All @@ -308,3 +338,19 @@ CAMLexport int caml_setup_stack_overflow_detection(void)
return 0;
#endif
}

CAMLexport int caml_stop_stack_overflow_detection(void)
{
#ifdef HAS_STACK_OVERFLOW_DETECTION
stack_t oldstk, stk;
stk.ss_flags = SS_DISABLE;
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;
#endif
}
8 changes: 6 additions & 2 deletions runtime/startup_nat.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "caml/mlvalues.h"
#include "caml/osdeps.h"
#include "caml/printexc.h"
#include "caml/signals.h"
#include "caml/stack.h"
#include "caml/startup_aux.h"
#include "caml/sys.h"
Expand Down Expand Up @@ -91,7 +92,6 @@ struct longjmp_buffer caml_termination_jmpbuf;
void (*caml_termination_hook)(void *) = NULL;

extern value caml_start_program (caml_domain_state*);
extern void caml_init_signals (void);
#ifdef _WIN32
extern void caml_win32_overflow_detection (void);
#endif
Expand All @@ -106,6 +106,7 @@ extern void caml_install_invalid_parameter_handler();
value caml_startup_common(char_os **argv, int pooling)
{
char_os * exe_name, * proc_self_exe;
value res;
char tos;

/* Initialize the domain */
Expand Down Expand Up @@ -152,10 +153,13 @@ value caml_startup_common(char_os **argv, int pooling)
exe_name = caml_search_exe_in_path(exe_name);
caml_sys_init(exe_name, argv);
if (sigsetjmp(caml_termination_jmpbuf.buf, 0)) {
caml_terminate_signals();
if (caml_termination_hook != NULL) caml_termination_hook(NULL);
return Val_unit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you also call caml_terminate_signals in this code path ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted! This code path corresponds to the initial thread calling Thread.exit. The caml_termination_hook will cause the process to terminate. But even in this case it's worth freeing the alternate stack in case a memory leak detector is being used (like they do at Redhat).

Come to think of it, there's also Stdlib.exit ! I added a call to caml_terminate_signals in this case too.

}
return caml_start_program(Caml_state);
res = caml_start_program(Caml_state);
caml_terminate_signals();
return res;
}

value caml_startup_exn(char_os **argv)
Expand Down
1 change: 1 addition & 0 deletions runtime/sys.c
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ CAMLexport void caml_do_exit(int retcode)
#ifdef _WIN32
caml_restore_win32_terminal();
#endif
caml_terminate_signals();
#ifdef NAKED_POINTERS_CHECKER
if (retcode == 0 && caml_naked_pointers_detected) {
fprintf (stderr, "\nOut-of-heap pointers were detected by the runtime.\n"
Expand Down
2 changes: 0 additions & 2 deletions tools/ci/inria/sanitizers/lsan-suppr.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
# ocamlyacc doesn't clean memory on exit
leak:ocamlyacc
# Alternate signal stacks are currently never freed (see #10266)
leak:caml_setup_stack_overflow_detection