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.

Sign up for GitHub

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

Jump to bottom

Dynamically allocate the alternate signal stack #10266

Merged
merged 3 commits into from Mar 5, 2021
Merged

Dynamically allocate the alternate signal stack #10266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
632563b
Dynamically allocate the alternate signal stack
xavierleroy Mar 2, 2021
9f5b312
Typo in comment
xavierleroy Mar 3, 2021
1a6cc60
Update Changes
xavierleroy Mar 5, 2021
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
5 changes: 5 additions & 0 deletions Changes
View file
Open in desktop
Expand Up @@ -69,6 +69,11 @@ Working version
to the debugger via a socket.
(Antonin Décimo, review by Xavier Leroy)

- #10250, #10266: Dynamically allocate alternate signal stacks to
accommodate changes in Glibc 2.34.
(Xavier Leroy, reports by Tomasz Kłoczko and R.W.M. Jones, review by Anil
Madhavapeddy, Stephen Dolan, and Florian Angeletti)

### Code generation and optimizations:

- #9876: do not cache the young_limit GC variable in a processor register.
Expand Down
2 changes: 1 addition & 1 deletion runtime/caml/signals.h
View file
Open in desktop
Expand Up @@ -87,7 +87,7 @@ 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 void caml_setup_stack_overflow_detection(void);
CAMLextern int caml_setup_stack_overflow_detection(void);

CAMLextern void (*caml_enter_blocking_section_hook)(void);
CAMLextern void (*caml_leave_blocking_section_hook)(void);
Expand Down
2 changes: 1 addition & 1 deletion runtime/signals_byt.c
View file
Open in desktop
Expand Up @@ -81,4 +81,4 @@ int caml_set_signal_action(int signo, int action)
return 0;
}

CAMLexport void caml_setup_stack_overflow_detection(void) {}
CAMLexport int caml_setup_stack_overflow_detection(void) { return 0; }
25 changes: 14 additions & 11 deletions runtime/signals_nat.c
View file
Open in desktop
Expand Up @@ -174,8 +174,6 @@ DECLARE_SIGNAL_HANDLER(trap_handler)
#error "CONTEXT_SP is required if HAS_STACK_OVERFLOW_DETECTION is defined"
#endif

static char sig_alt_stack[SIGSTKSZ];

/* Code compiled with ocamlopt never accesses more than
EXTRA_STACK bytes below the stack pointer. */
#define EXTRA_STACK 256
Expand Down Expand Up @@ -269,28 +267,33 @@ void caml_init_signals(void)
#endif

#ifdef HAS_STACK_OVERFLOW_DETECTION
{
stack_t stk;
if (caml_setup_stack_overflow_detection() != -1) {
struct sigaction act;
stk.ss_sp = sig_alt_stack;
stk.ss_size = SIGSTKSZ;
stk.ss_flags = 0;
SET_SIGACT(act, segv_handler);
act.sa_flags |= SA_ONSTACK | SA_NODEFER;
sigemptyset(&act.sa_mask);
if (sigaltstack(&stk, NULL) == 0) { sigaction(SIGSEGV, &act, NULL); }
sigaction(SIGSEGV, &act, NULL);
}
#endif
}

CAMLexport void caml_setup_stack_overflow_detection(void)
/* Allocate and select an alternate stack for handling signals,
especially SIGSEGV signals.
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). */

CAMLexport int 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_flags = 0;
if (stk.ss_sp)
sigaltstack(&stk, NULL);
return sigaltstack(&stk, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

This can now return ENOMEM under two circumstances -- either the malloc failing or the sigaltstack call failing (and the latter should never happen since SIGSTKSZ should never be < MINSIGSTKSZ). That seems right.

Do we need to adjust the invocation of caml_setup_stack_overflow_detection() in otherlibs/systhreads/st_stubs.c:caml_thread_start to do something in the event of it returning -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to adjust the invocation of caml_setup_stack_overflow_detection() in otherlibs/systhreads/st_stubs.c:caml_thread_start to do something in the event of it returning -1?

Perhaps... but I'm not sure what to do!

  1. We could abort the whole program, of course. That's not very subtle.
  2. Or we could disable the handling of SIGSEGV and disable the alternate stack in the new thread, so that it uses its system stack for signal handling, but not the alternate stack of the thread that created it. Other threads would still use their own alternate stacks. That's no longer necessary (if we no longer catch SIGSEGV) but safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about doing this?

caml_thread_uncaught_exception(caml_exn_Out_of_memory);

It prints a message and aborts the thread, but more importantly it does the same thing as any other out-of-memory error happening on that thread.

(If you're feeling particularly keen, you could also allocate the alt stack in the parent thread during caml_thread_new where caml_raise_out_of_memory() is a reasonable thing to do and pass it down via the caml_thread_t. I'm not sure it's worth it, though)

Copy link
Member

Choose a reason for hiding this comment

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

If it was just the main thread, I'd advocate for 1. since a malloc failing so early in the runtime initialisation would either indicate a critical lack of resources (many other future mallocs will fail), or some external thing trying to manipulate a specific condition into occurring (e.g. some malware that might be chaining an attack that wants to turn off stack overflow detection). But this is now for all threads begun with caml_thread_start, so aborting the whole program seems extreme.

The problem with 2. is that it's quite non-deterministic -- I'd hope that I could determine at program startup time if stack overflow detection is available or not (if I understand your suggestion correctly that we turn off overflow detection for the particular thread where malloc fails).

Is there an option 3. where we could simply terminate a thread that attempts to turn on stack overflow detection but fails to do so? This would be extremely rare, but at least the failure to start the thread would be observable by the mutator and so could potentially be handled by the application as with any other reason why a system thread might fail to spawn.

Copy link
Member

Choose a reason for hiding this comment

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

My comment was sent in parallel with @stedolan. Raising Out_of_memory seems ideal to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are nice suggestions, thanks @stedolan! However, caml_exn_Out_of_memory exists only in native code and is not exported... So, this would need some refactoring.

I'm going to table this issue for later work. There are other, more urgent changes in this department, such as freeing the alternate stack when the thread terminates. I'll address these issues later in a separate PR.

Right now, the code does exactly what it did before, i.e. ignore failures when setting up the alternate signal stack in a new thread. So there is no regression, and improvements will come later.

#else
return 0;
#endif
}