process: guard JSC's GC suspend signal against unsolicited SIGPWR on Linux - #33777
process: guard JSC's GC suspend signal against unsolicited SIGPWR on Linux#33777robobun wants to merge 8 commits into
Conversation
On Linux, JSC uses SIGPWR to suspend and resume JS threads for
conservative stack scanning. WTF::Thread::signalHandlerSuspendResume
unconditionally dereferences a file-static targetThread pointer that
only JSC's own suspend() populates, so any SIGPWR delivered from
outside that path (process.kill(pid, 30), subprocess.kill('SIGPWR'),
or an external kill -PWR) null-dereferenced at offset 0x58 and
segfaulted the whole process.
Install a thin wrapper over WTF's handler right after
WTF::initializeMainThread: signals that arrive via pthread_kill from
our own process (si_code == SI_TKILL, si_pid == getpid()) are passed
through to JSC unchanged; everything else is forwarded to the JS
signal queue so process.on('SIGPWR', ...) fires like it does under
Node. SIGPWR is also added to the process signal name tables so it
can be used with process.kill and process.on, and the listener
add/remove path is taught not to touch the sigaction for JSC's
reserved signal.
|
Updated 3:59 PM PT - Jul 8th, 2026
❌ @robobun, your commit 533775d has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33777That installs a local version of the PR into your bun-33777 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither of those is closed by this change.
This PR only changes what happens when SIGPWR arrives via |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Partial overlap, not a duplicate. #33776 adds This PR's load-bearing change is the |
|
Warning Review limit reached
Next review available in: 25 seconds 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 (2)
WalkthroughAdds SIGPWR mapping support, installs a Linux guard around JSC suspend/resume signal handling, wires that guard into JSC initialization, and adds Linux-only tests for SIGPWR delivery and listener behavior. ChangesSIGPWR support and suspend/resume guard
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/js/node/process/process-sigpwr.test.ts`:
- Around line 146-173: The GC stress test in the SIGPWR case is too expensive
for debug/ASAN lanes and can time out before it proves anything. Update the test
body around the concurrent case in process-sigpwr.test.ts to reduce the loop
count and/or workload when isDebug is true, while keeping the full 50-iteration
stress path for release runs. Use the existing test name and the Bun.gc(true),
process.kill, and setImmediate loop as the place to apply the conditional
scaling.
- Around line 11-38: The SIGPWR test file repeats the same
Bun.spawn/drain/assert pattern across multiple cases, so extract a shared helper
around Bun.spawn, Promise.all([proc.stdout.text(), proc.stderr.text(),
proc.exited]), and the returned stdout/stderr/exitCode/signalCode shape. Update
the individual test cases to call that helper with only the script and expected
output differences, using the existing test names and the shared runScript-style
helper to keep the assertions centralized.
- Line 18: The SIGPWR signal is hardcoded as the bare number 30 in multiple
places, which should be replaced with a named constant for clarity and
portability. Update the process-sigpwr tests to define and use a single SIGPWR
constant (preferably sourced from os.constants.signals.SIGPWR, with a local
fallback if needed) and replace all direct process.kill(..., 30) usages in the
relevant test cases with that constant.
- Around line 65-98: The helper spawnAndSignalAfterReady is only reading
proc.stdout in its loop and waits until the end to call proc.stderr.text(),
which can deadlock if stderr fills first. Update the test to drain proc.stderr
concurrently while the stdout reader loop is running, and keep the existing
ready-detection/deliver(proc) flow intact so the child cannot block on an unread
pipe.
🪄 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: 101aa7a4-3f32-4779-b397-3027a96ed350
📒 Files selected for processing (3)
src/jsc/bindings/BunProcess.cppsrc/jsc/bindings/ZigGlobalObject.cpptest/js/node/process/process-sigpwr.test.ts
Extract a shared runScript helper, drain stderr concurrently with the stdout reader in spawnAndSignalAfterReady, and scale the GC stress loop down under debug builds.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
getpid() is async-signal-safe, and a cached PID goes stale after fork-without-exec. The inline call is simpler and fork-correct.
With only the main thread registered in MachineThreads, Bun.gc(true) has nothing to suspend and never issues an internal pthread_kill(SIGPWR), so the SI_TKILL passthrough was not actually exercised. A Worker gives the collector a second thread to suspend on every iteration.
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/js/node/process/process-sigpwr.test.ts`:
- Around line 108-124: The Worker readiness wait in the GC suspend/resume test
only resolves on the first message and can hang if the Worker fails before
posting it. Update the promise around the `new Worker(...)` setup in
`process-sigpwr.test.ts` so it rejects on Worker `error` and `close` (and any
other failure/termination signal used in this test harness) instead of waiting
indefinitely. Keep the change localized to the `test.concurrent` block and the
`w.addEventListener("message", ...)` readiness logic.
🪄 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: b713c19b-5cc4-4414-b4ac-71d63fec3bd2
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/js/node/process/process-sigpwr.test.ts
Workers get their own VM/Heap/MachineThreads in Bun, so the main VM's collector still had nothing to suspend. BUN_JSC_collectContinuously runs a dedicated collector thread in the same VM that suspends the main mutator via pthread_kill(SIGPWR), so a broken passthrough would hang.
SigintWatcher::install (reached via vm.runInNewContext with breakOnSigint) primes the signal ring buffer without ever populating signalNumberToNameMap, so a signal drained from the ring in that state would null-deref. Return early when the map has not been built; there is no listener to emit to anyway. Adds a SIGPWR test for this path.
There was a problem hiding this comment.
All my earlier feedback has been addressed and I found nothing further, but this interposes a process-wide signal handler on JSC's GC suspend/resume path — worth a human sanity-check on the si_code == SI_TKILL discriminator and the onDidChangeListeners sigaction skip.
Extended reasoning...
Overview
This PR wraps WTF's signalHandlerSuspendResume (SIGPWR on Linux) with a thin guard installed during JSCInitialize. The guard passes through deliveries with si_code == SI_TKILL && si_pid == getpid() to WTF unchanged and diverts everything else into Bun__onPosixSignal, so an external kill -PWR no longer null-derefs targetThread inside WTF. Alongside that: SIGPWR is added to the signal name/number tables, onDidChangeListeners now skips touching the sigaction for g_wtfConfig.sigThreadSuspendResume on both add and remove, and Bun__onSignalForJS gained a null-check on signalNumberToNameMap. A new 8-case Linux-only test file covers self-kill by number and name, external delivery, subprocess.kill, no-listener survival, breakOnSigint priming, listener add/remove, and a collectContinuously stress loop.
Security risks
None identified. The change narrows an existing crash into a handled event; the discriminator only widens what reaches WTF's handler when SI_TKILL from self, and everything else is routed to the existing JS signal queue. No new attack surface, no auth/crypto/permissions.
Level of scrutiny
High. The guard runs inside a signal handler on every GC thread suspension for every Linux process. Its correctness depends on (a) JSC continuing to deliver via pthread_kill (so the kernel reports SI_TKILL), (b) Bun__onPosixSignal being async-signal-safe, and (c) the onDidChangeListeners skip not leaving any path that resets the disposition to SIG_DFL. These are the kind of assumptions a maintainer familiar with the WebKit threading model and prior SIGPWR issues (#31158, #31832) should confirm — particularly whether the SI_TKILL-from-self gate is robust across the WebKit upgrade cadence.
Other factors
I left four inline comments on earlier revisions (cached PID → inline getpid(), single-threaded GC test not exercising the passthrough, per-VM MachineThreads meaning a Worker doesn't help, and the signalNumberToNameMap null-deref via SigintWatcher); all were addressed in 2f0d2d2, 08b822b/a87266e, and 533775d respectively. All CodeRabbit threads are resolved. The bug-hunting system found nothing on the current head. Test coverage looks thorough and the collectContinuously case now genuinely exercises the SI_TKILL passthrough. There is a known partial overlap with #33776 on the signal-name-table indices that will need a small rebase whichever lands second.
|
CI on 533775d: 283 passed, 1 failed. The only failure is This diff is Linux-only: the guard and its call site are under Ready for review; the Windows Postgres failure is infra, not this change. |
1 participant
What
Delivering SIGPWR (signal 30) to a Bun process on Linux segfaults the whole runtime:
Reachable three ways:
process.kill(pid, 30),subprocess.kill("SIGPWR"), orkill -PWR <pid>from outside the process. The last one matters because container managers (lxc stop, systemd) send SIGPWR to PID 1 on power events, so a Bun server running as PID 1 segfaults instead of shutting down cleanly. Under Node the same code runs the listener and exits 0.Cause
On Linux JSC uses SIGPWR to suspend and resume JS threads for conservative stack scanning.
WTF::Thread::signalHandlerSuspendResumeloads a file-statictargetThreadpointer and immediately dereferencesthread->m_suspendCount.targetThreadis only set by JSC's ownThread::suspend()/resume()right before theypthread_killthe target, so any SIGPWR delivered from outside that path seestargetThread == nullptrand reads offset 0x58 of null.src/jsc/bindings/c-bindings.cppalready documents that SIGPWR is reserved for JSC's GC, but nothing protects the delivery path.Fix
Install a thin wrapper over WTF's handler immediately after
WTF::initializeMainThread(). JSC's suspend/resume always delivers viapthread_killfrom our own process, which the kernel reports assi_code == SI_TKILLwithsi_pid == getpid(); that path is passed through unchanged. Anything else (all three repros above usekill(2), which isSI_USER) is forwarded into the JS signal queue instead of reaching WTF.Alongside that:
SIGPWRis added to the process signal name tables soprocess.kill(pid, "SIGPWR")andprocess.on("SIGPWR", fn)are wired up.onDidChangeListenersno longer touches thesigactionforg_wtfConfig.sigThreadSuspendResume; the wrapper is permanent and already feedsBun__onPosixSignal.The one intentional divergence from Node: with no listener registered, an unsolicited SIGPWR is ignored rather than terminating the process, because JSC still owns the disposition.
Verification
Covers self-kill by number and by name, external
process.killon a child,subprocess.kill("SIGPWR"), no-listener survival, listener add/remove not resetting the disposition, and a GC stress loop (Bun.gc(true)interleaved withprocess.kill(pid, 30)) to prove theSI_TKILLgate still lets JSC's suspend/resume through.