Conversation
UCRT abort() never raises an exception: it calls the CRT-level SIGABRT handler if one is installed and otherwise __fastfail()s, so the VEH and unhandled-exception filter never saw it and a WTF RELEASE_ASSERT (which is std::abort() in release builds off Darwin), a mimalloc or BoringSSL abort, or any plain C abort() exited 0xC0000409 with no crash report. Install a CRT SIGABRT handler in init() that routes into crash_handler(Abort), and reset it wherever the VEH is torn down (reset_segfault_handler and raise_ignoring_panic_handler). Clear _WRITE_ABORT_MSG so the debug UCRT does not put up its own abort() message first. The abort test hook now calls the real abort() on Windows too, and raiseIgnoringPanicHandler takes an optional signal so the SIGABRT reset is covered.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
|
Updated 4:41 AM PT - Aug 15th, 2026
@robobun, your commit a83d314 is building: Status: reproduced and fixed in this PR (#38860); self-review follow-ups folded in.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes process-global crash-handler state on Windows (CRT SIGABRT slot, _set_abort_behavior) and touches bun run's signal-forwarding path, a human look would still be worthwhile.
What was reviewed:
- The CRT SIGABRT install/teardown pairing:
init()installs,reset_segfault_handler()andraise_ignoring_panic_handler_raw()both reset toSIG_DFL— the SIGABRT_COMPAT (6) aliasing means a forwarded child SIGABRT would otherwise re-enter the handler. handle_abort_windows→crash_handler→ Windowscrash()ends viaExitProcess(3), so no re-entry into UCRTabort(); UCRTraise()resetting the slot before dispatch gives one-shot semantics.js_abortnow calls reallibc::abort()on Windows (previously masked by direct handler invocation);js_raise_ignoring_panic_handlersignal arg goes throughsignal_code_jsc::from_js→ Linux numbering, matching whatraise_ignoring_panic_handler_rawexpects.- New tests pin
process.abort()(exit 134) andfastfail(0xC0000409) as non-reporting; the reworkedraise ignoring panic handlertest now asserts stderr is clean rather than sleeping-then-checking.
Extended reasoning...
Overview
This PR installs a CRT-level SIGABRT handler on Windows so that abort() calls (WTF RELEASE_ASSERT/CRASH(), mimalloc, BoringSSL, etc.) route into Bun's crash reporter instead of silently dying with 0xC0000409. It touches: src/crash_handler/lib.rs (install signal(SIGABRT, handle_abort_windows) + _set_abort_behavior in init(), reset in reset_segfault_handler()), src/bun_core/Global.rs (reset the slot in raise_ignoring_panic_handler_raw so bun run's forwarded SIGABRT doesn't produce a bogus report), src/runtime/api/crash_handler_jsc.rs (js_abort now calls real abort() on Windows; raiseIgnoringPanicHandler accepts a signal arg), plus comment-only updates in Coordinator.rs/parallel.test.ts and new/reworked tests in run-crash-handler.test.ts.
Security risks
None identified. The crash reporter is diagnostic-only; the new hook does not process untrusted input and does not weaken any existing check. _set_abort_behavior(0, _WRITE_ABORT_MSG) only suppresses the debug-CRT dialog.
Level of scrutiny
High. The crash handler is the last-resort diagnostic path and this change installs a process-global CRT signal handler on Windows. It also intersects bun run's signal-forwarding: without the SIG_DFL reset in raise_ignoring_panic_handler_raw, a child's SIGABRT (POSIX number 6, aliased to the CRT SIGABRT slot as SIGABRT_COMPAT) would print a spurious Bun crash report. The author covered that, and I verified the teardown runs on the outer CRASH_HANDLER_INSTALLED gate (not the inner VEH-handle-non-null check). The primary new test is Windows-only, so Linux CI won't exercise it; the PR provides a manual fail-before/pass-after run on Windows Server 2019.
Other factors
- Recursion: UCRT
raise()resets the slot toSIG_DFLbefore invoking the handler, andcrash_handler's Windowscrash()path terminates viaExitProcess(3)(the module-localabort()), so anabort()during report writing fast-fails rather than recursing. - The reworked
raise ignoring panic handlertest is stronger than before (asserts clean stderr and exact exit code / signal instead of.not.toBe(0)after a 2s sleep), and now covers SIGABRT on both platforms. js_raise_ignoring_panic_handlernow usessignal_code_jsc::from_js, which returns the Linux-numberedbun_sys::SignalCode(u8)— consistent with the existingraise_ignoring_panic_handler_rawcallers inrun_command.rs/bunx_command.rs.- I did not find a case where
process.on('SIGABRT')(libuv) would collide with the CRT slot; they are separate tables as the PR states.
Given the crash handler is critical infrastructure and the correctness hinges on UCRT internals I can only verify from documentation, deferring to a human reviewer.
…sh's own status EXCEPTION_BREAKPOINT (x64 int3 from JSC JIT abortWithReason and LLInt break, brk #0xF000 on arm64) was not classified, so those traps died silently like abort() did. arm64 Windows reports JSC's brk #0xbb08 as an illegal instruction, which was already classified. crash() used to ExitProcess(3) for every reported crash, which a parent cannot tell apart from process.exit(3); bun test --parallel therefore stopped classifying a worker's abort() as fatal once the new SIGABRT hook reported it. Exit with the status the unreported crash would have had instead (the exception code, or STATUS_STACK_BUFFER_OVERRUN for the abort/panic/OOM bucket, mirroring terminal_signal on POSIX), and only execute the debug-build breakpoint when a debugger is attached so that status is actually reached. The trap test hook now executes the real instruction on Windows too.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes process-wide crash-handler installation and exit-code semantics on Windows (CRT SIGABRT hook, VEH breakpoint classification, terminal_exit_code), a human look would still be worthwhile.
Checked: the SIGABRT hook is torn down everywhere the VEH is (reset_segfault_handler, raise_ignoring_panic_handler_raw); terminate_windows gates the int3 on IsDebuggerPresent so it can't recurse into the newly-classified EXCEPTION_BREAKPOINT path; terminal_exit_code's statuses line up with is_fatal_windows_exit_code so a reported crash still classifies as a panic in the coordinator; the aarch64 brk #0xbb08 change is POSIX-neutral (still SIGTRAP).
Extended reasoning...
Overview
Windows crash-handler coverage: installs a CRT signal(SIGABRT, ...) hook so UCRT abort() (WTF RELEASE_ASSERT, mimalloc, BoringSSL) reaches the crash reporter instead of __fastfailing silently; classifies EXCEPTION_BREAKPOINT in the VEH so x64 int3 (JSC JIT abortWithReason, LLInt) is reported; replaces the fixed ExitProcess(3) after a report with terminal_exit_code() so parents see the fault's own NTSTATUS. Test hooks (crash_handler_jsc.rs) now execute the real abort()/int3/brk #0xbb08 on Windows instead of calling the handler directly. raiseIgnoringPanicHandler gains an optional signal argument. Coordinator's fatal-exit-code allowlist gains STATUS_DATATYPE_MISALIGNMENT; a comment-only update elsewhere. Tests: new Windows describe.if matrix in run-crash-handler.test.ts, un-skipped Windows case in parallel.test.ts, updated NAPI nodeProcessAborted exit-code list.
Security risks
None identified. This is diagnostic/termination-path code; no untrusted input is parsed. _set_abort_behavior and IsDebuggerPresent are precondition-free by-value calls. The new libc::signal(SIGABRT, ...) slot is process-global but scoped to bun.exe's statically-linked CRT (addon CRTs are unaffected, as documented and pinned by the fastfail test).
Level of scrutiny
High. This is process-wide crash-handling infrastructure: the SIGABRT hook, VEH classification, and exit-code contract all affect how every C++/Rust assertion, JSC trap, and vendored-library abort surfaces to users, CI, and bun test --parallel. The exit-code change (3 → NTSTATUS) is user-visible to any parent classifying by status. The discriminating tests are Windows-only and can't be exercised on this Linux review machine — Windows CI is the actual verification. The PR description is unusually thorough (UCRT source citations, x64/arm64 probes, fail-before output), but the interaction surface (VEH ↔ CRT signal ↔ JSC's own SEH ↔ debugger-attached) is the kind a maintainer familiar with the Windows crash-handler history should sign off on.
Other factors
- The comment-cop bot flagged over-long comments; all were addressed in a83d314 and the threads are resolved.
- Confirmed the SIGABRT reset in
raise_ignoring_panic_handler_rawandreset_segfault_handlermirrors the existing VEH/UEF teardown, sobun runre-raising a child's SIGABRT won't loop into the reporter. - The debug-build
int3interminate_windowsis now guarded byIsDebuggerPresent(), avoiding re-entry into the newly-classifiedEXCEPTION_BREAKPOINThandler when no debugger is attached. terminal_exit_code()maps every reason to a code already inis_fatal_windows_exit_code, so reported crashes still abort a--parallelrun rather than being downgraded to per-file failures.- The aarch64
brk #0→brk #0xbb08change injs_trapis fine on POSIX (both deliver SIGTRAP) and matches the arm64-Windows probe result documented in the PR.
|
#39985 is stacked on this branch. It adds the out of memory case of a reported |
|
A data point from the CI build of #39985, which is built on this branch (https://buildkite.com/bun/bun/builds/102724): on the darwin aarch64 test lane, Unrelated to the test: the binary-size step of that build reports every target 1 to 3.7 MB over the current canary, but the sizes are identical to the ones in #97989, so that is only the age of the base of this branch. |
|
The docs work in #43039 found the third gap of this PR again. I reproduced it on canary The repro has five test files and runs
On Linux x64 (canary Docs coupling: #43039 describes the current behavior in
The PR that merges second must update or remove that sentence. This branch has merge conflicts with |
Problem
abort()inside bun.exe kills the process with exit code0xC0000409and prints nothing: noBun has crashedbanner, no bun.report trace string. WTFRELEASE_ASSERT/CRASH()compiles tostd::abort()in release builds off Darwin (vendor/WebKit/Source/WTF/wtf/Assertions.h:353), so this covers every JSC and Bun C++ assertion, plus aborts in mimalloc, BoringSSL and libuv. POSIX has reported these since crash_handler: catch SIGABRT and SIGTRAP so native aborts and traps get reported #34771.1.4.0-canary.1+a5c86aec7, Windows Server 2019 x64: theprocess.envsnippet from process.env: make a failed env build throw instead of aborting (Windows 0xC0000409 on Bun.$ / Bun.sql / process.env near the stack limit) #38821 exits0xC0000409; stderr holds only JSC's owndataLogline. With theaborttest hook calling a realabort()(below), stderr is empty.abort()(ucrt/startup/abort.cpp) callsraise(SIGABRT)only if a CRT-level SIGABRT handler is installed, otherwise it__fastfails. A fast-fail is not an exception, so the vectored handler and unhandled-exception filter thatinit()installs insrc/crash_handler/lib.rsnever run. Bun never installed a CRT SIGABRT handler.int3(JSC JITabortWithReason(), LLIntbreak,__debugbreak) raisesEXCEPTION_BREAKPOINT, whichclassify_exception_windowsdid not know, so those died silently too (exit0x80000003). The POSIX fix caught SIGABRT and SIGTRAP together; this is its Windows twin.crash()ended every Windows crash inExitProcess(3), which a parent cannot tell fromprocess.exit(3).bun test --parallelclassifies worker deaths by exit status (Coordinator.rs,is_fatal_windows_exit_code), so reporting an abort would have turned a run-aborting0xC0000409worker death (bun test --parallel: abort the run when a Windows worker dies with a fatal NTSTATUS #37129) into a per-file failure.Fix
init()installs a CRT SIGABRT handler (signal(SIGABRT, handle_abort_windows)) that enterscrash_handler(CrashReason::Abort, ..), the same entry point the POSIX SIGABRT handler uses. It is removed wherever the VEH is:reset_segfault_handler()after a report, andraise_ignoring_panic_handler_raw()(bun runre-raising a child's signal; UCRTraise(6)maps onto the same slot, so without that reset a forwarded SIGABRT would print a bogus Bun report)._set_abort_behavior(0, _WRITE_ABORT_MSG)stops the debug UCRT from showing its ownabort() has been calledmessage first; the release UCRT never sets that flag.classify_exception_windowsmapsEXCEPTION_BREAKPOINTtoCrashReason::Trap. It goes through the same out-of-image rule as every other code in the VEH (JIT-pool traps reach the handler through JSC's unwind-info route from crash_handler(windows): let foreign first-chance AVs reach SEH via JSC unwind info #35083), and an attached debugger consumes breakpoints before any handler runs. WTF's own VEH (SignalsWin.cpp) only recovers access violations, illegal instructions and FP exceptions, never breakpoints.crash()exits withCrashReason::terminal_exit_code(), the Windows counterpart ofterminal_signal(): the exception's own status for faults (0xC0000005,0xC000001D,0xC00000FD,0x80000002,0x80000003),STATUS_STACK_BUFFER_OVERRUN(0xC0000409, what an unreported abort or Rust abort exits with) for the abort/panic/OOM bucket. Parents therefore see the same status with or without a report; the coordinator's allowlist gains0x80000002, the one classified code it lacked. The debug-buildint3beforeExitProcessnow only runs when a debugger is attached; unconditionally it terminated the process with0x80000003before the exit code was reached.abort()has and it is consulted before the fast-fail (verified against the UCRT source in the Windows SDK and with a/MTprobe, below); UCRTraise()resets the slot toSIG_DFLbefore calling the handler, so an abort during the report fast-fails instead of recursing (one-shot, likeSA_RESETHAND); nothing else in bun uses the slot (process.on("SIGABRT")is libuv,process.abort()is_exit(134)on Windows, Rust aborts and thefastfailhook are bare__fastfails), and the tests pin all three staying unreported. bun.exe links the UCRT statically, so onlyabort()calls that resolve to bun.exe's CRT (JSC/WTF, Bun, vendored deps) are caught; an addon linked against ucrtbase.dll still fast-fails and still classifies as before.brkwith any immediate it does not define, including WTF's0xbb08that JSC emits everywhere, asSTATUS_ILLEGAL_INSTRUCTION; onlybrk #0xF000(__debugbreak) is a breakpoint. So JSC traps on arm64 were already reported as illegal instructions; the silent-trap gap was x64. Documented onCrashReason::Trap, and the Windows test expects per architecture.crash_handler.abort()and.trap()now execute the realabort()/int3/brk #0xbb08on Windows instead of calling the handler directly (which is what hid this);raiseIgnoringPanicHandler()takes an optional signal.test/cli/run/run-crash-handler.test.ts: adescribe.if(isWindows)matrix asserting banner text and exit status forabort,panic,outOfMemory(9, the low byte of0xC0000409asBun.spawnreports it),segfault(5) andtrap(3 on x64, 0x1D on arm64, with a real fault address);process.abort()(134) andfastfail()(9) print nothing;abortadded to the upload loop; theraise ignoring panic handlertest now covers SIGSEGV and SIGABRT on every platform.test/cli/test/parallel.test.ts: the worker-segfault case runs on Windows too and asserts the coordinator seesexit code 0xC0000005and aborts the run.test/napi/node-napi-tests/test/common/index.js: the Windows abort exit-code list moves from 3 to 9 (verified:spawnSyncof a panicking bun reportsstatus: 9on Windows).src/crash_handler/lib.rsandsrc/bun_core/Global.rsreverted: the abort test fails withReceived: "", everything else passes. Full branch:run-crash-handler.test.ts19 pass / 0 fail,parallel.test.tscrash cases 6 pass,crash-report-command-char.test.ts3 pass.bun bd(ASAN):run-crash-handler.test.ts19 pass / 16 skip,parallel.test.tscrash cases 6 pass,30205.test.ts4 pass.cargo checkofbun_crash_handler,bun_core,bun_sysforx86_64-andaarch64-pc-windows-msvcclean;cargo fmt --check, prettier clean. Deliberately left out, as follow-up material: classifyingIN_PAGE_ERROR/ integer-divide / privileged-instruction exceptions (the SIGBUS/SIGFPE analogs).Background
signal()stores a function pointer in a CRT-global slot;raise()resets the slot and calls it synchronously on the calling thread;SIG_DFLis_exit(3). Unrelated to libuv'suv_signal, which is whatprocess.on(signal)uses on Windows.__fastfail: anint 0x29that makes the kernel terminate the process at once with0xC0000409, skipping all user-mode exception dispatch (VEH, SEH, unhandled-exception filter). It is how UCRTabort()and Rust'sstd::process::abort()end a process on Windows.AddVectoredExceptionHandlerandSetUnhandledExceptionFilter, the hooks the crash handler already had. They only see exceptions, so faults were reported and aborts were not;EXCEPTION_BREAKPOINTis an exception they did see but did not classify.bun test --paralleland bun's CI runner classify on, and whatterminal_exit_code()reproduces. Bun'sBun.spawn/child_processcurrently expose only the low byte of it (hence 9, 5, 3 in the tests); the coordinator reads the full value.UCRT abort(), probes, and fail-before output
ucrt/startup/abort.cpp(Windows SDK 10.0.26100):ucrt/misc/signal.cppraise():SIGABRTandSIGABRT_COMPAT(6) share one action;SIG_DFLis_exit(3); the action is set back toSIG_DFLbefore the user handler is called.x64 probe (
clang-cl,abort()/raise(6)with and without asignal(SIGABRT, h)that_exit(77)s):arm64 probe (Windows 11 arm64, VEH printing the exception code):
Canary
1.4.0-canary.1+a5c86aec7on Windows Server 2019 running the #38821 snippet:Fail-before (this branch minus
lib.rs/Global.rs), debug build, Windows Server 2019:Full branch, same machine:
run-crash-handler.test.ts19 pass / 16 skip / 0 fail;parallel.test.tsprintsa test worker process crashed with exit code 0xC0000005andAbortingfor a worker that went through the crash handler;spawnSyncof a panicking child reportsstatus: 9.no test proof · iteration 0 · 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 test/cli/test/parallel.test.ts test/regression/issue/30205.test.ts