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

Allow custom signal handler from non-main thread #2551

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Allow custom signal handler from non-main thread #2551

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions core/shared/platform/common/posix/posix_thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,8 +509,8 @@ mask_signals(int how)
pthread_sigmask(how, &set, NULL);
}

static os_thread_local_attribute struct sigaction prev_sig_act_SIGSEGV;
static os_thread_local_attribute struct sigaction prev_sig_act_SIGBUS;
static struct sigaction prev_sig_act_SIGSEGV;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try keeping the os_thread_local_attribute flag and call wasm_runtime_init_thread_env in the callback of the spawned thread? In that way prev_sig_act_SIGSEGV of that thread will be set.

Per my understanding, each thread can has its sig action, we had better keep the os_thread_local_attribute flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that the sigaction is per process, not per thread:

The sigaction() system call is used to change the action taken by a process on receipt of a specific signal.

(source: https://man7.org/linux/man-pages/man2/sigaction.2.html)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the answer I got from ChapGPT:
image

Not very sure, but if keeping os_thread_local_attribute flag works for you, had better keep it.

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 don't think that's very reliable.

The signal disposition is a per-process attribute: in a
multithreaded application, the disposition of a particular signal
is the same for all threads.

(https://man7.org/linux/man-pages/man7/signal.7.html)
From what I read, signal handers are per process, signal masks are per thread.

If I keep os_thread_local_attribute, it doesn't work. And I can't use wasm_runtime_init_thread_env in that thread since it doesn't import wamr dependencies.
I don't think that's any harm in having the prev sigaction global instead of thread-local.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's merge it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the intention of the wasmtime code seems like restore the signal handler and then make it re-fault.
i'm not sure it it works reliably.
even if it does, it leaves the signal handler modified, which seems broken for our purpose.
maybe it might not be a problem for wasmtime, where "multi-tenancy" is somehow optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

So any suggestion to handle SIG_DFL here? Or keep the code unchanged?

Copy link
Collaborator

Choose a reason for hiding this comment

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

my suggestion is to remove the SIG_DFL (and probably SIG_IGN) cases because apps can hardly rely on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about those cases to be honest, personally I only tested the prev_sig_act->sa_sigaction and prev_sig_act->sa_handler cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so let's just handle the two cases, I will submit another PR to fix it.

static struct sigaction prev_sig_act_SIGBUS;

/* ASAN is not designed to work with custom stack unwind or other low-level \
things. > Ignore a function that does some low-level magic. (e.g. walking \
Expand Down Expand Up @@ -540,11 +540,16 @@ signal_callback(int sig_num, siginfo_t *sig_info, void *sig_ucontext)
if (prev_sig_act && (prev_sig_act->sa_flags & SA_SIGINFO)) {
prev_sig_act->sa_sigaction(sig_num, sig_info, sig_ucontext);
}
else if (prev_sig_act
&& ((void *)prev_sig_act->sa_sigaction == SIG_DFL
|| (void *)prev_sig_act->sa_sigaction == SIG_IGN)) {
else if (prev_sig_act && (void *)prev_sig_act->sa_handler == SIG_DFL) {
/* Default action */
sigaction(sig_num, prev_sig_act, NULL);
}
else if (prev_sig_act && (void *)prev_sig_act->sa_handler == SIG_IGN) {
/* Ignore this signal */
}
else if (prev_sig_act && prev_sig_act->sa_handler) {
prev_sig_act->sa_handler(sig_num);
}
/* Output signal info and then crash if signal is unhandled */
else {
switch (sig_num) {
Expand Down