fix(signal): honor SA_SIGINFO, SA_RESETHAND, and implement per-thread signal masks - #128
Merged
Merged
Conversation
…nature `__wasm_signal` always invoked the registered handler as `void(int)`, the `sa_handler` form, ignoring `SA_SIGINFO`. A handler installed through `sa_sigaction` takes `(int, siginfo_t *, void *)`, and on wasm the argument count is part of the function type, so dispatching it through the one-argument signature traps with "indirect call type mismatch" and kills the instance rather than silently ignoring the extra arguments the way native ABIs do. Branch on the `SA_SIGINFO` flag, which `__libc_sigaction` already records in the callback table, and call through a matching signature. The signal is delivered by the host rather than raised from a faulting instruction, so there is no machine context to hand over: `ucontext` is null and the `siginfo_t` carries only the fields the host can attest to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`__wasm_signal` never consulted `SA_RESETHAND`, so a handler installed with it stayed installed while running. POSIX resets the disposition to `SIG_DFL` on entry, which is what a handler that re-raises its own signal to reach the default action depends on — the pattern Node's `SignalExit` uses. Without the reset the re-raise re-enters the handler instead, and because every delivery crosses a host/guest boundary the recursion exhausts the host stack and segfaults the runtime process. This was masked until SA_SIGINFO handlers were dispatched correctly, since the mismatched indirect call trapped before any handler body ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sa_mask was stored but never applied and SA_NODEFER was never consulted, so every handler ran as though SA_NODEFER were set and could be re-entered by a second delivery mid-execution. Nothing could be blocked either: pthread_sigmask was a stub returning 0, sigpending returned EINVAL, and the only blocking primitive re-registered the signal callback under the name `__wasm_signal_blocked` -- a name nothing defines, which "worked" only because the host stores no callback for a missing export and then leaves deliveries queued. Give each thread a real mask and pending set in `struct pthread`, consulted by __wasm_signal(): a blocked signal is recorded as pending instead of dispatched, and a handler runs with sa_mask installed plus its own signal unless SA_NODEFER. Deferred deliveries are drained iteratively once the mask is restored, so a handler that re-raises its own signal runs again sequentially rather than nesting. pthread_sigmask and sigpending become real, and block.c becomes plain mask arithmetic. Because signals are only ever delivered at syscall boundaries, unblocking has to flush pending deliveries itself -- there is no interrupt that would come along and do it. Two adjacent bugs had to be fixed for this to work: - __wasi_init_tp() allocates `struct pthread` with aligned_alloc(), which does not zero, so the mask started as heap garbage and made arbitrary signals look blocked. __init_tp() now clears it explicitly. - The host tracks callback registration per instance, but the guest-side "already registered" flag is ordinary memory that survives fork. A forked child therefore looked registered while the host had no callback for it and silently discarded its signals. Previously __restore_sigs() re-registered on every call and _Fork() happened to call it; now that the accident is gone, the child registers explicitly. sa_mask is not yet honored by sigsuspend/sigtimedwait, which remain stubs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check-symbols diffs the built sysroot against the expected symbol lists, and the mask implementation adds two cross-TU helpers, __sig_deliver_pending and __sig_register_callback. Each build configuration has its own list: the base Makefile checks against expected/$(TARGET_TRIPLE), while Makefile-eh selects expected/$(TARGET_TRIPLE)-eh or -ehpic depending on PIC, so all three need the additions. The exnref variants share the lists of their non-exnref counterparts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three POSIX conformance bugs in WASIX signal handling. Each was hidden behind the one before it, so they are best reviewed in order.
1. SA_SIGINFO was ignored
__wasm_signalalways invoked the registered handler asvoid(int)— thesa_handlerform:A handler installed through
sa_sigactiontakes(int, siginfo_t *, void *). On native ABIs calling it through the one-argument signature is harmless — the surplus register arguments are ignored. On wasm the argument count is part of the function type, so thecall_indirecttraps:The runtime maps that to
WasiError::Exit(Errno::Intr)— exit code 27 — killing the instance.Any Node-derived guest hits this, because Node's
RegisterSignalHandlerinstallsSIGINT/SIGTERMhandlers this way. On Wasmer Edge a graceful instance stop (SIGTERMviaInstanceHandle::stop_graceful) therefore killed the instance instead of running its shutdown path, surfacing as a 5xx whenever it raced an in-flight request. Reproduced against the publishedwasmer/edgejs-quickjs:Fixed by branching on
SA_SIGINFO— already recorded in__eintr_handler_callbacksby__libc_sigaction— and dispatching through a matching signature. There is no machine context to hand over, since the host delivers the signal rather than it arising from a faulting instruction:ucontextis null and thesiginfo_tcarries onlysi_signoandsi_code = SI_USER, which is what Node-style handlers use.The dispatch goes through a union rather than a function-pointer cast: a direct cast is rejected by the build's
-Werror,-Wcast-function-type-mismatch, which is the very arity mismatch this patch is about.2. SA_RESETHAND was never implemented
With handlers finally reaching their bodies, the next defect surfaced immediately.
SA_RESETHANDappeared nowhere in the signal path, so a handler installed with it stayed installed while running. POSIX resets the disposition toSIG_DFLon entry, which is what a handler that re-raises its own signal to reach the default action depends on — again theSignalExitpattern. Without it the re-raise re-enters the handler:Every delivery crosses a host/guest boundary, so the recursion exhausts the host stack and segfaults the runtime process (exit 139, core dumped) rather than trapping in the guest.
Fixed by clearing the table entry before dispatch when
SA_RESETHANDis set.3. There was no signal mask at all
sa_maskwas stored and never applied, andSA_NODEFERwas never consulted, so every handler effectively ran asSA_NODEFERand any non-reentrant handler could be re-entered mid-execution. This is also why bug 2 could recurse:edgejs'sRegisterSignalHandlercallssigfillset(&sa.sa_mask), and honoring that alone would have prevented it.Nothing could be blocked either —
pthread_sigmaskwasreturn 0,sigpendingwasreturn EINVAL, andblock.c"blocked" by re-registering the callback as__wasm_signal_blocked, a name nothing defines. That only appeared to work becausecallback_signalstoresNonefor a missing export andprocess_signalsthen skipspop_signals(), leaving deliveries queued host-side.Each thread now owns a real mask and pending set in
struct pthread:__wasm_signalrecords a blocked signal as pending instead of dispatching it.sa_maskinstalled, plus its own signal unlessSA_NODEFER.pthread_sigmaskandsigpendingare real;block.cis plain mask arithmetic.Because signals are only ever delivered at syscall boundaries, unblocking flushes pending deliveries itself — no interrupt will come along to do it. Applications still cannot block
SIGTIMER/SIGCANCEL/SIGSYNCCALL, orpthread_canceland timers would break.Two adjacent bugs had to be fixed to make this work, both worth a look on their own:
struct pthreadwas never zeroed.__wasi_init_tp()allocates it withaligned_alloc(), so the new mask started as heap garbage and made arbitrary signals appear blocked.__init_tp()now clears it explicitly. Anyone adding a field to that struct should know this.fork(), so the child looked registered while the host had no callback and silently discarded its signals. This was previously masked by accident: the old__restore_sigs()re-registered on every call and_Fork()calls it. With that side effect gone the child registers explicitly. It cost me a hang in the pre-existinglibc/signalfixture to find.Not addressed
sigsuspendandsigtimedwaitremainEINVALstubs, sosa_maskis not yet honored there.Testing
Regression tests in wasmerio/wasmer#6838 — seven fixtures, each written red against a sysroot without the corresponding fix and green after. The mask pair is the interesting one: the same program with and without
SA_NODEFER, asserting the handler runs 5 times at nesting depth 1 versus depth 5.Full
wasm_testssuite against sysroots built from this branch withbuild-all.sh: 1100 passed, 0 failed, 71 ignored.End-to-end,
edgejsrebuilt against the patched sysroot (with its ownSA_SIGINFOfix, since it setsa_sigactionwithout the flag):termination signal: Interrupttermination signal: TerminatedTerminating on an unhandled
SIGINT/SIGTERMis the point:SignalExitnow runs its stdio-reset path and lets the default action terminate, instead of dying on a type trap.Review note
Commit 3 changes behavior for programs that work today — signals are now deferred while a handler runs, and
sigprocmaskstops being a no-op. Commits 1 and 2 only allow previously-trapping code to run. Worth weighing separately.🤖 Generated with Claude Code