crash_handler(windows): walk the fault CONTEXT via RtlVirtualUnwind - #35074
Conversation
WalkthroughChangesWindows crash handling now seeds stack walking from the exception Windows crash unwinding
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:06 AM PT - Jul 23rd, 2026
❌ @robobun, your commit d3098e4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35074That installs a local version of the PR into your bun-35074 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
b0a0151 resolves the two prior-round findings: the sp-advance guard is gone (so ARM64 zero-stack unwinds and directly-recursive stacks are no longer truncated), and is_valid_memory now gates the x64 leaf [Rsp] deref. I re-checked termination after dropping the guard — next_pc == 0, the ARM64 Lr == control_pc leaf check, and the n < out.len() cap together bound every path, and the earlier null-call / recursion cases stay fixed. Remaining item is the inline nit on is_valid_memory's Windows predicate.
Extended reasoning...
b0a0151 addressed both open findings from the previous round. I verified the new loop cannot spin (x64 leaf always advances Rsp; ARM64 leaf breaks on Lr == control_pc; RtlVirtualUnwind termination is Pc == 0; n < out.len() is the hard cap) and does not reintroduce the earlier null-call or recursive-frame truncations. The CONTEXT layouts are pinned by static size/offset asserts. The one remaining nit — is_valid_memory on Windows passes MEM_RESERVE/PAGE_GUARD — is narrow and non-blocking; leaving the approve/merge call to a human given this is hand-rolled Windows unwinder FFI verified only on x64.
Windows 11 ARM64 verification (build 10.26100)Before (main's 6 junk frames; the 256-byte tolerance scan found no match so the trim never fired. After (b0a0151, No handler or ntdll-dispatch frames. On Windows Server 2019 x64 (10.17763), the old tolerance-trim path happens to work for this case because |
|
@robobun address all comments |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 147-157: Update the frame-count assertion in the crash-handler
test to require at least 7 captured frames, matching the six frames inspected by
frameAddrs.slice(1, 7). Keep the existing high-address grouping assertion
unchanged.
🪄 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: e53ce5da-a903-4bca-bcd3-8edbdd7f1835
📒 Files selected for processing (5)
src/bun_core/debug.rssrc/crash_handler/lib.rssrc/runtime/api/crash_handler_jsc.rssrc/windows_sys/externs.rstest/cli/run/run-crash-handler.test.ts
|
@Jarred-Sumner all review comments addressed as of a390bd2:
Verified at 2417a88 (last functional change before the two nits in a390bd2):
Overlaps with #34814 at |
There was a problem hiding this comment.
No new findings — 069e6f7 addressed the last open nit (test now asserts max−min span < 2 GiB instead of grouping by high-32, which removes the HEASLR straddle flake). Deferring to a human for the final sign-off: this is a rewrite of the Windows VEH stack walker with hand-rolled CONTEXT layouts and arch-gated unsafe code, and the ARM64 path can only be exercised on hardware CI doesn't cover on every lane.
Checked this round: the new span assertion still fails-before on the ARM64 repro (ntdll-dispatch frames at 4–6 span the >10 GiB EXE↔ntdll gap) and cannot false-red intra-image; the CONTEXT size/offset static asserts pin both arch layouts; the tightened is_valid_memory predicate has no other Windows callers whose semantics change (the only other caller is MemoryAccessor::read's fallback, which wants the same tightening).
Extended reasoning...
Overview
Rewrites the Windows branch of capture_from_context (src/bun_core/debug.rs) from RtlCaptureStackBackTrace + tolerance-trim to RtlLookupFunctionEntry + RtlVirtualUnwind seeded from the fault CONTEXT. Adds hand-rolled CONTEXT structs for x64 and ARM64 plus RUNTIME_FUNCTION and the two Rtl externs in src/windows_sys/externs.rs (with size_of/offset_of static asserts). Passes ContextRecord through TraceSeed::Fault in src/crash_handler/lib.rs. Adds a segfaultInDll test hook and a Windows-debug-only regression test.
Security risks
None. The crash handler runs after a fatal fault, on process-local state only; no external input is parsed and no privilege boundary is crossed. The one raw deref (*(Rsp as *const u64)) is now gated on 8-alignment + VirtualQuery commit/protection checks, and a nested fault is bounded by PANIC_STAGE.
Level of scrutiny
High. This is arch-cfg-gated unsafe code that runs inside a VEH handler on a crashing thread, with hand-rolled Win32 ABI struct layouts for two architectures. A layout or termination-condition mistake degrades every Windows crash report. It has been through five review rounds (11 findings, all addressed and verified on Win11 ARM64 + Server 2019 x64), so the remaining risk is the class of things only real Windows execution catches — which is exactly why a human with Windows context should approve.
Other factors
All prior inline threads are resolved; the last one (4 GiB-window grouping in the test) was addressed in 069e6f7 by switching to a max−min span bound, which was one of the two fixes I suggested. The bug-hunting system found nothing new this run. The PR description flags an overlap with #34814 that whichever lands second will need to rebase — a maintainer should sequence that.
|
CI (build 77635): all Windows lanes green (x64 build+test, aarch64 build+test). Remaining failures are all tagged
Ready for merge. |
|
@robobun resolve all the conflicts |
handle_segfault_windows previously captured the stack from inside the VEH handler with RtlCaptureStackBackTrace and then trimmed by scanning for an address near the fault PC. That trim only works when RtlCaptureStackBackTrace can unwind through KiUserExceptionDispatcher back to the faulting frame, which is Windows-version dependent. When it cannot (observed in Sentry for a fault inside CRYPTSP.dll on a 1.4.0 build), the trace is just the handler's own frames plus the fault PC, and none of the bun callers are recorded. This mirrors the POSIX path: seed the walk from the saved register context instead of from inside the handler. On Windows that is the EXCEPTION_POINTERS ContextRecord the VEH receives, walked with RtlLookupFunctionEntry + RtlVirtualUnwind. The handler's frames are never in the chain and faults in external DLLs unwind into their bun callers. Adds CONTEXT / RUNTIME_FUNCTION / RtlLookupFunctionEntry / RtlVirtualUnwind to bun_windows_sys with static size/offset assertions for both x64 and arm64, and a segfaultInDll test hook that faults inside ntdll.dll so the Windows test can assert the captured trace reaches the bun callers.
…e ENTRIES.len(), tighten test assertion - Remove the top-of-loop control_pc==0 check so a fault with Rip/Pc==0 (indirect call through a null function pointer) falls into the leaf branch and recovers the caller from [Rsp]/Lr instead of returning [0]. - Replace the next_pc==control_pc stuck-unwind guard with a stack-pointer advance check: two directly-recursive frames share a return PC but not an Sp, so comparing PCs truncated recursive stacks after two frames. - Pass ENTRIES.len() to create_empty_object so the inline-capacity hint can't drift. - Test: assert frames 1..7 all share one high-32 value. The old path left three ntdll-dispatch frames at indices 4-6 on Windows 11 ARM64 so this set has two members there; the new path keeps frames 1..7 in bun's image. Drops the hi(0)!=hi(1) assertion, which could false-red when HEASLR places ntdll and bun in the same 4 GiB window.
The ARM64 leaf step (Pc = Lr) correctly leaves Sp unchanged, so a top-of-loop sp <= prev_sp check truncated the walk one iteration later. Check sp-advance only after RtlVirtualUnwind (the only path that can get stuck), and guard the ARM64 leaf branch against Lr == control_pc instead.
On ARM64 RtlVirtualUnwind can legitimately leave Sp unchanged (fault at prolog offset 0, or a .pdata-bearing zero-stack leaf: bl/blr writes Lr without pushing). The sp_after <= sp_before check truncated the trace to [fault PC] in that case; confirmed on Windows 11 ARM64 where ntdll's RtlFillMemory is a zero-stack leaf. Match the reference RtlVirtualUnwind loop: terminate on next_pc == 0 and the buffer cap only. Also guard the x64 leaf-branch [Rsp] deref with is_valid_memory(), same convention as StackIterator::next on the POSIX side.
State != MEM_FREE passes MEM_RESERVE (no backing) and MEM_COMMIT pages with PAGE_NOACCESS or PAGE_GUARD (the stack guard page after EXCEPTION_STACK_OVERFLOW), so the guard did not actually make the following deref safe. Require MEM_COMMIT with a readable protection and no PAGE_NOACCESS/PAGE_GUARD bit, matching Crashpad/Breakpad's SafeReadMemory. Also benefits MemoryAccessor::read's existing fallback.
…l to fixture usage
The >> 32n grouping can false-red when bun.exe straddles a 4 GiB boundary (HEASLR aligns the EXE base to 64 KiB, not 4 GiB). The suggested >> 36n widening loses the fail-before signal entirely: on the Windows 11 ARM64 before/after data in this PR, ntdll (0x7ffd...) and bun.exe (0x7ff6...) share the same high-36 value. Assert the invariant directly: the span of frames 1..6 fits inside one image (< 2 GiB), vs the >10 GiB gap to the system-DLL region when ntdll-dispatch frames are interleaved.
069e6f7 to
c365892
Compare
|
Rebased onto main (c365892). Conflicts were mechanical:
Re-verified on Windows 11 ARM64 at c365892: |
… Pc nor Sp changed On x64 RtlVirtualUnwind sets Rip = 0 at end-of-chain; on ARM64 it can leave the CONTEXT unchanged instead. rust-lang/backtrace-rs handles this with ip == prev_ip && sp == prev_sp. Safe for the cases an Sp-only or Pc-only check truncates: ARM64 zero-stack unwinds change Pc (to Lr) but not Sp, and directly-recursive frames share Pc but never Sp.
Prior art comparisonCrashpad and Breakpad are out-of-process: they snapshot raw stack memory and unwind offline in the processor, so neither contains an in-process
The leaf handling matches CoreCLR (the most complete reference). backtrace-rs and Go don't do manual leaf unwind because Rust/Go emit One change from the research (3d160d8): adopted backtrace-rs's |
…n block The windows-sys-link-cfg source lint from #35084 requires every #[link] in bun_windows_sys to use the cfg_attr form. The block is already #[cfg(windows)] (CONTEXT only exists there), so the cfg_attr is redundant but matches the enforced pattern.
There was a problem hiding this comment.
No new findings on the post-rebase revision. All 11 prior review threads are addressed; the span-based test assertion (c365892) and the backtrace-rs (pc==prev && sp==prev) end-of-chain check (3d160d8) both look correct. Deferring the final call to a human — this is hand-rolled Windows unwinding FFI (CONTEXT layouts, RtlVirtualUnwind loop) in the crash handler path, and while it's been verified on both x64 and ARM64, it warrants maintainer sign-off.
What was reviewed
- CONTEXT struct layouts pinned by static
size_of/offset_ofasserts against winnt.h values for both arches - x64 leaf deref now guarded by 8-alignment + tightened
is_valid_memory(MEM_COMMIT + readable + !GUARD/!NOACCESS) - Termination check matches backtrace-rs;
n < out.len()remains the hard cap - Rebase merge (abort/trap entries,
cfg_attr(windows, link),ENTRIES.len()capacity hint) — checked each conflict site
Extended reasoning...
Overview
Rewrites the Windows branch of capture_from_context in src/bun_core/debug.rs to walk the stack via RtlLookupFunctionEntry + RtlVirtualUnwind seeded from the fault CONTEXT, replacing the previous RtlCaptureStackBackTrace-from-inside-the-handler + tolerance-trim approach. Adds CONTEXT (x64 + ARM64), RUNTIME_FUNCTION, M128A, PAGE_* constants, and the two Rtl externs to src/windows_sys/externs.rs. Passes ContextRecord through TraceSeed::Fault in src/crash_handler/lib.rs. Adds a segfaultInDll test hook that faults inside ntdll!RtlFillMemory, plus a Windows-only debug-build test asserting frames 1..7 span < 2 GiB (i.e. all in bun.exe, not interleaved with ntdll dispatch frames). Tightens is_valid_memory on Windows to require MEM_COMMIT + readable protection + no PAGE_GUARD/PAGE_NOACCESS.
Security risks
None identified. This is diagnostic-only code that runs after a fatal fault, on the way to process termination. The one raw deref (*(ctx.Rsp as *const u64) in the x64 leaf path) is now guarded by alignment + VirtualQuery commit/protection checks, and a nested fault is bounded by PANIC_STAGE to a degraded abort rather than a hang or corruption. The new FFI struct layouts are pinned by compile-time size_of/offset_of assertions matching winnt.h, so an ABI mismatch would fail the build rather than corrupt memory at runtime.
Level of scrutiny
High. The crash handler is exactly the code that must not itself crash, and this replaces its Windows stack-walk with hand-rolled FFI against undocumented-in-parts ntdll surface. That said, the approach mirrors CoreCLR / backtrace-rs (per the prior-art table posted in-thread), the struct layouts have static assertions, and the author manually verified on Windows Server 2019 x64 and Windows 11 ARM64 with before/after symbolized traces. CI passed on both Windows lanes. This has been through five automated review rounds with 11 findings all addressed.
Other factors
- Two maintainers (Jarred-Sumner, dylan-conway) have engaged but neither has approved yet; dylan-conway's last action was requesting a rebase, which was done.
- Since my last inline finding (08:01Z on 07-22), the changes were: test assertion switched to a 2 GiB span check (addressing my HEASLR-straddle nit correctly — my suggested
>> 36nwas wrong and the author explained why), the end-of-chain check adopted backtrace-rs's(pc==prev && sp==prev)(safer than pc-only for ARM64), a mechanical rebase over #35084/#34771, and a lint fix. - The new test is
isWindows && isDebug-gated so it only runs on the two Windows CI lanes; on Server 2019 the old code also passed, so it's a regression guard for the new path rather than a fails-before proof (the fails-before evidence is Sentry data + the ARM64 manual run in-thread). - Overlaps with #32871 and #34814 at the same call sites; whichever lands second needs a small rebase.
Not approving because this is not a simple/mechanical change — it's low-level platform FFI in a must-not-fail path — but I have no outstanding concerns.
|
Build 78416: Source lints pass. All 16 Windows test shards (x64 + aarch64) passed;
Ready to merge. |
…C unwind info Replaces the previous SCOPE_TABLE walk heuristic with a deterministic design: - VEH returns CONTINUE_SEARCH when the fault PC is outside bun.exe's image (Go's isgoexception, CoreCLR's RhpVectoredExceptionHandler). Stack overflow is always claimed since no foreign __except recovers from it and dispatch itself costs guard-page stack. - JSC now registers RtlAddGrowableFunctionTable unwind info for its JIT pool (oven-sh/WebKit#315) with a language-specific SEH handler that routes to Bun__crashHandlerFromJSCFrame; that's the deterministic catch point for unguarded foreign faults under JIT frames. LLInt is not covered (Windows only consults static .pdata for in-module PCs; needs offlineasm .seh_* emission, follow-up). - SetUnhandledExceptionFilter as the remaining backstop. - All three handlers seed capture_from_context with the fault CONTEXT so the RtlVirtualUnwind walk from #35074 applies to each. - WebKit bumped to autobuild-preview-pr-315-ed1c14e9. Four Windows tests: IsBadReadPtr survives (SEH-guarded probe), RtlFillMemory crash-reports (unguarded), RtlLookupFunctionEntry resolves a JIT PC (validates the hand-encoded unwind bytes), JIT-warm then FFI fault after clearing UEF still reports (isolates the JSC handler). The CRYPTSP 0xE8 sentinel + NULL-hProv analysis and V8/SpiderMonkey/ python-etwtrace prior art are in oven-sh/WebKit#315.
…C unwind info (#35083) ## What Bun's Vectored Exception Handler intercepts every first-chance access violation process-wide and treats it as fatal. Windows system code and injected third-party DLLs (AV/EDR hooks, BeyondTrust PGHook.dll, virtualization guest tools) deliberately raise AVs inside `__try`/`__except` as part of normal operation; VEH runs before SEH, so Bun kills the process for what the callee was about to recover from. Sentry groups [BUN-3PJM](https://bun-p9.sentry.io/issues/7573578898/), [BUN-2V6E](https://bun-p9.sentry.io/issues/7403380795/), [BUN-3K05](https://bun-p9.sentry.io/issues/7559009286/), [BUN-3K2N](https://bun-p9.sentry.io/issues/7559259017/) are all this one crash (~18.8k events, 1,299 machines): BeyondTrust's `PGHook.dll` hooks `MoveFileExW`, passes a `NULL` `HCRYPTPROV` to `CryptCreateHash`, `CRYPTSP.dll` validates via `cmp [rcx+0E8h], 11111111h` under SEH, and Bun's VEH reports the `0xE8` probe as a segfault. Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056. ## Fix Three handlers, each deterministic, no heuristic parsing: 1. **VEH** (`handle_segfault_windows`): only claim the fault when `ExceptionAddress` is inside `bun.exe`'s own image. Otherwise return `CONTINUE_SEARCH` so frame-based SEH can run. Matches Go's [`isgoexception`](https://github.com/golang/go/blob/master/src/runtime/signal_windows.go) and CoreCLR NativeAOT's [`RhpVectoredExceptionHandler`](https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/Runtime/EHHelpers.cpp). Stack overflow is always claimed here: no foreign `__except` recovers from it in practice, and SEH dispatch itself costs guard-page stack. 2. **JSC SEH handler** (`Bun__crashHandlerFromJSCFrame`, via `JSC::setJITExceptionHandlerWin`): JSC now registers `RtlAddGrowableFunctionTable` unwind info for its fixed JIT pool ([oven-sh/WebKit#315](oven-sh/WebKit#315)), with a language-specific handler. When SEH dispatch reaches a JIT frame with an unhandled fault, that handler calls this function, which crash-reports. This is the deterministic catch point for unguarded faults under JIT frames, on real Windows and on Wine. LLInt is not yet covered: it lives in image `.text` and Windows only consults static `.pdata` for in-module PCs, so covering it needs build-time `.seh_*` emission in offlineasm (follow-up; see the comment in `ExecutableAllocator.cpp`). 3. **UEF** (`handle_unhandled_exception_windows`, via `SetUnhandledExceptionFilter`): backstop for anything no SEH handler claimed and no JIT frame caught. All three seed `capture_from_context` with the fault `CONTEXT`, so #35074's `RtlVirtualUnwind` walk applies to each. ## Verification Repro (canary `5b98630ac`, Server 2019): ```sh bun -e "require('bun:ffi').dlopen('kernel32.dll',{IsBadReadPtr:{args:['usize','usize'],returns:'i32'}}).symbols.IsBadReadPtr(0xE8,8)" ``` Before: `panic(main thread): Segmentation fault at address 0xE8`. After: exits 0. Four Windows tests in `run-crash-handler.test.ts`: - `IsBadReadPtr(0xE8, 8)` survives (SEH-guarded probe) - `RtlFillMemory(0xE8, 8, 0)` still crash-reports (unguarded) - `RtlLookupFunctionEntry` returns non-null for a JIT pool PC (validates the WebKit-side unwind-info registration) - JIT-warm a function (`jitPolicyScale=0`) then `SetUnhandledExceptionFilter(0)` and fault via FFI from inside it; crash is still reported, isolating `jscJITSEHHandler` as the catch point ## Prior art V8 [`RegisterNonABICompliantCodeRange`](https://github.com/v8/v8/blob/main/src/diagnostics/unwinding-info-win64.cc), SpiderMonkey [`RegisterExecutableMemory`](https://searchfox.org/firefox-main/source/js/src/jit/ProcessExecutableMemory.cpp), [microsoft/python-etwtrace](https://github.com/microsoft/python-etwtrace/blob/main/src/etwtrace/_etwtrace.c), and Steve Dower's guidance in [python/cpython#126910](python/cpython#126910) ("`RtlAddGrowableFunctionTable` is actually the only one that works") all converge on this design. Go issue [golang/go#56082](golang/go#56082) describes the exact VEH-vs-SEH failure class. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
Problem
handle_segfault_windowsreadsExceptionAddressfor frame 0 but then captures the rest of the stack from inside the handler withRtlCaptureStackBackTrace, trimming the handler's own frames by scanning for an address within 256 bytes of the fault PC. That trim only works whenRtlCaptureStackBackTracecan unwind throughKiUserExceptionDispatcherback to the faulting frame. When it cannot, the capture is just[fault PC, capture_from_context, crash_handler, handle_segfault_windows, ntdll]and none of the bun callers are recorded.Observed in Sentry (BUN-3K2N and siblings): 1180 events in the last 30 days on
bun@1.4.0+*builds wherecapture_from_contextis visible in the stack, 88% Windows ARM64 / 12% x64. Example trace for a fault insideCRYPTSP.dll:On Windows Server 2019 x64 the old path happens to work (RtlCaptureStackBackTrace does unwind through the dispatcher there), so this is not reproducible on every Windows version.
Fix
Mirror the POSIX path: seed the walk from the saved register context instead of from inside the handler. The VEH receives
EXCEPTION_POINTERS::ContextRecord; pass that throughTraceSeed::Faultand walk withRtlLookupFunctionEntry+RtlVirtualUnwind. The handler's frames are never in the chain, and faults in external DLLs unwind into their bun callers regardless of Windows version or architecture. This is the same approach Crashpad, Breakpad and the Sentry native SDK use.src/windows_sys/externs.rs: addCONTEXT(x64 and ARM64 layouts with static size/offset assertions),RUNTIME_FUNCTION,RtlLookupFunctionEntry,RtlVirtualUnwind,UNW_FLAG_NHANDLER.src/bun_core/debug.rs: rewrite the Windows branch ofcapture_from_contextto copy the CONTEXT and loopRtlLookupFunctionEntry/RtlVirtualUnwind, with leaf-function handling for both arches.src/crash_handler/lib.rs: passinfo.ContextRecordthrough instead offp: 0.src/runtime/api/crash_handler_jsc.rs: add asegfaultInDlltest hook that faults insidentdll!RtlFillMemoryso the test has a fault PC outside bun.exe.Verification
Built on Windows x64, ran
segfaultInDll, and symbolized the captured addresses:No
capture_from_context/crash_handler/handle_segfault_windowsframes.cargo checkclean onx86_64-pc-windows-msvc,aarch64-pc-windows-msvc, and the host.The new test is Windows-only; on Server 2019 both the old and new code produce a correct trace for this case, so the test acts as a regression guard for the new path rather than a fail-before proof. The fail-before evidence is the Sentry data above.
Touches the same files as #32871 (frameless leaf handling) but is orthogonal; whichever lands second will need a small rebase.
no test proof · iteration 3 · 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