Allow custom signal handler from non-main thread #2551
Conversation
|
@wenyongh Is there any reason for the second condition in the It forces my custom signal handler to be initialized with |
@eloparco, do you use wasm_runtime_spawn_thread to create the thread? And do you call wasm_runtime_init_thread_env() in at the beginning of the callback of thread spawned? Normally for the thread created by embedder but not runtime, we should call the function to initialize the thread environment, including prev_sig_act_SIGSEGV. Note that eventually os_thread_signal_init is be called. |
Hi, it was implemented according to the reference document: But here it ignored the situation that prev_sig_act->sa_handler is not NULL. The document also mentions: The code may have some issues, per my understanding, the correct handling may be like: /* Forward the signal to next handler if found */
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_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) {
...
}
}Could you help confirm and have a try? Thanks. |
eloparco
force-pushed
the
eloparco/fix-signal-handler-hw-bound-check
branch
from
1c2d8be
to
3f3d75a
Compare
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
This is the answer I got from ChapGPT:
Not very sure, but if keeping os_thread_local_attribute flag works for you, had better keep it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So any suggestion to handle SIG_DFL here? Or keep the code unchanged?
There was a problem hiding this comment.
my suggestion is to remove the SIG_DFL (and probably SIG_IGN) cases because apps can hardly rely on them.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, so let's just handle the two cases, I will submit another PR to fix it.
In my use case, the thread creation is done in a separate part of the code that is independent of the runtime so I can't use WAMR functions there.
Yes, just tried that and it works for me. With your suggestion, if I don't set |
…nce#2551) Remove thread local attribute of prev_sig_act_SIGSEGV/SIGBUS to allow using custom signal handler from non-main thread since in a thread spawned by embedder, embedder may be unable to call wasm_runtime_init_thread_env to initialize them. And fix the handling of prev_sig_act when its sa_handler is SIG_DFL, SIG_IGN, or a user customized handler.
…nce#2551) Remove thread local attribute of prev_sig_act_SIGSEGV/SIGBUS to allow using custom signal handler from non-main thread since in a thread spawned by embedder, embedder may be unable to call wasm_runtime_init_thread_env to initialize them. And fix the handling of prev_sig_act when its sa_handler is SIG_DFL, SIG_IGN, or a user customized handler.
When hardware bound check is enabled and a custom signal handler is defined, it is stored in a thread-local variable.
If the embedder sets up the WAMR runtime in the main thread, spawns a thread and the signal handler is called from the spawn thread, the custom signal handler is not available.