crash_handler(windows): let foreign first-chance AVs reach SEH via JSC unwind info - #35083
Conversation
|
Warning Review limit reached
Next review available in: 4 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 (8)
WalkthroughChangesWebKit revision
Windows crash handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:21 PM PT - Jul 23rd, 2026
❌ @robobun, your commit 76c80a8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35083That installs a local version of the PR into your bun-35083 --bun |
|
Reproduced on Windows x64 (Server 2019): bun -e "require('bun:ffi').dlopen('kernel32.dll',{IsBadReadPtr:{args:['usize','usize'],returns:'i32'}}).symbols.IsBadReadPtr(0xE8,8)"canary The unguarded counterpart (
The new tests are All review threads resolved. The diff is entirely |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The UEF backstop in 8cbdcfb addresses the napi/ffi coverage gap from the earlier review — thanks. Beyond the inline findings, I also checked whether GetModuleHandleExW(FROM_ADDRESS) on the VEH continue-path can deadlock under the loader lock — it takes the PEB loader lock read-side but the continue path only runs for out-of-image PCs and matches the existing StackLine::from_address usage, so ruled out.
Extended reasoning...
The author pushed commit 8cbdcfb ("wip: add UEF backstop") in response to the earlier review, adding SetUnhandledExceptionFilter(handle_unhandled_windows) so unguarded external-DLL faults still reach crash_handler on second-chance dispatch. That closes the regression I flagged. The two remaining inline findings (UEF has no test; sibling teardown in raise_ignoring_panic_handler_raw not updated) are new to this revision and posted separately. A finder also raised a potential loader-lock deadlock from calling GetModuleHandleExW inside the VEH; verifiers ruled it out — the call only fires on the out-of-image continue path and mirrors the pre-existing symbolication call pattern. Not approving: crash-handler code, "wip:" commit prefix, and REVIEW.md requires the load-bearing UEF clause to break at least one test when deleted.
2469a92 to
6c6ef82
Compare
|
I arrived at the same root cause independently while investigating BUN-3PJM / BUN-2V6E / BUN-3K05 (same crash, different stack depths; ~18.8k events total, 1,299 unique machines). A few additions that might be useful for the description or review:
Same DLL shows up in adoptium/adoptium-support#429 and git-for-windows/git#4830. The fix is identical either way. Why cmp dword ptr [rcx+0E8h], 11111111h ; no NULL check first
Standalone repro (no BeyondTrust required), verified on Server 2019: AddVectoredExceptionHandler(0, veh); // veh records then returns CONTINUE_SEARCH
CryptCreateHash((HCRYPTPROV)0, CALG_SHA_256, 0, 0, &hh);
// -> veh observes AV at 0xE8, SEH recovers, call returns FALSE gle=87Same call with a VEH that treats AV as fatal (current bun) kills the process at Prior art for the PC-in-own-image check:
I had a simpler variant (bounds-only check + UEF backstop) on |
7dbe576 to
16e9b0b
Compare
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/crash_handler/lib.rs (1)
2067-2074: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
ExceptionInformation[1]behindNumberParameters >= 2.
EXCEPTION_ACCESS_VIOLATIONonly definesExceptionInformation[1]when at least two parameters are present; malformed or software-raised records can leave it undefined and misclassify the crash.Suggested change
bun_sys::windows::EXCEPTION_ACCESS_VIOLATION => { + if record.NumberParameters < 2 { + return None; + } CrashReason::SegmentationFault(record.ExceptionInformation[1]) }🤖 Prompt for 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. In `@src/crash_handler/lib.rs` around lines 2067 - 2074, In classify_exception_windows, guard access to record.ExceptionInformation[1] for EXCEPTION_ACCESS_VIOLATION with record.NumberParameters >= 2; otherwise avoid constructing SegmentationFault from undefined data and preserve the existing fallback classification behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/crash_handler/lib.rs`:
- Around line 2067-2074: In classify_exception_windows, guard access to
record.ExceptionInformation[1] for EXCEPTION_ACCESS_VIOLATION with
record.NumberParameters >= 2; otherwise avoid constructing SegmentationFault
from undefined data and preserve the existing fallback classification behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 760ee263-9380-4622-92bd-b66dc266876b
📒 Files selected for processing (2)
src/crash_handler/lib.rssrc/sys/windows/mod.rs
💤 Files with no reviewable changes (1)
- src/sys/windows/mod.rs
There was a problem hiding this comment.
Confirmed afab2b0 correctly addresses all six prior nits: the disposition::ExceptionContinueSearch split, .take(24) scan depth, EXCEPTION_UNWIND guard + WINDOWS_SEGFAULT_HANDLE.is_null() teardown gate in Bun__crashHandlerFromJSCFrame, the StackOverflow exemption from the out-of-image gate, and the JIT-frame test now clearing UEF via FFI to isolate the JSC handler. Two remaining nits inline; still needs a human look given the cross-repo dependency (WEBKIT_VERSION is on a preview autobuild tag pending WebKit#315 merge).
Extended reasoning...
afab2b0 is a fix-up commit responding to my prior review round. I walked each of the six earlier findings against the current diff: the bun_sys::windows::disposition module with the two-enum warning comment is present and used; capture_from_context scans 24 frames with the updated comment naming the UEF path; Bun__crashHandlerFromJSCFrame now gates on both EXCEPTION_UNWIND and the null VEH handle; the VEH's out-of-image check exempts CrashReason::StackOverflow; and the fourth Windows test calls SetUnhandledExceptionFilter(0) before faulting so it fails if setJITExceptionHandlerWin is deleted. All landed as suggested. The two new inline findings this run are cosmetic (a missing ThrowScope in Windows-only test-helper C++, and four comments still claiming LLInt unwind-info coverage that commit 8f0c63e's own message says was dropped). Not approving: this is process-wide Windows exception handling with a paired oven-sh/WebKit change still on a preview tag.
There was a problem hiding this comment.
No new findings this pass — the last two rounds of nits (afab2b0, 707846d) landed cleanly. Deferring to a human for the merge decision: this is a redesign of the Windows crash-handling architecture (VEH → JSC SEH → UEF) coupled to oven-sh/WebKit#315, and WEBKIT_VERSION still points at the preview tag autobuild-preview-pr-315-ed1c14e9, which per the earlier thread needs bumping to the merged main-branch hash before landing.
What was reviewed:
- VEH out-of-image gate,
EXCEPTION_DISPOSITIONvs filter-constant enum split, unwind-phase / teardown guards inBun__crashHandlerFromJSCFrame. exe_image_range()PE-header offsets (e_lfanewat 0x3C,SizeOfImageat NT+4+20+56) — match winnt.h for IMAGE_NT_HEADERS64.- Throw-scope discipline in
jsFunctionStartOfFixedExecutableMemoryPool; LLInt comment drift across the four sites. - The four Windows tests: subprocess-drain pattern,
SetUnhandledExceptionFilter(0)isolation of the JSC handler, andRtlLookupFunctionEntrysmoke test.
Extended reasoning...
Overview
This PR reworks Bun's Windows crash handler from a single greedy Vectored Exception Handler into three cooperating handlers: (1) a VEH that only claims faults whose PC is inside bun.exe's own image, (2) a language-specific SEH handler wired into JSC's JIT-pool unwind info via the companion oven-sh/WebKit#315, and (3) a SetUnhandledExceptionFilter backstop. The goal is to stop Bun's VEH from killing the process on first-chance access violations that injected DLLs (BeyondTrust PGHook, CRYPTSP handle probes, EDR agents) raise-and-recover under their own __try/__except. Nine files touched: crash_handler/lib.rs (core logic), sys/windows/mod.rs and windows_sys/externs.rs (new Win32 surface: exe_image_range, SetUnhandledExceptionFilter, GetModuleHandleW, EXCEPTION_DISPOSITION constants), bun_core/Global.rs and debug.rs (teardown symmetry, deeper dispatcher-frame trim), ZigGlobalObject.cpp (setJITExceptionHandlerWin wiring), JSCTestingHelpers.cpp (test hook), webkit.ts (version bump), and four new Windows tests.
Security risks
Low. The change narrows which first-chance exceptions Bun claims; the concern is observability regression (missing a crash report), not privilege escalation or data exposure. exe_image_range() reads the process's own mapped PE header at fixed offsets — not attacker-controlled input. The EXCEPTION_UNWIND and post-teardown guards in Bun__crashHandlerFromJSCFrame prevent re-entry during unwind/teardown.
Level of scrutiny
High. Crash handling is process-wide, runs at fault time on a possibly-corrupted stack, and errors here either silently drop crash telemetry or spuriously kill user processes. The design went through ~12 iterations on this thread, pivoting from a heuristic SCOPE_TABLE-walking predicate to the deterministic image-range + JSC-unwind-info approach. That evolution is well-reasoned and matches Go/CoreCLR/V8/SpiderMonkey prior art, but it's exactly the kind of architectural decision a maintainer should sign off on.
Other factors
- Cross-repo dependency not finalized:
WEBKIT_VERSION = "autobuild-preview-pr-315-ed1c14e9"is a preview-PR artifact tag. The author acknowledged (2026-07-23T05:39:43Z) it will be bumped to the merged commit hash before landing — that hasn't happened yet, so a fresh clone would fetch an ephemeral release. This alone blocks auto-approval. - Windows-only tests: all four new tests are
describe.if(isWindows); the PR evidence marker says they didn't run locally and defer to CI. Windows CI results should be green before merge. - Prior review rounds: I raised ~20 inline findings across earlier revisions; every one is now marked resolved and the fixes are visible in the current diff (stack-overflow exemption,
ExceptionContinueSearchnamed constant, unwind-phase guard, teardown-null gate,.take(24)trim depth, throw scope on the BigInt helper, LLInt comment sync). The bug-hunting system found nothing new this run.
…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.
707846d to
4566938
Compare
| // The Windows crash handler is a Vectored Exception Handler, which sees every | ||
| // first-chance exception process-wide before frame-based SEH does. Third-party | ||
| // DLLs injected into the process (AV/EDR agents such as BeyondTrust's | ||
| // PGHook.dll, virtualization guest tools, shell extensions) routinely raise | ||
| // and then handle access violations under SEH as part of normal operation. | ||
| // The VEH must let those through rather than treating them as a fatal crash. | ||
| // `IsBadReadPtr` is the canonical example: it probes its argument inside a | ||
| // `__try`/`__except` in kernel32, so the AV it raises is inside a system DLL | ||
| // and is immediately swallowed by that DLL's own SEH. | ||
| // | ||
| // See https://github.com/oven-sh/bun/issues/10056 (Carbon Black), | ||
| // https://github.com/oven-sh/bun/issues/11898 (Trend Micro). | ||
| describe.if(isWindows)("Windows VEH handler and first-chance faults in external DLLs", () => { |
There was a problem hiding this comment.
🟡 🟡 nit (comment accuracy — same "update every consumer atomically" class as the four sites fixed in 707846d; this is a fifth sibling that sweep missed): the pre-existing test immediately above at :130-165 (segfault inside a system DLL captures the bun callers, added by #35074 / 892b1da) faults via RtlFillMemory(0xDEADBEEF, ...) with the PC in ntdll.dll, and its comment at :130-135 states "the VEH handler must walk the stack from the fault CONTEXT record" — but this PR's new out-of-image gate (lib.rs:2122-2130) makes the VEH return CONTINUE_SEARCH for exactly this fault; the CONTEXT walk now happens from Bun__crashHandlerFromJSCFrame/UEF instead. The test's assertions still hold (all three entry points feed the same fault CONTEXT to capture_from_context), so this is prose-only. Suggest rewording :130-135 to name the actual entry point, e.g. "the crash handler must walk the stack from the fault CONTEXT record (RtlVirtualUnwind) — post-#35083 the VEH declines this ntdll-PC fault and dispatch reaches Bun__crashHandlerFromJSCFrame/UEF, which seed the same walk". Separately, no remaining test exercises the VEH's own CONTEXT-walk path for an in-image fault — worth a follow-up if that property matters independently.
Extended reasoning...
What this is
The pre-existing test test.if(isWindows && isDebug)("Windows: segfault inside a system DLL captures the bun callers") at run-crash-handler.test.ts:136-165 was added by the immediately-preceding PR #35074 / commit 892b1da. It faults via js_segfault_in_dll (src/runtime/api/crash_handler_jsc.rs:103-118) → RtlFillMemory(0xDEADBEEF, ...), so the faulting instruction is inside ntdll.dll. Its explanatory comment at :130-135 states the property under test:
Windows: the VEH handler must walk the stack from the fault CONTEXT record (RtlVirtualUnwind), not from inside the handler. When the fault is in an external DLL the old RtlCaptureStackBackTrace path could stop at KiUserExceptionDispatcher …
This PR changes handle_segfault_windows so that when ExceptionAddress is outside bun.exe's image range (and the reason isn't StackOverflow), the VEH returns EXCEPTION_CONTINUE_SEARCH (src/crash_handler/lib.rs:2122-2130). An ntdll PC is outside bun.exe's image, so post-PR the VEH declines this test's fault. The crash instead routes via SEH dispatch → JSC's jscJITSEHHandler → Bun__crashHandlerFromJSCFrame, or falls through to handle_unhandled_exception_windows (UEF). The comment now names the wrong handler.
Why this is the same class as 707846d
Commit 707846d in this PR ("Addressed" the earlier claude review at 2026-07-23T09:15) swept four stale "LLInt range" mentions across webkit.ts, lib.rs, ZigGlobalObject.cpp, and this test file after commit 8f0c63e changed what WebKit#315 registers. That was exactly REVIEW.md's "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR" / "one source of truth; update every consumer atomically". The :130-135 comment is a fifth site describing pre-PR routing that the same sweep should have caught — it just lives immediately above the diff hunk rather than inside it, so it's easy to miss.
Step-by-step: routing before vs after
Pre-PR (892b1da, when the comment was written):
js_segfault_in_dll→RtlFillMemory(0xDEADBEEF, 8, 0)faults at arep stosbin ntdll.KiUserExceptionDispatcher→RtlDispatchException→RtlpCallVectoredHandlers→handle_segfault_windows.- VEH matches
EXCEPTION_ACCESS_VIOLATION, callscrash_handler(SegmentationFault(0xDEADBEEF), TraceSeed::Fault { pc, fp: info.ContextRecord }). capture_from_contextwalks viaRtlVirtualUnwindfrom the fault CONTEXT → the test's ≥7-frame / span-<2³¹ assertions pass.
Post-PR (this change):
- Same fault, same
KiUserExceptionDispatcher→ VEH. handle_segfault_windowscomputespc = record.ExceptionAddress(in ntdll), loadsWINDOWS_EXE_IMAGE_BASE..END, sees!(base..end).contains(&pc), returnsCONTINUE_SEARCH(lib.rs:2127-2130). VEH does not walk anything.RtlDispatchExceptionproceeds to frame-based dispatch. The FFI trampoline / native-call thunk sits in JSC's fixed executable memory pool, whose UNWIND_INFO (WebKit#315) namesjscJITSEHHandleras language-specific handler →Bun__crashHandlerFromJSCFrame(record, _, context, _)(lib.rs:2146-2179).- That handler calls
crash_handler(SegmentationFault(0xDEADBEEF), TraceSeed::Fault { pc, fp: context as usize })— sameTraceSeed::Faultshape, and per MSDN thePEXCEPTION_ROUTINE'sContextRecordduring search phase is the original fault CONTEXT. capture_from_contextwalks the same fault CONTEXT viaRtlVirtualUnwind→ the test's assertions still pass, for the same underlying reason.
So the property the test validates (fault-CONTEXT-seeded RtlVirtualUnwind walk recovers bun callers) is still exercised — just via a different entry point than the comment names.
Why the assertions are unaffected (why this is nit, not normal)
All three Windows entry points converge on identical inputs to the same sink:
- VEH (lib.rs:2136):
TraceSeed::Fault { pc, fp: info.ContextRecord as usize } Bun__crashHandlerFromJSCFrame(lib.rs:2177):TraceSeed::Fault { pc, fp: context as usize }- UEF (lib.rs:2197):
TraceSeed::Fault { pc, fp: info.ContextRecord as usize }
and capture_from_context treats fp as *const CONTEXT and RtlVirtualUnwind-walks from it regardless of caller. The test's frame-count (≥7) and address-span (<2³¹) assertions measure the output of that walk, not which handler invoked it. So no assertion is stale — only the prose.
The original bug report also raised a speculative "test may fail if SEH dispatch derails at LLInt frames lacking .pdata". That concern applies equally to this PR's own new unguarded fault still crash-reports and unguarded fault from inside a JIT-compiled frame tests (same FFI→ntdll→SEH-dispatch shape), and host-function calls go through a JIT-pool native-call thunk that WebKit#315's growable table covers, so jscJITSEHHandler should fire before dispatch reaches any .pdata-less LLInt asm frame. The PR body explicitly defers Windows verification to CI, which will catch all of these together if that assumption is wrong.
Secondary observation (coverage gap, not a bug)
Post-PR, the only case where the VEH itself seeds capture_from_context from the fault CONTEXT is an in-image fault (PC inside bun.exe). This test no longer exercises that path, and no other test does — the segfault fixture faults inside bun.exe but doesn't assert trace quality on Windows. If validating the VEH-specific CONTEXT-walk property matters independently of the JSC-SEH/UEF paths, an in-image variant (or repointing js_segfault_in_dll at an in-image address) would restore it. This is optional follow-up, not a defect.
Suggested fix
Update the comment at :130-135 to something like:
// Windows: the crash handler must walk the stack from the fault CONTEXT
// record (RtlVirtualUnwind), not from inside the handler. When the fault is
// in an external DLL the old RtlCaptureStackBackTrace path could stop at
// KiUserExceptionDispatcher on some Windows versions, leaving only the
// handler's own frames. Post-#35083 the VEH declines out-of-image PCs; this
// fault reaches capture_from_context via Bun__crashHandlerFromJSCFrame/UEF,
// which seed the same fault-CONTEXT walk.
| describe.if(isWindows)("Windows VEH handler and first-chance faults in external DLLs", () => { | ||
| test("SEH-guarded probe survives", async () => { |
There was a problem hiding this comment.
🟡 These four tests each spawn an independent Bun subprocess with no shared state, so they should use test.concurrent(...) instead of plain test(...) — REVIEW.md's "Tests reviewers reject" section calls for "test.concurrent for independent subprocess suites", and this file already applies that convention in the SIGABRT/SIGTRAP describe below (test.concurrent.each / test.skipIf(isASAN).concurrent.each). The JIT-warmup test in particular runs 10000 iterations under debug JSC with useConcurrentJIT: 0, so serial execution adds noticeable wall-clock on the Windows lane.
Extended reasoning...
What this is
The four new tests inside describe.if(isWindows)("Windows VEH handler and first-chance faults in external DLLs", ...) at test/cli/run/run-crash-handler.test.ts:179 each spawn an independent Bun subprocess and use plain test(...):
"SEH-guarded probe survives"— spawnsbun -ewith anIsBadReadPtrFFI probe"unguarded fault still crash-reports"— spawnsbun -ewithRtlFillMemory"RtlLookupFunctionEntry resolves JSC JIT pool PCs"— spawnsbun -ecallingjscInternals.startOfFixedExecutableMemoryPool()+ FFI"unguarded fault from inside a JIT-compiled frame ..."— spawnsbun -ewith a 10000-iteration warmup loop underBUN_JSC_jitPolicyScale=0/BUN_JSC_useConcurrentJIT=0
Each is a self-contained await using proc = Bun.spawn(...) + Promise.all([stdout.text(), stderr.text(), exited]) on its own pipes, spreading the shared noReportEnv per call. There is no shared server, port, temp directory, or mutable module-level state between them.
Why the repo convention applies
REVIEW.md's "Tests reviewers reject" → "Copy harness conventions exactly" section says:
test.concurrentfor independent subprocess suites … Keep tests fast (~1s per test; debug+ASAN runs 10-100x slower); a new file over ~10s on the default lane gets scrutinized fortest.concurrentand staying serial needs a stated reason. A correct but slow test still gets changes-requested.
And this file already applies that convention locally: the describe.if(isPosix)("SIGABRT/SIGTRAP are caught by the crash handler", ...) block immediately below uses test.concurrent.each(...), test.skipIf(isASAN).concurrent.each(...), and test.concurrent(...) for the identical shape (independent Bun.spawn + await, no shared state). So test.concurrent is the established local idiom for this pattern in this file, and the new block diverges from it without a stated reason.
Step-by-step: why serial execution is measurable here
- Test 4 sets
BUN_JSC_jitPolicyScale: "0"andBUN_JSC_useConcurrentJIT: "0", then runsfor (let i = 0; i < 10000; i++) hot(i)to forcehot()up the JIT tiers synchronously. Under a debug JSC build (which is what the Windows debug lane runs), disabling the concurrent JIT means each tier-up compilation blocks the main thread; 10000 iterations under debug JSC is on the order of seconds, not milliseconds. - Tests 1-3 each pay full process-startup cost (debug
bun.exe+ FFIdlopenof kernel32/ntdll + JSC init). CLAUDE.md notes debug+ASAN builds run 10-100× slower than release. - Serially, the describe's wall-clock is
t1 + t2 + t3 + t4. Withtest.concurrent, it collapses to roughlymax(t1..t4)≈t4(the JIT-warmup test), since all four subprocesses can run in parallel on the multi-core Windows CI runner. - This describe is Windows-only, so the wall-clock lands entirely on the (already slowest) Windows CI lane.
Why this is nit-severity
Nothing is functionally wrong — the tests are correct, hermetic, and will pass. This is purely a test-hygiene / CI-wall-clock concern that the repo's own review checklist flags. Other serial subprocess blocks also exist in this file (the "terminal signal reflects the crash cause" test.each and the "automatic crash reporter" for-loop), so this is not uniquely deviant; but those are pre-existing, whereas these four are newly added in this PR alongside a sibling block that already uses .concurrent.
Suggested fix
Change the four test(...) calls to test.concurrent(...):
describe.if(isWindows)("Windows VEH handler and first-chance faults in external DLLs", () => {
test.concurrent("SEH-guarded probe survives", async () => { ... });
test.concurrent("unguarded fault still crash-reports", async () => { ... });
test.concurrent("RtlLookupFunctionEntry resolves JSC JIT pool PCs", async () => { ... });
test.concurrent("unguarded fault from inside a JIT-compiled frame ...", async () => { ... });
});No shared state exists to make this unsafe: each test's await using proc owns its own subprocess and pipes, noReportEnv is spread per-call (test 4's extra BUN_JSC_* keys go into a fresh object), and there are no beforeEach/afterEach hooks in the describe.
3 participants
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/__exceptas 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, BUN-2V6E, BUN-3K05, BUN-3K2N are all this one crash (~18.8k events, 1,299 machines): BeyondTrust's
PGHook.dllhooksMoveFileExW, passes aNULLHCRYPTPROVtoCryptCreateHash,CRYPTSP.dllvalidates viacmp [rcx+0E8h], 11111111hunder SEH, and Bun's VEH reports the0xE8probe as a segfault.Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056.
Fix
Three handlers, each deterministic, no heuristic parsing:
VEH (
handle_segfault_windows): only claim the fault whenExceptionAddressis insidebun.exe's own image. Otherwise returnCONTINUE_SEARCHso frame-based SEH can run. Matches Go'sisgoexceptionand CoreCLR NativeAOT'sRhpVectoredExceptionHandler. Stack overflow is always claimed here: no foreign__exceptrecovers from it in practice, and SEH dispatch itself costs guard-page stack.JSC SEH handler (
Bun__crashHandlerFromJSCFrame, viaJSC::setJITExceptionHandlerWin): JSC now registersRtlAddGrowableFunctionTableunwind info for its fixed JIT pool (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.textand Windows only consults static.pdatafor in-module PCs, so covering it needs build-time.seh_*emission in offlineasm (follow-up; see the comment inExecutableAllocator.cpp).UEF (
handle_unhandled_exception_windows, viaSetUnhandledExceptionFilter): backstop for anything no SEH handler claimed and no JIT frame caught.All three seed
capture_from_contextwith the faultCONTEXT, so #35074'sRtlVirtualUnwindwalk applies to each.Verification
Repro (canary
5b98630ac, Server 2019):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)RtlLookupFunctionEntryreturns non-null for a JIT pool PC (validates the WebKit-side unwind-info registration)jitPolicyScale=0) thenSetUnhandledExceptionFilter(0)and fault via FFI from inside it; crash is still reported, isolatingjscJITSEHHandleras the catch pointPrior art
V8
RegisterNonABICompliantCodeRange, SpiderMonkeyRegisterExecutableMemory, microsoft/python-etwtrace, and Steve Dower's guidance in python/cpython#126910 ("RtlAddGrowableFunctionTableis actually the only one that works") all converge on this design. Go issue golang/go#56082 describes the exact VEH-vs-SEH failure class.