Skip to content

Don't forward SIGPWR in sync-spawn signal forwarding#31131

Closed
robobun wants to merge 1 commit into
mainfrom
farm/51debdae/sigpwr-gc-forwarding
Closed

Don't forward SIGPWR in sync-spawn signal forwarding#31131
robobun wants to merge 1 commit into
mainfrom
farm/51debdae/sigpwr-gc-forwarding

Conversation

@robobun

@robobun robobun commented May 20, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Removes SIGPWR from the set of signals that Bun__registerSignalsForForwarding() overrides during sync-spawn (bun run, bunx, and the background thread of Bun.openInEditor()).

On Linux, JSC's GC uses SIGPWR (via g_wtfConfig.sigThreadSuspendResume in WTF/wtf/posix/ThreadingPOSIX.cpp) to suspend and resume threads for conservative stack scanning. The sync-spawn signal-forwarding path was installing an SA_RESETHAND forwarder for SIGPWR, which breaks the GC in two ways:

  1. If the GC sends SIGPWR while the forwarder is installed, the forwarder runs instead of WebKit's suspend handler. SA_RESETHAND then resets the disposition to SIG_DFL, and the next GC SIGPWR terminates the process.
  2. Bun.openInEditor() spawns a detached thread that runs the same sync-spawn path, so concurrent openInEditor calls race on the process-global previous_actions[] array: one thread's memset(previous_actions, 0, ...) runs before another's restore, which then installs SIG_DFL for every forwarded signal — including SIGPWR.

Either way the next GC thread-suspend terminates the process with SIGPWR (TERMSIG: 30).

SIGPWR is already excluded from process.on() for the same reason (BunProcess.cpp checks g_wtfConfig.sigThreadSuspendResume); this excludes it from the forwarding set as well.

Found by Fuzzilli (fingerprint 66c688589d402c9f).

How did you verify your code works?

Added test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts which:

  • Fires 100 concurrent Bun.openInEditor() calls (each spawns a background thread that runs the register/unregister cycle), waits for the race to settle, then runs a GC loop.
  • Before the fix: the subprocess terminates with SIGPWR 100% of the time on debug builds.
  • After the fix: the subprocess exits cleanly with code 0.

Also verified the original fuzzer repro no longer crashes over 30 runs.

On Linux, JSC's GC uses SIGPWR (via g_wtfConfig.sigThreadSuspendResume) to
suspend and resume threads for conservative stack scanning. The sync-spawn
signal-forwarding path (Bun__registerSignalsForForwarding) was installing
an SA_RESETHAND forwarder for SIGPWR, which both:

- resets the disposition to SIG_DFL after the first GC suspend, and
- races on the shared previous_actions[] array when called from the
  detached background thread spawned by Bun.openInEditor, leaving the
  disposition as SIG_DFL once the last unregister runs after a memset.

Either way, the next GC suspend terminates the process with SIGPWR.

SIGPWR is already excluded from process.on() for the same reason; exclude
it from the forwarding set too.
@robobun

robobun commented May 20, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:07 AM PT - May 20th, 2026

@robobun, your commit 0e310b4 has 1 failures in Build #56410 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31131

That installs a local version of the PR into your bun-31131 executable, so you can run:

bun-31131 --bun

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Prevents bun.spawnSync from registering the SIGPWR signal handler on Linux, allowing JavaScriptCore's GC to use SIGPWR for suspend/resume without process termination. Adds a test verifying spawned processes complete normally under forced GC.

Changes

SIGPWR Signal Handling

Layer / File(s) Summary
Exclude SIGPWR from spawnSync signal forwarding
src/jsc/bindings/c-bindings.cpp, test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts
The FOR_EACH_LINUX_ONLY_SIGNAL macro removes SIGPWR with comments explaining JSC uses it for GC suspend/resume. New Linux-only test verifies that Bun.openInEditor's sync-spawn path does not override the SIGPWR handler and completes successfully under forced GC conditions.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely summarizes the main change: excluding SIGPWR from signal forwarding in sync-spawn.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description comprehensively covers what the change does and how it was verified, following the required template structure.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. spawnSync: make signal-forwarding register/unregister safe for concurrent callers #30956 - Also removes SIGPWR from FOR_EACH_LINUX_ONLY_SIGNAL in c-bindings.cpp to fix the same JSC GC thread-suspend crash during sync spawn signal forwarding

🤖 Generated with Claude Code

@robobun

robobun commented May 20, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #30956, which has the more complete fix (also removes the aliased SIGIOT/SIGPOLL entries and adds a mutex + depth counter so concurrent register/unregister can't corrupt previous_actions[]). Closing in favor of that one.

This was fuzzer fingerprint 66c688589d402c9f; #30956 was 11551e051c08db2c — same root cause.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — removing SIGPWR from the forwarding set is the right call given JSC uses it for GC thread suspend/resume; left one non-blocking note about pre-existing SIGIOT/SIGPOLL aliases in the same list.

Extended reasoning...

Overview

This PR removes SIGPWR from the FOR_EACH_LINUX_ONLY_SIGNAL macro in src/jsc/bindings/c-bindings.cpp, so that Bun__registerSignalsForForwarding() no longer installs an SA_RESETHAND forwarder for it during sync-spawn. A 4-line comment documents the rationale, and a new Linux-only regression test (spawnSync-sigpwr-gc.test.ts) reproduces the crash via concurrent Bun.openInEditor() calls followed by forced GC. The production change is effectively a one-line deletion from a signal list.

Security risks

None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change strictly reduces the set of signals whose process-wide disposition Bun temporarily overrides.

Level of scrutiny

Signal-handling code is normally subtle, but this change is a pure subtraction: stop overriding one signal that the runtime's own GC depends on. The failure mode it fixes (process killed with TERMSIG: 30 when GC fires during/after sync-spawn signal forwarding) is well-explained, matches the existing exclusion of SIGPWR in BunProcess.cpp's process.on() path, and is consistent with g_wtfConfig.sigThreadSuspendResume usage in WebKit. The only behavioral "loss" is that a real SIGPWR sent to the parent during spawnSync is no longer relayed to the child — but SIGPWR is essentially never user-delivered (it's a UPS/power-fail signal, and on Linux is repurposed by glibc/JSC for thread suspension), so this is a non-concern.

Other factors

  • The regression test is Linux-gated, uses an empty PATH/EDITOR so openInEditor resolves to Editor::None and fails fast without launching anything, then asserts signalCode !== "SIGPWR" and exit code 0. Reasonable and self-contained.
  • The bug-hunting system flagged a pre-existing issue in the adjacent list entries (SIGIOT aliases SIGABRT, SIGPOLL aliases SIGIO, so previous_actions[] save/restore is corrupted for those slots). That's a valid observation and is posted as a non-blocking inline comment, but it predates this PR and doesn't affect the correctness of the SIGPWR removal.
  • No outstanding reviewer comments; CI build was triggered.

Given the minimal, well-justified diff and the added test, I'm comfortable approving without human review.

Comment on lines 940 to 942
#define FOR_EACH_LINUX_ONLY_SIGNAL(M) \
M(SIGPOLL); \
M(SIGPWR); \
M(SIGSTKFLT);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing, but since you're already pruning this list: SIGIOT is an alias for SIGABRT (both 6) and SIGPOLL is an alias for SIGIO (both 29 on Linux), so REGISTER_SIGNAL runs twice for the same previous_actions[N] slot — the second sigaction() overwrites the saved original handler with the forwarder we just installed, and Bun__unregisterSignalsForForwarding then "restores" the forwarder instead of the user's handler. Dropping SIGIOT from FOR_EACH_POSIX_SIGNAL and SIGPOLL here would fix it with the same rationale as the SIGPWR removal.

Extended reasoning...

What the bug is

FOR_EACH_POSIX_SIGNAL lists both SIGABRT and SIGIOT, and on Linux FOR_EACH_SIGNAL adds SIGPOLL on top of the SIGIO already in the POSIX list. On Linux these are aliases for the same signal number — SIGABRT == SIGIOT == 6 and SIGIO == SIGPOLL == 29 (verified against /usr/include/asm-generic/signal.h and by compiling a test program). The npm list this was copied from uses string signal names so duplicates were harmless there, but here they're integer macros that collide on the same previous_actions[] index.

The code path that triggers it

REGISTER_SIGNAL(SIG) expands to sigaction(SIG, &sa, &previous_actions[SIG]). So Bun__registerSignalsForForwarding() does, in order:

  1. sigaction(SIGABRT /*6*/, &sa, &previous_actions[6]) — installs the forwarder, saves the original SIGABRT handler into previous_actions[6].
  2. …other signals…
  3. sigaction(SIGIOT /*6*/, &sa, &previous_actions[6]) — installs the forwarder again (no-op), but the old action returned is now the forwarder that step 1 just installed, which overwrites the original handler saved in previous_actions[6].

The same thing happens for SIGIO (step N) followed by SIGPOLL (step N+k) at index 29.

Then in Bun__unregisterSignalsForForwarding(), UNREGISTER_SIGNAL(SIGABRT) and UNREGISTER_SIGNAL(SIGIOT) both write previous_actions[6] — the forwarder — back as the active handler. The original handler is gone.

Why nothing repairs it

The only thing that runs after unregister is crash_handler.resetOnPosix() (process.zig:2400), which only re-installs handlers for SIGSEGV/SIGILL/SIGBUS/SIGFPE. It does not touch SIGABRT or SIGIO, so the corruption persists.

Step-by-step proof of impact

  1. User does process.on('SIGABRT', handler) → libuv installs a SIGABRT handler.
  2. User calls Bun.spawnSync(...)Bun__registerSignalsForForwarding() runs. After the SIGIOT iteration, previous_actions[6] holds the forwarder lambda with SA_RESETHAND, not libuv's handler.
  3. spawnSync finishes → Bun__unregisterSignalsForForwarding() "restores" previous_actions[6], i.e. re-installs the forwarder. crash_handler.resetOnPosix() doesn't touch SIGABRT.
  4. Something sends SIGABRT to the process. The forwarder runs, sees Bun__currentSyncPID == 0, stashes the signal in Bun__pendingSignalToSend, and returns — the signal is silently swallowed. SA_RESETHAND then resets SIGABRT to SIG_DFL.
  5. The user's process.on('SIGABRT') handler never fires, and the next SIGABRT core-dumps the process via SIG_DFL.

Same story for SIGIO/SIGPOLL at index 29.

How to fix

Drop the redundant alias entries: remove M(SIGIOT); from FOR_EACH_POSIX_SIGNAL (it's identical to SIGABRT on every platform we support) and remove M(SIGPOLL); from FOR_EACH_LINUX_ONLY_SIGNAL (it's identical to SIGIO on Linux). This is the same kind of one-line list cleanup as the SIGPWR removal in this PR.

This is pre-existing — not introduced by this PR — but the PR edits the line directly adjacent to SIGPOLL in FOR_EACH_LINUX_ONLY_SIGNAL for the same class of "this signal shouldn't be in the forwarding list" reason, so it seemed worth flagging while you're here. Not blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant