WTF: read interrupted SP from ucontext in signalHandlerSuspendResume - #235
WTF: read interrupted SP from ucontext in signalHandlerSuspendResume#235robobun wants to merge 1 commit into
Conversation
|
Small suggestion: now that the handler is robust to SA_ONSTACK via ucontext SP, the comment on the SIGPWR override (from Suggested replacement: #if OS(LINUX) && USE(BUN_JSC_ADDITIONS)
// Thread suspension on Linux uses a signal (macOS uses Mach ports instead —
// this entire signal-based mechanism is Linux/FreeBSD-only). We override the
// default SIGUSR1 to SIGPWR because npm packages commonly register
// process.on('SIGUSR1') handlers. SIGPWR ("power failure") is effectively
// unused on modern Linux.
//
// Note: dlopen'd runtimes (Go cgo, Mono, JNI) may add SA_ONSTACK to this
// handler's sigaction flags via their init routines. The handler reads the
// interrupted SP from ucontext rather than its own frame pointer, making it
// robust to alt-stack delivery regardless of flag mutation.
// See oven-sh/bun#31158.
g_wtfConfig.sigThreadSuspendResume = SIGPWR;
#endifThis ties together the "why SIGPWR", "why Linux-only", and "why it's safe" in one place. |
|
Warning Review limit reached
More reviews will be available in 14 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe signal-handler suspend/resume mechanism is improved to detect the interrupted thread's stack pointer from machine-context ChangesSignal handler stack pointer reliability
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Preview Builds
|
Point WEBKIT_VERSION at the preview autobuild of oven-sh/WebKit#235, which teaches signalHandlerSuspendResume to read the interrupted thread's SP from the ucontext instead of the handler's own currentStackPointer(). That's the root-cause fix — regardless of what any dlopen'd library (Go cgo, Mono, JNI, Rust cdylib, …) does to SA_ONSTACK, the stack-range check always sees the interrupted stack rather than wherever the handler landed. Delete the per-dlopen sigaction scan on the Bun side. With the WTF change in place it's pure overhead, and the reporter rightly pointed out that chaining workarounds for workarounds is the wrong direction. Regression test (test/regression/issue/31158.test.ts) is unchanged and now exercises the upstream fix directly. Swap the preview-pr-235 tag for the merged autobuild hash once oven-sh/WebKit#235 lands.
925c056 to
0eedb19
Compare
0eedb19 to
e478893
Compare
71211c9 to
71e0011
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
71e0011 to
66e7173
Compare
…e7173c) Rebase of the signalHandlerSuspendResume ucontext-SP fix onto the current WebKit main pin 09f04cd5, plus a cross-reference comment tying interruptedStackPointer() to JSC::MachineContext::stackPointerImpl so a future arch addition updates both copies.
66e7173 to
3593e2c
Compare
697e8a7 to
83f3867
Compare
There was a problem hiding this comment.
Thanks for addressing both nits — the sync note and the SP/FP wording look good now. The change reads correct to me, but since this is async-signal-safe code in the GC thread-suspend path (register snapshot correctness feeds conservative stack scanning), I'd like a human to sign off rather than auto-approving.
Extended reasoning...
Overview
This PR changes Thread::signalHandlerSuspendResume in Source/WTF/wtf/posix/ThreadingPOSIX.cpp to read the interrupted thread's stack pointer from the kernel-saved ucontext_t register state instead of calling currentStackPointer() inside the handler. A new interruptedStackPointer(ucontext_t*) helper covers Linux/FreeBSD × x86_64/arm64/arm/riscv64, mirroring JSC::MachineContext::stackPointerImpl. The !HAVE(MACHINE_CONTEXT) path and the retry/back-off semantics are unchanged; a comment in initializePlatformThreading() documents the SA_ONSTACK interaction. Both of my earlier inline nits (cross-reference/sync comment for the duplicated per-arch accessors, and the "frame pointer" → "stack pointer" wording) have been addressed in 66e7173 and 697e8a7, and the bug-hunting pass found nothing new.
Security risks
No new attack surface: the handler already received and dereferenced ucontext_t (via registersFromUContext), so reading one more field from the same kernel-provided struct doesn't change trust boundaries. No auth/crypto/permissions involved. The main correctness risk would be a wrong per-arch field name causing a bogus SP to pass the m_stack.contains() check — but each accessor matches the long-standing entries in MachineContext.h, and a wrong value would almost certainly fall outside the recorded stack bounds and trigger the existing back-off.
Level of scrutiny
High. This runs in signal context on the GC suspend/resume path for every non-Darwin build. The register snapshot it publishes is what the conservative GC scans, so a mistake here could manifest as a livelock (the original bug), a missed root, or an inconsistent snapshot. That's well outside the "simple/mechanical/obvious" bar for auto-approval even though the diff is small and the reasoning in the PR description is convincing.
Other factors
- The fix is strictly a superset of the old behavior on unsupported archs (falls back to
currentStackPointer()), so no regression risk there. - A regression test is landing on the Bun side (oven-sh/bun#31161) rather than in this repo.
- All prior review threads on this PR are resolved; no outstanding human reviewer comments.
- Given the criticality of the code path, deferring to a human maintainer for final sign-off is the safer call.
signalHandlerSuspendResume validated the snapshot by calling currentStackPointer(), the SP of the handler frame itself. That assumes the handler always runs on the thread's normal stack, which fails when SA_ONSTACK is set on our sigaction and the thread has a sigaltstack installed (Go's cgo initsig does this to inherited handlers; combined with ASAN/crash-handler alt stacks the check fails on every retry and Thread::suspend() spins forever). Read the interrupted SP from the ucontext instead, which is stable regardless of which stack the handler runs on. See oven-sh/bun#31158, oven-sh/bun#29843.
83f3867 to
48bb10b
Compare
There was a problem hiding this comment.
Both earlier nits are addressed and I didn't find further issues, but this is async-signal-safe code in the GC thread-suspend path — worth a human sign-off.
What was reviewed:
- Per-arch
mcontext_t→SP accessors matchJSC::MachineContext::stackPointerImpl; sync comment now documents the drift hazard. userContexthoisted into the earlierHAVE(MACHINE_CONTEXT)block stays in scope for the laterregistersFromUContextuse; the!HAVE(MACHINE_CONTEXT)path is behaviourally unchanged (rename only).interruptedStackPointer()is a pure struct-field read — no allocation/locks, so async-signal-safety is preserved.- Retry-on-alt-stack semantics still hold when the interrupted code is genuinely on an alt stack (ucontext SP reflects that).
Extended reasoning...
Overview
Single-file change to Source/WTF/wtf/posix/ThreadingPOSIX.cpp. Adds interruptedStackPointer(ucontext_t*) (per-arch mcontext_t SP accessor for Linux/FreeBSD × x86_64/arm64/arm/riscv64) and switches signalHandlerSuspendResume's stack-containment check from currentStackPointer() (handler's own SP) to the kernel-saved interrupted SP, with fallback to the old behaviour where no accessor exists. Also expands comments in the handler and in initializePlatformThreading() to document the SA_ONSTACK interaction with dlopen'd runtimes (Go cgo, Mono, JNI).
Security risks
None introduced. The change reads a field from the kernel-populated ucontext_t that the handler already receives and already dereferences (via registersFromUContext). No new external input, no allocation, no new signal registration.
Level of scrutiny
High. This is a signal handler on the GC stop-the-world suspend/resume path — a bug here manifests as deadlock (the very failure mode being fixed) or, worse, an incorrect m_platformRegisters snapshot feeding conservative root scanning. The logic itself is small and the fix is well-argued (with a C repro in the PR description and a Bun-side regression test), but signal-handler + per-arch mcontext_t layout + async-signal-safety is exactly the kind of code where a second pair of human eyes on the field offsets and preprocessor branching is cheap insurance.
Other factors
- Both of my earlier inline comments (MachineContext.h duplication/drift hazard; "frame pointer" → "stack pointer" wording) were addressed in 66e7173 and 697e8a7 respectively and are resolved.
- The reviewer suggestion about expanding the SIGPWR comment was incorporated in spirit — the
initializePlatformThreading()comment now documents the SA_ONSTACK/ucontext relationship. - Preview builds have been produced for every revision since May, and a regression test exists on the Bun side (oven-sh/bun#31161).
- The bug-hunting pass on the current revision found nothing.
Given the critical-path nature of the code, deferring rather than auto-approving.
2 participants
Fixes the SIGPWR suspend-loop deadlock reported in oven-sh/bun#31158 (and oven-sh/bun#29843 — same mechanism via Prisma's WASM-backed adapter).
The bug
Thread::suspend()sends the GC suspend-resume signal to the target thread and then spins:The handler decides whether to publish a register snapshot by checking
currentStackPointer()— the SP of the handler's own frame — againstthread->m_stack:That only works so long as the handler runs on the normal stack. If
SA_ONSTACKis set on the sigaction and the thread has asigaltstackinstalled, the handler runs on the alt stack, the check fails on every retry, and the loop spins forever.Who trips this
Go's cgo runtime.
runtime.initsigwalks every signal and, for any handler it didn't install itself, callssetsigstackto force-addSA_ONSTACKso Go's own threads' synchronous faults stay on a managed alt stack. That includes our SIGPWR handler. Any c-shared library that follows the same recipe (Mono, some JNI layouts, Rust cdylibs) hits it too.Combined with sanitizer runtimes / libbacktrace / the host's crash handler installing an alternate signal stack on the main thread (ASAN does this unconditionally), the next GC thread-suspend delivers SIGPWR onto the alt stack → stack check fails every retry → the suspender wedges at 100% CPU.
Reproducible in a plain C program to confirm
currentStackPointervs the ucontext-saved SP:The fix
Read the SP the thread was running at when the signal arrived straight out of the ucontext (kernel-saved register state). That SP is stable regardless of whether the handler itself runs on the normal or the alt stack, and the existing retry-on-alt-stack semantics still work: if the thread genuinely was on an alt stack when the signal arrived (e.g. a nested handler), the ucontext SP reflects that and the check backs off as before.
currentStackPointer()stays as the fallback for non-HAVE(MACHINE_CONTEXT) platforms.Test
Regression test coming from the Bun side (oven-sh/bun#31161) — spawns a child that sets
SA_ONSTACKon the suspend-resume signal the same way Go'sinitsigdoes, then drives the WASM install path that triggersresetInstructionCacheOnAllThreads. Passes with this fix applied; hangs to the test-runner timeout without it.