Repository navigation
Fix read-only shared-code SIGSEGV; backport upstream signal fixes - #14
Conversation
Read-only threads share one module's code pages and timed_run_has_timed_out. A peer's timed_run exit guard can clear the flag before it re-enables the revoked pages, so a thread faulting on them chained to the default handler and aborted nodeop. Jump out of any fault in the thread's own code arena instead, and add a regression case that fails on the old handler.
utils.hpp uses fixed-width integer types without including their header. Backport of gofractally/eos-vm#22.
On macOS siglongjmp restores the saved mask with sigprocmask, which is process-wide there, so every VM recovery copied this thread's mask onto the signal-handling thread and left nodeop unable to stop on SIGTERM. Capture the mask before sigsetjmp(dest, 0) and restore it on every exit path. Backport of gofractally/eos-vm#23.
Compile a module, revoke its code pages as the expiring watchdog does, clear timed_run_has_timed_out as a peer's exit guard does, and execute it from a worker thread: the run must unwind into wasm_memory_exception and the module must run again once the pages are re-enabled.
Darwin's ucontext_t points at its machine context, so the zeroed synthetic context made signal_instruction_pointer() dereference null and both handler-level cases crashed on macOS. Carry a real mcontext and set its PC to the fault address.
| // fetch (where both are equal), so a code-generation fault inside the arena now unwinds | ||
| // instead of aborting. | ||
| const void* const interrupted_pc = signal_instruction_pointer(uap); | ||
| if (pc_in_code_memory_range(addr) || (interrupted_pc && pc_in_code_memory_range(interrupted_pc))) |
There was a problem hiding this comment.
High — restore the outer VM ranges across nested calls. invoke_with_signal_handler overwrites the thread-local code_memory_range and memory_range, but restores only signal_dest. A supported host-function reentry into a different backend or module therefore leaves the inner spans installed after returning to outer JIT code. If the outer module then encounters the revoke/clear-before-enable race, its fault address or RIP does not match the stale inner span; with the timeout flag clear, the handler declines and chains to the fatal previous handler. I reproduced this on the PR head: declined=1 jumped=0. Please save and restore both spans on the normal, exception, and longjmp exits and add a two-backend nested regression. The analogous upstream stack-discipline fix is gofractally/eos-vm@76288f5.
There was a problem hiding this comment.
Fixed in a5b4ae2: invoke_with_signal_handler saves both spans before sigsetjmp and restores them on the normal, exception and longjmp exits (same discipline as upstream 76288f5). Added a two-backend nested regression in jit_revoked_code_tests.cpp: the outer span is checked after the host call, and an outer fault after it must still unwind — on the previous head the span check fails and the process terminates.
| // fetch (where both are equal), so a code-generation fault inside the arena now unwinds | ||
| // instead of aborting. | ||
| const void* const interrupted_pc = signal_instruction_pointer(uap); | ||
| if (pc_in_code_memory_range(addr) || (interrupted_pc && pc_in_code_memory_range(interrupted_pc))) |
There was a problem hiding this comment.
Medium — distinguish synchronous faults from directed signals. This shortcut ignores siginfo_t::si_code. A kill with SIGSEGV or pthread_kill with SIGSEGV delivered while the thread executes generated code has SI_USER or SI_TKILL but an interrupted PC inside code_memory_range, so the new branch longjmps into wasm_memory_exception instead of chaining to the prior handler or producing the requested core dump. I reproduced that on the PR head: declined=0 jumped=1. Please gate the location shortcut on synchronous fault provenance while retaining the timeout-flag path below for any deliberately directed timeout signal.
There was a problem hiding this comment.
Fixed in a5b4ae2: the code-arena shortcut now requires a kernel-raised fault — si_code SEGV_MAPERR/SEGV_ACCERR (and the SIGBUS/SIGILL equivalents), spelled out rather than si_code > 0 because Darwin's SI_USER is positive. A kill()/pthread_kill() SIGSEGV keeps the timeout-flag handling below. Added handler cases: kill()-sent with the flag clear is declined; with the flag set it still unwinds.
…ults Review of PR #14 (huangminghuang) found two gaps in the location-based classification. invoke_with_signal_handler overwrote the thread-local code_memory_range and memory_range and restored only signal_dest, so a host function that ran another backend left the inner spans installed; an outer run then faulting on its own revoked pages no longer matched them and, with the timeout flag clear, was declined to the previous handler. Save both spans before sigsetjmp and restore them on the normal, exception and longjmp exits, as upstream 76288f5 does for its protected ranges. The code-arena shortcut also claimed any SIGSEGV whose interrupted PC lay in generated code, including one sent with kill() or pthread_kill(), which then unwound as wasm_memory_exception instead of reaching the previous handler. Claim it only for a kernel-raised fault (si_code SEGV_MAPERR/SEGV_ACCERR and the SIGBUS/SIGILL equivalents); a directed signal keeps the timeout-flag handling below. Tests: a two-backend nested run that checks the outer span is back after the host call and that an outer fault after it still unwinds (process terminates on the previous head), and handler cases for a kill()-sent signal with the flag clear (declined) and set (unwound).
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed the complete PR at a5b4ae2. The nested signal-range state is now restored on normal, exception, and longjmp exits, and the location-based recovery is restricted to synchronous fault codes so directed signals retain their prior handling. The new regressions cover both fixes. Local focused tests (28 assertions / 7 cases), the full suite (1,458 assertions / 262 cases), and the Ubuntu/macOS CI matrix all pass. No remaining actionable findings.
Review of PR #15 (huangminghuang) found a race in the exit guard: it reads the in-progress counters and then clears the timeout flag or re-enables the module's pages, and a new run can register in between. A stale clear or re-enable that lands after that run's watchdog has fired leaves it executing with no timeout in force, and generated code has no cooperative poll to stop it. The first version's entry re-enable block made the stale clear worse: a spurious wasm_memory_exception became an unbounded run. Registration and the exit guard now take timed_run_transition_mutex. Both run in normal thread context, before the watchdog is armed and after it is disarmed, never in a signal handler; the timeout publication (flag store and page revoke) is the only signal-context work and takes no lock. It fires only for a registered run, whose counts keep any exit from clearing or re-enabling while it is in force. The counters become plain integers guarded by the mutex; the flag and the page state stay atomic because the signal path writes them. The entry re-enable block is gone: the state it repaired cannot be observed by a registered run any more. The lock is taken twice per timed_run, at entry and exit, around a few counter updates, and never while the body runs, so runs still execute concurrently. Measured: 30 → 37 ns per timed_run uncontended; with 16 threads doing nothing but enter and exit, 0.9 → 2.8 µs. Tests drive the timeout from the test instead of racing for it. Fired after the entry check, a run reports timeout_exception with the body never executed and the pages revoked, which stay revoked with the flag set while another run of the module is registered; the next run finds the timeout in force and, as the last registered run, restores both on exit. A second case runs one compiled module through a per-thread backend and context each, as nodeop's read-only threads do: a newer run timing out and exiting while the older run is registered leaves the timeout in force, and the older run's exit restores it.
Review of PR #15 (huangminghuang): the two timed_run cases pass with both lock_guards removed, because each orders its runs sequentially. This case holds timed_run_transition_mutex from the test: a run on another thread cannot arm its watchdog until the lock is released, since registration sits behind it, and once inside its body it cannot report completion until the lock is released again, since so does the exit transition. Each half fails without its lock_guard.
Read-only shared-code SIGSEGV fix + upstream signal-handling backports
The crash (wire-sysio CI run 39807473127,
read-only-trx-irreversible-test)Read-only threads share one compiled JIT module: one code arena and the process-wide
timed_run_has_timed_out. On x86_64 an expiringtimed_runinterrupts execution by revoking themodule's code pages (
growable_allocator::disable_code,mprotect PROT_NONE); the executingthread faults on an instruction fetch inside its own
code_memory_rangeand the signal handlerjumps out of the run.
A peer thread's
timed_runexit guard clearstimed_run_has_timed_outbefore it re-enablesthe pages. A run that enters the revoked code in that window faults with the flag already
false.
handle_active_vm_signalclassified the fault by that flag, returnedfalse, and chainedit to the previous handler (wire-sysio's sys-vm-oc
segv_handler, which has no chain of its own)→
SIG_DFL+raise→nodeopaborted. Confirmed from the core: fault address == RIP, insidethe faulting thread's
code_memory_range; flagfalse; a peer mid-enable_code().Commits
Classify revoked-code faults by location, not the shared timeout flag — a fault whose
address or interrupted PC lies in the thread's own code arena is always a VM fault; jump out
regardless of the flag (the predicate reads only thread-local and signal-context data, so it
is immune to the cross-thread race). Includes a handler-level regression case that fails on
the old handler.
Include
<cstdint>in utils.hpp — backport of add cstdint include gofractally/eos-vm#22.Restore the signal mask ourselves after a longjmp recovery — backport of
avoid macOS process-wide signal mask overwrite on longjmp gofractally/eos-vm#23 (macOS
siglongjmprestores the mask process-wide, leaving nodeopunable to stop on SIGTERM after any VM recovery). Behaviour on Linux is unchanged.
Test recovery from revoked JIT code with the timeout flag clear — an integration test
through the real machinery: compile a module, revoke its code as the watchdog does, clear the
flag as a peer's exit guard does, execute from a worker thread, and require the run to unwind
into
wasm_memory_exceptionand the module to run again after re-enabling. Terminates thetest process on the old handler; passes with the fix.
Tighten the comments on the handler and the tests.
Give the handler tests a complete Darwin ucontext — Darwin's
ucontext_tpoints at its machine context; the synthetic context now carries one with the PC set (this is what crashed the two handler-level cases on macOS CI).Restore the outer spans across nested runs; claim only synchronous faults — review fixes:
invoke_with_signal_handlernow saves and restores both thread-local spans on every exit (a nested backend run no longer leaves its spans installed for the outer run), and the code-arena shortcut applies only to kernel-raised faults (si_code), so akill()/pthread_kill()SIGSEGV keeps its previous handling. New tests: nested two-backend recovery; kill()-sent signal declined when no timeout is in force and unwound when one is.Upstream PRs reviewed and not taken
maybe_unique_ptr) — already in this fork.machine_code_writeralready usesstd::get_ifand has no equivalent of the loop that was fixed.interpreter does not account for host-call frames that way; nothing to restore.
THREAD_LOCAL) — a realmulti-threaded data-race fix, but it lives in
external/softfloat.cmake, which this fork doesnot have: Wire builds softfloat from the
softfloatvcpkg port. Tracked separately forwire-vcpkg-registry.Validation
nodeoprebuilt with the exact CI gcc-14 configuration against the patched header (theoriginal build-id was reproduced first); links cleanly.
unit_testsruns of the signals suite and the new JIT recovery test.documented classification tradeoff in
handle_active_vm_signal.Follow-up (not in this PR)
backend.hpptimed_runexit guard: re-enable code before clearing the flag (or gate entry on!code_disabled()) to remove the residual window in which a racing read-only run throws aspurious
wasm_memory_exception. Not a crash with this PR; a correctness nicety.