SignalsWin: fix VEH return value and register behind AddressSanitizer's handler - #331
Conversation
…he fault vectoredHandler() defaulted its result to EXCEPTION_EXECUTE_HANDLER and returned it from a vectored exception handler. That constant is only valid as an __except() filter result; a VEH must return either EXCEPTION_CONTINUE_EXECUTION or EXCEPTION_CONTINUE_SEARCH. Returning 1 is treated as continue-execution by the dispatcher, so an access violation no WTF handler claimed would re-execute the faulting instruction, fault again, and re-enter the handler in an unbounded loop until the stack was gone. Because WTF installs itself first in the VEH chain, handlers registered behind it (ASan's shadow-page committer is the obvious one) never saw exceptions they own. Default to EXCEPTION_CONTINUE_SEARCH so unclaimed faults continue down the chain, matching the Mach (KERN_FAILURE) and POSIX (call old handler) paths for SignalAction::NotHandled. The SignalAction::Handled case still returns EXCEPTION_CONTINUE_EXECUTION as before.
WalkthroughChangesWindows exception handling
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-constant fix aligning the VEH return value with the documented Windows contract.
What was reviewed:
- Confirmed
PVECTORED_EXCEPTION_HANDLERmust return onlyEXCEPTION_CONTINUE_EXECUTIONorEXCEPTION_CONTINUE_SEARCH;EXCEPTION_EXECUTE_HANDLERis SEH-filter-only (all other uses in this repo are inside__except()). - Verified the Mach (
KERN_FAILUREon not-handled) and POSIX (chain tooldAction.sa_sigaction) paths inSignals.cpp— this now matches them. - The
Signal::Unknownearly return a few lines up already usedEXCEPTION_CONTINUE_SEARCH, and theHandled → EXCEPTION_CONTINUE_EXECUTIONpath is untouched.
Extended reasoning...
Overview
Single-line semantic change in Source/WTF/wtf/win/SignalsWin.cpp: the default return value of vectoredHandler() (a Windows vectored exception handler) is changed from EXCEPTION_EXECUTE_HANDLER (1) to EXCEPTION_CONTINUE_SEARCH (0), plus an explanatory comment. The SignalAction::Handled branch continues to return EXCEPTION_CONTINUE_EXECUTION as before.
Correctness
Per Microsoft's PVECTORED_EXCEPTION_HANDLER documentation, a VEH callback may return only EXCEPTION_CONTINUE_EXECUTION or EXCEPTION_CONTINUE_SEARCH. EXCEPTION_EXECUTE_HANDLER is only meaningful in an SEH __except() filter expression — every other occurrence of that constant in this repo (jsc.cpp, testRegExp.cpp, MiniBrowser, gtest, ANGLE StackWalker, skia) is inside an __except filter, not a VEH. ANGLE's own VEH in system_utils_win32.cpp:165-169 uses exactly the CONTINUE_EXECUTION / CONTINUE_SEARCH pair, matching this fix.
I also verified the cross-platform-parity claim: Source/WTF/wtf/threads/Signals.cpp:280 returns KERN_FAILURE when no Mach handler claims the fault, and lines 538-542 chain to the previously installed sigaction on POSIX when !didHandle. Returning EXCEPTION_CONTINUE_SEARCH is the Windows equivalent. The same function already returns EXCEPTION_CONTINUE_SEARCH at line 98 for Signal::Unknown, so both "not ours" paths are now consistent.
Security risks
None introduced. The change makes unclaimed hardware faults propagate to downstream handlers (ASan shadow committer, crash reporters, SEH frames) instead of being swallowed into an infinite re-fault loop. It does not add new code paths, allocate, or touch user-controlled data.
Level of scrutiny
Low-to-medium. It is a one-constant correction to a well-documented Windows API contract, with a clear failure mode described (infinite re-fault when a WTF handler declines an access violation). The blast radius is Windows-only and only affects the fallthrough path when no JSC signal handler claims the exception — the wasm/VMTraps handled path is unchanged.
Other factors
No prior human or bot review comments to address (CodeRabbit was rate-limited). The bug hunting system found no issues. The diff is minimal and mechanically verifiable against the Win32 docs, so I'm comfortable approving without human review.
There was a problem hiding this comment.
LGTM — one-constant fix to a documented-invalid VEH return value.
What was reviewed:
- Confirmed
EXCEPTION_EXECUTE_HANDLERis not a valid VEH return;EXCEPTION_CONTINUE_SEARCHis the correct "not mine" value and already used a few lines up forSignal::Unknown. - Cross-checked the Mach path (
didHandle ? KERN_SUCCESS : KERN_FAILURE) and POSIX path (falls through to oldsigaction) inSignals.cpp— this now matches both. - Checked the registered handlers (Wasm fault handler, VMTraps) — both legitimately return
NotHandledfor faults they don't own, so continuing the search is the intended behavior; theHandled→EXCEPTION_CONTINUE_EXECUTIONpath is unchanged.
Extended reasoning...
Overview
Single-line change in Source/WTF/wtf/win/SignalsWin.cpp: the default return value of vectoredHandler() is changed from EXCEPTION_EXECUTE_HANDLER (1) to EXCEPTION_CONTINUE_SEARCH (0), plus a three-line explanatory comment. The SignalAction::Handled branch that sets EXCEPTION_CONTINUE_EXECUTION is untouched.
Security risks
None introduced. The old value was undefined behavior for a vectored exception handler (Microsoft documents only EXCEPTION_CONTINUE_EXECUTION and EXCEPTION_CONTINUE_SEARCH as valid). The new behavior lets unclaimed faults propagate to later VEH handlers and, ultimately, the default unhandled-exception path — which is strictly safer than the previous re-execute-the-faulting-instruction loop. No auth, crypto, or permission surfaces touched.
Level of scrutiny
Low. This is a well-understood Windows API contract fix. The same function already returns EXCEPTION_CONTINUE_SEARCH for the Signal::Unknown early-out, so the change is internally consistent. I verified the parallel logic in Source/WTF/wtf/threads/Signals.cpp for Mach (line 280: KERN_FAILURE on not-handled) and POSIX (line 538+: chain to old sigaction) — the PR description's claim that this aligns Windows with the other platforms is accurate.
Other factors
The registered Signal::AccessFault handlers in JSC (WasmFaultSignalHandler.cpp, VMTraps.cpp) return SignalAction::NotHandled for faults outside their range, so nothing was relying on the old (broken) default to terminate the process — the old value just caused a fault loop. No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
Preview Builds
|
Under ASAN_ENABLED, register the WTF vectored handler with First=0 instead of First=1. AddressSanitizer's runtime installs its own vectored handler that commits shadow pages lazily and unpoisons the dispatch context before the handlers behind it run. Registered ahead of it, the WTF handler reads the CONTEXT record and the exception-dispatch stack before ASan has unpoisoned them, so every fault produces spurious stack-buffer reports attributed to the WTF handler. Registering behind ASan removes those reports; non-ASan builds keep the existing first-slot registration.
dylan-conway
changed the title
There was a problem hiding this comment.
LGTM — correct fix aligning the VEH return value with the documented Windows contract, plus an ASan-only registration-order tweak.
What was reviewed:
- Verified
EXCEPTION_CONTINUE_SEARCHis the documented valid "not handled" return for a vectored handler; the oldEXCEPTION_EXECUTE_HANDLERis a frame-SEH-only value. - Confirmed
ASAN_ENABLEDis always defined (0/1) inCompiler.h, so#if ASAN_ENABLEDis correct and the non-ASan registration path is byte-identical to before. - Checked the POSIX
Signals.cpp— the new default matches itsNotHandledfall-through-to-old-action semantics.
Extended reasoning...
Overview
Two small changes to Source/WTF/wtf/win/SignalsWin.cpp:
- In
vectoredHandler(), the default result when no registered WTF handler returnsSignalAction::Handledchanges fromEXCEPTION_EXECUTE_HANDLER(1) toEXCEPTION_CONTINUE_SEARCH(0). - In
SignalHandlers::finalize(), under#if ASAN_ENABLEDthe vectored handler is registered withAddVectoredExceptionHandler(0, …)(append) instead of(1, …)(prepend), so ASan's own VEH runs first.
Security risks
None. This is Windows-only exception-dispatch plumbing. No user-controlled input, parsing, auth, or crypto is involved. The change makes unclaimed faults propagate to the rest of the handler chain rather than returning an undocumented value — strictly a correctness improvement.
Level of scrutiny
Moderate. Signal/exception handling underpins VM traps and Wasm fault handling, so it's worth reading carefully — but the diff is tiny and mechanical:
- Change 1 replaces a value that is invalid per Microsoft's VEH contract (a vectored handler may return only
EXCEPTION_CONTINUE_EXECUTIONorEXCEPTION_CONTINUE_SEARCH) with the documented correct one. TheHandled→EXCEPTION_CONTINUE_EXECUTIONpath is untouched, so JSC's own trap handling (the case that matters in production) behaves identically. - Change 2 is compiled in only when
ASAN_ENABLEDis truthy (defined 0/1 inwtf/Compiler.h), so release/production builds keep the exact priorAddVectoredExceptionHandler(1, …)call. The ASan-only ordering change affects debug/instrumented builds and is well-motivated (ASan unpoisons the dispatch context for handlers that follow it).
Other factors
- No CODEOWNERS entry covers
Source/WTF/wtf/win/. - Preview builds for the PR head completed successfully per the CI comment.
- The new default mirrors the POSIX/Mach path in
Signals.cpp, whereNotHandledfalls through to the previously-installed action — so cross-platform semantics are now consistent. SignalAction::ForceDefaultstill hits the switchdefault:and now yieldsEXCEPTION_CONTINUE_SEARCH; that's the closest Windows analogue to "let the default handler take it" and is no worse than the prior invalid return.- The bug hunting system found no issues.
2 participants
Two fixes to the Windows vectored exception handler in
Source/WTF/wtf/win/SignalsWin.cpp.1. Return
EXCEPTION_CONTINUE_SEARCHwhen no handler claims the faultvectoredHandler()initialized its result toEXCEPTION_EXECUTE_HANDLERand returned it whenever no registered WTF signal handler reportedSignalAction::Handled. A vectored exception handler may only returnEXCEPTION_CONTINUE_EXECUTIONorEXCEPTION_CONTINUE_SEARCH;EXCEPTION_EXECUTE_HANDLER(1) is a frame-based SEH filter result and is invalid here. The dispatcher does not treat it as "search" — it re-executes the faulting instruction with nothing resolved, re-entering the handler in an unbounded loop.The visible consequence is under AddressSanitizer: ASan reserves its shadow up front and commits pages lazily from its own vectored handler on the first touch. WTF's handler returning the invalid value on those first-touch faults starved ASan's committer, so instrumented binaries died at startup with a silent access violation. The default result is now
EXCEPTION_CONTINUE_SEARCH, matching the POSIX/MachNotHandledpath, so unclaimed faults proceed down the chain.2. Register behind AddressSanitizer's handler under
ASAN_ENABLEDSignalHandlers::finalize()registered withAddVectoredExceptionHandler(1, …), placing WTF's handler ahead of ASan's. ASan's handler unpoisons the exception dispatch context (theCONTEXTrecord and dispatch stack) for the handlers that run after it; a handler in front of it reads those regions while still poisoned, so every fault produced spuriousstack-buffer-*reports attributed to WTF's handler. UnderASAN_ENABLEDthe handler is now registered withFirst = 0(behind ASan's); non-ASan builds keep the existing first-slot registration.With both changes an ASan-instrumented consumer of these libraries starts and runs cleanly.