spawn: don't forward SIGPWR on Linux#30983
Conversation
On Linux, JSC uses SIGPWR to suspend/resume mutator threads for GC (WTF/wtf/posix/ThreadingPOSIX.cpp). Bun__registerSignalsForForwarding() was installing a SA_RESETHAND forwarding handler for SIGPWR, replacing JSC's signalHandlerSuspendResume process-wide for the duration of each sync::spawn(). When multiple threads call sync::spawn() concurrently (e.g. via Bun.openInEditor, which spawns a detached thread per call), the register/unregister pair races on the global previous_actions[] array: one thread's unregister memsets the saved handler to zero, so the next unregister restores SIG_DFL for SIGPWR. The next GC-sent SIGPWR then terminates the process with signal 30. The signal list was copied from npm, which runs on Node.js and has no special use for SIGPWR. Drop SIGPWR from the Linux forwarding list. Also replace a stray lowercase assert() in wtf-bindings.cpp with WTF's ASSERT so the file compiles standalone in a unified build.
|
Updated 7:20 AM PT - May 18th, 2026
❌ @robobun, your commit 80b86f4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30983That installs a local version of the PR into your bun-30983 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR addresses a GC signal issue in ChangesspawnSync GC signal handling fix
Assertion macro consistency
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30956, which is the more complete fix (also removes the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-justified removal of SIGPWR from the Linux forwarding list with a regression test; the SIGIOT/SIGPOLL aliasing note is pre-existing and a reasonable follow-up.
Extended reasoning...
Overview
This PR removes SIGPWR from the FOR_EACH_LINUX_ONLY_SIGNAL macro in c-bindings.cpp so that Bun__registerSignalsForForwarding() no longer overwrites JSC's GC thread-suspend handler during spawnSync. It adds an explanatory comment, a Linux-only regression test in spawnSync.test.ts that loops spawnSync + Bun.gc(true), and swaps a stray lowercase assert() for WTF's ASSERT in wtf-bindings.cpp.
Security risks
None. The change is purely subtractive — one fewer signal has its process-wide handler temporarily replaced. No new inputs, no privilege changes, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. Signal handling is runtime-critical, but this is a one-line deletion from a static list with an unambiguous root cause: JSC documents g_wtfConfig.sigThreadSuspendResume = SIGPWR on Linux, and the npm-derived list never accounted for that. Removing SIGPWR can only reduce side effects; there's no plausible regression from not forwarding a power-failure signal to a child process. The assert → ASSERT change is a trivial macro consistency fix.
Other factors
- The fix is backed by a fuzzer repro and fingerprint, with a clear before/after.
- A targeted regression test is included and scoped to Linux via
skipIf. - The one bug-hunter finding (SIGIOT≡SIGABRT and SIGPOLL≡SIGIO causing double-registration that clobbers
previous_actions[]) is explicitly pre-existing, not introduced here, and is surfaced as an inline follow-up suggestion rather than a blocker. - No prior reviewer comments to address; CI build is in progress.
| #define FOR_EACH_LINUX_ONLY_SIGNAL(M) \ | ||
| M(SIGPOLL); \ | ||
| M(SIGPWR); \ | ||
| M(SIGSTKFLT); |
There was a problem hiding this comment.
🟣 Pre-existing issue, but same bug class as the SIGPWR fix and in the macro being edited: SIGIOT is an alias for SIGABRT (everywhere) and SIGPOLL is an alias for SIGIO (Linux), so FOR_EACH_SIGNAL expands REGISTER_SIGNAL twice for the same signal number. The second sigaction() call overwrites previous_actions[N] with the just-installed forwarding handler, so Bun__unregisterSignalsForForwarding "restores" the SA_RESETHAND forwarder instead of the original — after every spawnSync, any user-installed SIGIO/SIGABRT handler is permanently lost. Dropping SIGIOT from FOR_EACH_POSIX_SIGNAL and SIGPOLL from FOR_EACH_LINUX_ONLY_SIGNAL would fix it.
Extended reasoning...
What the bug is
On Linux, SIGIOT is #defined to SIGABRT (both signal 6) and SIGPOLL is #defined to SIGIO (both signal 29). On Darwin and FreeBSD, SIGIOT == SIGABRT as well. Yet FOR_EACH_POSIX_SIGNAL lists both SIGABRT and SIGIOT (and SIGIO), and FOR_EACH_LINUX_ONLY_SIGNAL — the macro this PR edits — still lists SIGPOLL. So FOR_EACH_SIGNAL(REGISTER_SIGNAL) calls sigaction() twice for signal 6, and on Linux twice for signal 29.
The code path that triggers it
REGISTER_SIGNAL(SIG) expands to sigaction(SIG, &sa, &previous_actions[SIG]). The third argument is the output slot where the kernel writes the previously-installed handler. When the macro expands twice for the same signum:
REGISTER_SIGNAL(SIGABRT)→sigaction(6, &sa, &previous_actions[6])— installs the forwarding handler, saves the original handler intoprevious_actions[6].REGISTER_SIGNAL(SIGIOT)→sigaction(6, &sa, &previous_actions[6])— installs the forwarding handler again (no-op), but now saves the forwarding handler from step 1 intoprevious_actions[6], overwriting the original.
The same happens for SIGIO (in FOR_EACH_POSIX_SIGNAL) followed by SIGPOLL (in FOR_EACH_LINUX_ONLY_SIGNAL) on Linux.
Then Bun__unregisterSignalsForForwarding does sigaction(SIG, &previous_actions[SIG], NULL) for each signal — for 6 and 29, this "restores" the forwarding handler (with SA_RESETHAND) instead of whatever was there before spawnSync.
Why existing code doesn't prevent it
The npm signal list this was copied from is a JS array of string names iterated with process.on(name, ...); Node tolerates duplicate names because the second process.on just adds another listener. But this C++ implementation indexes previous_actions[] by signal number, so aliased names collide on the same array slot. There's no dedup check — the second sigaction() for the same number silently clobbers the saved original.
Impact
After every spawnSync completes, the process is left with a one-shot (SA_RESETHAND) forwarding handler for SIGABRT and (on Linux) SIGIO, instead of the original disposition. With Bun__currentSyncPID == 0 post-spawn, the next delivery of either signal is silently swallowed (the lambda just sets Bun__pendingSignalToSend and returns), then SA_RESETHAND resets it to SIG_DFL. Concretely: a user who installs process.on('SIGIO', ...) or process.on('SIGABRT', ...) loses that handler permanently after the first spawnSync call.
Step-by-step proof
- User calls
process.on('SIGIO', handler)— libuv installs a handler for signal 29. - User calls
Bun.spawnSync(...)→Bun__registerSignalsForForwarding():M(SIGIO)→sigaction(29, &fwd, &previous_actions[29])—previous_actions[29]= libuv's handler. ✓M(SIGPOLL)→sigaction(29, &fwd, &previous_actions[29])—previous_actions[29]= the forwarding handler. ✗
- Child exits →
Bun__unregisterSignalsForForwarding():M(SIGIO)→sigaction(29, &previous_actions[29], NULL)— installs the forwarding handler (not libuv's).M(SIGPOLL)→ same again.memset(previous_actions, 0, ...).
- Something sends
SIGIOto the process. Forwarding lambda runs withBun__currentSyncPID == 0, setsBun__pendingSignalToSend = 29, returns.SA_RESETHANDresets signal 29 toSIG_DFL. The user's handler never fires. - The next
SIGIOhitsSIG_DFL(ignored on Linux). The user's handler is gone for good.
The SIGABRT/SIGIOT pair behaves identically on all three POSIX platforms.
How to fix
Drop the aliased duplicates: remove M(SIGIOT) from FOR_EACH_POSIX_SIGNAL (it's SIGABRT everywhere we build) and remove M(SIGPOLL) from FOR_EACH_LINUX_ONLY_SIGNAL (it's SIGIO on Linux). Alternatively, guard REGISTER_SIGNAL to skip a signum whose previous_actions[SIG] slot has already been populated this round.
Relationship to this PR
This is pre-existing — not introduced here. But it's the identical previous_actions[] corruption class the PR is fixing for SIGPWR, and the PR edits FOR_EACH_LINUX_ONLY_SIGNAL (which contains the SIGPOLL duplicate) directly, so it's worth flagging as a natural follow-up while this code is being touched.
1 participant
On Linux, JSC uses
SIGPWRto suspend/resume mutator threads for GC (seeWTF/wtf/posix/ThreadingPOSIX.cpp):Bun__registerSignalsForForwarding()was installing aSA_RESETHANDforwarding handler forSIGPWR, replacing JSC'ssignalHandlerSuspendResumeprocess-wide for the duration of eachsync::spawn().When multiple threads call
sync::spawn()concurrently (e.g. viaBun.openInEditor, which spawns a detached thread per call that runssync::spawnfor the editor process), the register/unregister pair races on the globalprevious_actions[]array: one thread's unregistermemsets the saved handler to zero, so the next thread's unregister restoresSIG_DFLforSIGPWR. The next GC-sentSIGPWRthen terminates the process with signal 30.The signal list was copied from npm, which runs on Node.js and has no special use for
SIGPWR. Bun does, so dropSIGPWRfrom the Linux forwarding list — there is no legitimate use case for forwarding a power-failure signal to a child script anyway.Repro
Before: process killed by
SIGPWR(exit 158) during or after cleanup.After: exits cleanly.
Also replaces a stray lowercase
assert()inwtf-bindings.cppwith WTF'sASSERTso the file compiles standalone in a unified build (pre-existing break exposed when the PCH is regenerated).Found by Fuzzilli. Fingerprint
0166e0b96fc64e94.