Skip to content

crash_handler: register a sigaltstack on every thread and keep SA_ONSTACK after JSC init - #34775

Open
robobun wants to merge 7 commits into
mainfrom
farm/170abbb9/per-thread-sigaltstack
Open

crash_handler: register a sigaltstack on every thread and keep SA_ONSTACK after JSC init#34775
robobun wants to merge 7 commits into
mainfrom
farm/170abbb9/per-thread-sigaltstack

Conversation

@robobun

@robobun robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What

A native stack overflow on any thread other than the main thread (Worker, HTTP thread, thread-pool thread) died silently with no crash report. After JSC was initialised, the same was true even on the main thread.

Repro

// in a Worker, recurse in native code until the guard page is hit
require("bun:internal-for-testing").crash_handler.stackOverflow();

Before: process terminates with a raw SIGSEGV and empty stderr, no bun.report link.
After: the crash handler (or ASAN's stack-overflow diagnostic in ASAN builds) runs and prints a report.

Cause

  • sigaltstack(2) is per-thread state, but update_posix_segfault_handler() only registered the static 512 KiB alternate stack on whichever thread ran crash_handler::init() (the main thread). Every other Bun-spawned thread inherited the process-wide SA_ONSTACK disposition but had no alternate stack of its own, so on a guard-page fault the kernel had nowhere to push the signal frame and forced the default action.
  • WTF::SignalHandlers::finalize() (run on the first JSC::VM creation) installs its own SIGSEGV/SIGBUS action with sa_flags = SA_SIGINFO, which dropped our SA_ONSTACK bit. After that even the main thread could no longer use its alternate stack.

Fix

  • bun_crash_handler::init_thread() heap-allocates and registers a per-thread alternate stack; called from Source::configure_thread() / configure_thread_no_js(), the common entry point for every Bun-spawned thread. A smaller pre-existing stack (e.g. ASAN's ~56 KiB) is replaced and restored on thread exit.
  • bun_crash_handler::ensure_sa_onstack() re-applies SA_ONSTACK to the SIGSEGV/SIGBUS/SIGILL/SIGFPE dispositions immediately after Zig__GlobalObject__create, without otherwise disturbing WTF's handler (which chains back to ours).
  • update_posix_segfault_handler() now always sets SA_ONSTACK on the action, not just on the first call.

Verification

New native stack overflow produces a crash report tests in test/cli/run/run-crash-handler.test.ts add a stackOverflow() hook that recurses in native code on both the main thread and a Worker, and assert the process emits a crash diagnostic instead of dying silently. With the src/ changes reverted and the hook present, both tests fail with empty stderr.

No-op on Windows (VEH is process-wide).


no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-crash-handler.test.ts

…TACK after JSC init

sigaltstack(2) is per-thread state, but the crash handler only registered the
static 512 KiB alternate stack on whichever thread ran init() (the main
thread). Workers, the HTTP thread and every thread-pool thread inherited the
process-wide SIGSEGV disposition with SA_ONSTACK set but had no alternate stack
of their own, so on a guard-page fault the kernel had nowhere to push the
signal frame and the process died with no bun.report output. Additionally,
WTF::SignalHandlers::finalize() (run on the first JSC::VM creation) installs
its own SIGSEGV/SIGBUS action with sa_flags = SA_SIGINFO, dropping SA_ONSTACK
from the disposition, so after VM init even the main thread could no longer use
its alternate stack.

Fix by:
- adding bun_crash_handler::init_thread(), which heap-allocates and registers a
  per-thread alternate stack; called from Source::configure_thread() and
  configure_thread_no_js(), the common entry point for every Bun-spawned
  thread. A smaller pre-existing stack (e.g. ASAN's ~56 KiB) is replaced and
  restored on thread exit.
- adding bun_crash_handler::ensure_sa_onstack(), which re-applies SA_ONSTACK to
  the SIGSEGV/SIGBUS/SIGILL/SIGFPE dispositions without otherwise disturbing
  whatever handler WTF installed (it chains to ours); called right after
  Zig__GlobalObject__create.
- always setting SA_ONSTACK in update_posix_segfault_handler() rather than only
  on the first call.

Add a stackOverflow() hook to the internal-for-testing crash_handler object
that recurses in native code until the guard page is hit, and tests that both
the main thread and a worker thread produce a crash diagnostic (Bun's
'Segmentation fault at address' or, under ASAN, its stack-overflow report)
instead of dying silently.
@robobun

robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

The new native stack overflow produces a crash report tests pass on every lane in build 76117. Before this change, a native stack overflow on a Worker thread (or on the main thread after the first JSC VM is created) exits with a raw SIGSEGV and empty stderr; after, the crash handler chain runs and prints a report.

CI is red on test/js/node/test/parallel/test-http2-reset-flood.js (ExceptionScope::assertNoException assertion on x64-asan), which is a known main break already being handled separately and is unrelated to this diff's signal-handling changes; all other failures are tagged flaky on main. The diff itself is green.

@robobun

robobun commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:29 AM PT - Jul 20th, 2026

@robobun, your commit 31493a5 has 1 failures in Build #76117 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34775

That installs a local version of the PR into your bun-34775 executable, so you can run:

bun-34775 --bun

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Crash stack handling

Layer / File(s) Summary
POSIX alternate-stack implementation
src/bun_core/lib.rs, src/crash_handler/lib.rs
Adds zeroable support for libc::stack_t, introduces thread-local heap-backed alternate stacks, restores prior configurations on drop, and preserves SA_ONSTACK for fatal signals.
Runtime thread and VM wiring
src/bun_core/Global.rs, src/bun_core/output.rs, src/jsc/JSGlobalObject.rs, src/jsc/VirtualMachine.rs
Initializes crash-handler stacks during thread setup and reapplies SA_ONSTACK during JavaScriptCore and VM initialization.
Native overflow API and tests
src/runtime/api/crash_handler_jsc.rs, test/cli/run/run-crash-handler.test.ts
Exposes a native stackOverflow binding and tests crash diagnostics for main-thread and worker-thread overflows.

Possibly related PRs

  • oven-sh/bun#34024: Updates the same crash-handler test harness for intentional crash diagnostics and report-related environment configuration.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: per-thread altstack registration and restoring SA_ONSTACK after JSC init.
Description check ✅ Passed The description covers the PR purpose and verification, with the required what/fix/verification details despite using different headings.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/cli/run/run-crash-handler.test.ts`:
- Around line 125-130: Trim the rationale comment immediately before the
signal-stack setup to three lines or fewer, preserving only the essential
explanation of per-thread sigaltstack and reapplying SA_ONSTACK after VM
initialization. Do not modify the surrounding implementation or the other
comments.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 214b85d8-7965-4b49-a992-9a37640de3d4

📥 Commits

Reviewing files that changed from the base of the PR and between 5c28061 and 095e502.

📒 Files selected for processing (8)
  • src/bun_core/Global.rs
  • src/bun_core/lib.rs
  • src/bun_core/output.rs
  • src/crash_handler/lib.rs
  • src/jsc/JSGlobalObject.rs
  • src/jsc/VirtualMachine.rs
  • src/runtime/api/crash_handler_jsc.rs
  • test/cli/run/run-crash-handler.test.ts

Comment thread test/cli/run/run-crash-handler.test.ts Outdated
Comment thread test/cli/run/run-crash-handler.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No bugs found in the current revision (the earlier bun.report assertion issue was fixed in ee718b6). Deferring to a human because this is signal-handling / crash-handler infrastructure with a per-thread resource decision worth a maintainer's sign-off.

What was reviewed:

  • init_thread() lifecycle: Drop restores the prior altstack before buf is freed; the ≥SIGALTSTACK_SIZE early-return keeps the main thread's static stack in place.
  • ensure_sa_onstack(): read-modify-write of process-wide sigaction is idempotent across concurrent VM creates; SIG_DFL/SIG_IGN skip is correct on both Linux and macOS field layouts.
  • Per POSIX, SA_ONSTACK on a thread with no altstack falls back to the normal stack, so unconditionally OR-ing it in update_posix_segfault_handler is not a regression.
  • Every configure_thread* caller (thread pool, HTTP client, workers, watchers, waitpid, etc.) now heap-allocates 512 KiB — flagged as a design trade-off, not a bug.
Extended reasoning...

Overview

The PR registers a per-thread 512 KiB heap-backed sigaltstack from Source::configure_thread() / configure_thread_no_js() (the common entry for every Bun-spawned thread), and re-applies SA_ONSTACK to the SIGSEGV/SIGBUS/SIGILL/SIGFPE dispositions after Zig__GlobalObject__create (because WTF's SignalHandlers::finalize() reinstalls those actions with SA_SIGINFO only). It also adds a stackOverflow() internal-for-testing hook and two POSIX subprocess tests covering main-thread and worker-thread native stack overflow.

Security risks

None identified. This is crash-diagnostic infrastructure; no untrusted input is parsed and no privilege boundary is crossed. The signal-disposition rewrite reads the kernel's current sigaction and only ORs in SA_ONSTACK, leaving WTF's handler chain intact.

Level of scrutiny

High. Signal handling is process-global state that interacts with JSC/WTF's own SIGSEGV handler chain and with ASAN's per-thread altstack, and mistakes here manifest as silent lost crash reports or use-after-free of a freed altstack buffer during thread teardown. The Drop ordering in ThreadAltStack looks correct (previous altstack restored via sigaltstack before buf is dropped), but this class of code benefits from a maintainer who knows the WTF signal-trampoline behaviour confirming the reapply point is sufficient (e.g. that nothing later in VM init re-clobbers the flags).

Other factors

  • 512 KiB per thread: configure_thread* is called from ~15 thread entry points including the generic ThreadPool, HTTP client thread, bundler workers, fs watchers, and the waitpid thread. Each now allocates 512 KiB of heap. That's a deliberate trade-off (crash reports on stack overflow vs. memory) that a maintainer should ratify — it may be fine, or they may prefer a smaller per-thread stack with the full 512 KiB reserved for the main thread only.
  • The extern "Rust" link-time symbol pattern for __bun_crash_handler_init_thread mirrors the existing __bun_crash_handler_dump_stack_trace right next to it, so layering is consistent.
  • Tests were not run locally per the PR description ("Deferring to CI"); the previous test-assertion bug I flagged has been addressed. The remaining assertions look sound for both ASAN and non-ASAN lanes.
  • No outstanding reviewer comments; the CodeRabbit nit and my earlier inline finding are both resolved.

robobun and others added 2 commits July 20, 2026 06:48
…or release/ASAN CI

- stackOverflow(): recurse through a black-boxed fn pointer with volatile
  frame I/O so release LLVM can't prove the call is self-recursive and turn
  it into a loop (hung forever on linux-x64-asan).
- init(): call init_thread() on the main thread too so it gets the 512 KiB
  altstack even under ASAN, where reset_on_posix() early-returns and only
  ASAN's ~56 KiB stack was registered.
- test: accept 'Bus error at address' as well as 'Segmentation fault at
  address'; macOS delivers a guard-page fault as SIGBUS.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The earlier test-assertion issue is fixed and I didn't find further bugs, but this touches process-wide signal-handling infrastructure (per-thread heap-backed sigaltstacks, rewriting sigaction flags after WTF installs its handler, ASAN interaction) plus a 512 KiB heap allocation on every Bun-spawned thread — worth a maintainer look.

What was reviewed:

  • ThreadAltStack::drop ordering — sigaltstack is restored before the Box<[u8]> backing buffer is freed, so no window where the kernel points at freed memory.
  • Main-thread double-registration — non-ASAN init() registers the static altstack first, so the subsequent init_thread() sees ss_size >= SIGALTSTACK_SIZE and skips.
  • ensure_sa_onstack() leaves SIG_DFL/SIG_IGN alone and only ORs the flag bit, so concurrent worker-VM creation is idempotent.
  • macOS guard-page → SIGBUS path is covered by the (Segmentation fault|Bus error) at address regex (matches CrashReason::BusError output at src/crash_handler/lib.rs:691).
Extended reasoning...

Overview

Adds per-thread alternate signal stacks so a native stack overflow on any Bun-spawned thread reaches the crash handler instead of dying with empty stderr, and reapplies SA_ONSTACK after JSC's WTF::SignalHandlers::finalize() overwrites the SIGSEGV/SIGBUS disposition. Wires init_thread() into Source::configure_thread{,_no_js}() via a link-time extern "Rust" symbol (same pattern as the existing __bun_crash_handler_dump_stack_trace), adds ensure_sa_onstack() after both Zig__GlobalObject__create sites, exposes a stackOverflow() test hook in bun:internal-for-testing, and adds two POSIX-only tests (main thread + worker).

Security risks

None identified. The change only affects how fatal signals are delivered/handled inside the process; no untrusted input reaches the new code, and the test hook is behind bun:internal-for-testing.

Level of scrutiny

High. Signal handling is process-global state with subtle interactions across ASAN, JSC/WTF, and platform differences (Linux SIGSEGV vs macOS SIGBUS for guard-page faults). The ThreadAltStack Drop path runs during thread-local destruction, and getting restore-before-free wrong would be a use-after-free the kernel could write into. The 512 KiB-per-thread heap allocation is a deliberate memory-footprint tradeoff (thread pools, HTTP threads, workers all pay it) that a maintainer should sign off on.

Other factors

  • My earlier finding (the /bun\.report/ assertion vs empty BUN_CRASH_REPORT_URL) was addressed in ee718b6; a follow-up commit (f74b252) hardened the stackOverflow() hook against release-LLVM tail-call elimination and added the ASAN main-thread init_thread() call.
  • The link-time extern "Rust" dispatch under cfg(not(test)) mirrors the existing __bun_crash_handler_dump_stack_trace right below it, so it doesn't introduce a new link-shape constraint.
  • PR description states "no test proof · Deferring to CI" — the tests are POSIX-gated and branch on isASAN, so behaviour on the full CI matrix (release Linux/macOS × x64/aarch64, debug+ASAN) is the real verification.

…guard page on CI

CI Linux runners set `ulimit -s unlimited` (scripts/bootstrap.sh:499), so the
main thread has no guard page and the native recursion in stackOverflow() never
faults within the 20 s timeout. Worker threads already have a fixed 4 MiB stack.
Wrap both spawns in `sh -c 'ulimit -s 8192 && exec ...'` so the child's initial
stack is bounded at exec time.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No bugs found this pass, but this touches process-wide signal handling and adds a 512 KiB heap allocation on every Bun-spawned thread — worth a human look at the memory/design tradeoff and the cross-platform CI results.

What was reviewed:

  • ThreadAltStack::Drop ordering — restores the prior altstack before buf is freed, and normalizes ss_flags to SS_DISABLE/0 so the queried SS_ONSTACK status bit isn't fed back as a request.
  • ensure_sa_onstack() — leaves SIG_DFL/SIG_IGN alone; sa_flags/SA_ONSTACK types match on both Linux and macOS libc.
  • The earlier bun.report assertion issue was addressed in ee718b6; later commits add the ulimit -s wrapper and ASAN main-thread altstack.
Extended reasoning...

Overview

This PR reworks POSIX alternate-signal-stack handling so that native stack overflows on any thread produce a crash report instead of a silent SIGSEGV. It adds init_thread() (per-thread 512 KiB heap altstack, stored in a thread_local! and restored on drop), ensure_sa_onstack() (re-applies SA_ONSTACK after WTF's SignalHandlers::finalize() clobbers it), wires the former into Source::configure_thread{,_no_js}() via a link-time extern "Rust" symbol, and the latter into both VM-creation paths. A new stackOverflow() test hook and two subprocess tests exercise main-thread and worker-thread guard-page faults.

Security risks

None in the traditional sense — this is crash-diagnostic plumbing. The signal-handling changes are additive (setting a flag bit, registering an altstack) and do not weaken any existing check.

Level of scrutiny

High. Signal handling is process-global, platform-sensitive, and interacts with ASAN, JSC/WTF's own signal machinery, and TLS destruction ordering. The change also introduces a fixed 512 KiB per-thread heap allocation for every thread that goes through configure_thread — that's a memory-footprint decision (demand-paged in practice, but still address-space and RSS under pressure) that a maintainer should sign off on rather than a bot. The tests are POSIX-only and were explicitly not run locally ("no test proof · Deferring to CI"), so correctness on macOS (SIGBUS path, sa_flags layout) and across ASAN/release lanes rests on the CI build that was still in flight at review time.

Other factors

I previously flagged a test assertion that could never match under noReportEnv; that was fixed in ee718b6. Two further commits landed after that (ulimit -s bounding for CI's unlimited-stack runners, and calling init_thread() from init() so ASAN's main thread also gets the full-size altstack). The ThreadAltStack::Drop looks correct (restore-then-free), ensure_sa_onstack() correctly skips SIG_DFL/SIG_IGN, and the extern "Rust" link-time hook follows the existing __bun_crash_handler_dump_stack_trace pattern in the same file. Nothing blocking from my side, but the scope and platform surface put this outside what I'd approve without a human reviewer.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Another instance of this, on a bundler thread pool thread in an ASAN build (linux x64, debug bun bd binary built from b7a0431; the handler setup involved is unchanged on current main). It is fixed by the same change, so adding the evidence here rather than opening a second PR.

Repro (works while #38481 is unmerged, since that PR stops the CSS parser from recursing this deep):

// deep.mjs
require("fs").writeFileSync("/tmp/deep.css", ":is(".repeat(200) + "a" + ")".repeat(200) + "{c:d}");
await Bun.build({ entrypoints: ["/tmp/deep.css"], minify: true });
  • bun-debug deep.mjs: exit 139, nothing on stderr. The fault is a guard page hit on Bun Pool 6 inside the CSS selector parser.
  • bun-debug build /tmp/deep.css --outdir /tmp/out (no JS VM, so WTF::SignalHandlers::finalize() never runs): normal AddressSanitizer: stack-overflow report, exit 1.

Tracing rt_sigaction / sigaltstack in the failing run shows the mechanism this PR describes, and pins down which half matters in ASAN builds:

  • ASAN installs AsanOnDeadlySignal for SIGSEGV/SIGBUS with sa_flags = SA_SIGINFO | SA_ONSTACK | SA_NODEFER, and registers a 0xe300 byte altstack on every thread it sees created, including each Bun Pool N thread.
  • The first VM creation then runs WTF::SignalHandlers::finalize() (vendor/WebKit/Source/WTF/wtf/threads/Signals.cpp:592), which replaces both dispositions with jscSignalHandler and sa_flags = SA_SIGINFO only. Nothing touches them again before the fault.
  • So in ASAN builds the pool thread does have an altstack; the only thing between the guard page fault and the ASAN report (reached through jscSignalHandler chaining to the saved action) is the missing SA_ONSTACK, which ensure_sa_onstack() restores. The per-thread init_thread() altstack is what the release build additionally needs, where pool threads (std::thread in src/threading/ThreadPool.rs) have no altstack at all.

The branch currently shows as conflicting with main. A 3-way apply of the diff onto current main applies src/crash_handler/lib.rs, src/bun_core/* and src/jsc/VirtualMachine.rs cleanly; the conflicts are limited to the hook site in src/jsc/JSGlobalObject.rs, the entry list in src/runtime/api/crash_handler_jsc.rs, and the test file, so a rebase should be mechanical.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

One more data point for the main thread half of this, from a release build (x64 Linux, reports 1.4.0), since the evidence above is from an ASAN build on a pool thread.

Repro (needs a tree without #38481, which stops this particular recursion from getting deep enough):

// inproc.ts
import { cssInternals } from "bun:internal-for-testing";
const d = 500;
cssInternals.minifyTest("@media screen {".repeat(d) + ".t{color:red}" + "}".repeat(d), "", undefined);
$ sh -c 'ulimit -s 1024; BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1 bun inproc.ts'; echo $?
Segmentation fault (core dumped)
139

Nothing on stderr, no crash report. Logging rt_sigaction / sigaltstack from gdb until the fault shows the sequence this PR describes:

sigaltstack thread=1 ss_sp=0x4cffc24 ss_flags=0 ss_size=524288          # crash_handler init, main thread
rt_sigaction sig=11 handler=0x261c7b0 flags=0x9c000004                  # bun: SA_SIGINFO|SA_RESTART|SA_RESETHAND|SA_ONSTACK
rt_sigaction sig=7  handler=0x261c7b0 flags=0x9c000004
rt_sigaction sig=11 handler=0x3555280 flags=0x04000004                  # WTF SignalHandlers::finalize: SA_SIGINFO only
rt_sigaction sig=7  handler=0x3555280 flags=0x04000004

Thread 1 "bun" received signal SIGSEGV
rsp     = 0x7ffc6231ce60
si_addr = 0x7ffc6231ce58                                                # guard page, rsp - 8

So at fault time the main thread still has its 512 KiB alternate stack registered, but the installed SIGSEGV disposition no longer carries SA_ONSTACK, the kernel cannot push the signal frame onto the overflowed stack, and the process is killed with the default action. ensure_sa_onstack() in this PR is what covers it; pool threads (ThreadPool.rs goes through configure_named_thread) additionally need the per-thread altstack from init_thread(), also in this PR.

The branch currently conflicts with main in src/jsc/JSGlobalObject.rs, src/runtime/api/crash_handler_jsc.rs and test/cli/run/run-crash-handler.test.ts (the src/crash_handler/lib.rs part still merges cleanly), so it needs a rebase before it can land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants