Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/jsc/bindings/c-bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,10 +933,13 @@
M(SIGIO);

#if OS(LINUX)
// SIGPWR is intentionally excluded: JSC's GC uses it to suspend/resume
// threads (see WTF/wtf/posix/ThreadingPOSIX.cpp). Overriding it with an
// SA_RESETHAND forwarder resets the disposition to SIG_DFL after the first
// GC suspend, and the next GC SIGPWR terminates the process.
#define FOR_EACH_LINUX_ONLY_SIGNAL(M) \
M(SIGPOLL); \
M(SIGPWR); \
M(SIGSTKFLT);

Check notice on line 942 in src/jsc/bindings/c-bindings.cpp

View check run for this annotation

Claude / Claude Code Review

Duplicate signal aliases (SIGIOT==SIGABRT, SIGPOLL==SIGIO) corrupt previous_actions restore 

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` h
Comment on lines 940 to 942

@claude claude Bot May 20, 2026

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.


#endif

Expand Down
40 changes: 40 additions & 0 deletions test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

// On Linux, JSC's GC uses SIGPWR to suspend/resume threads for conservative
// stack scanning. Bun.openInEditor spawns a detached thread that runs the
// internal sync-spawn path, which installs a temporary signal-forwarding
// handler for the signals it knows about. If SIGPWR is in that set,
// concurrent register/unregister races on the shared previous_actions[]
// array leave the SIGPWR disposition as SIG_DFL, and the next GC suspend
// terminates the process with SIGPWR.
test.skipIf(process.platform !== "linux")(
"sync-spawn signal forwarding does not override the GC's SIGPWR handler",
async () => {
const script = `
for (let i = 0; i < 100; i++) {
try { Bun.openInEditor("/tmp/__nonexistent__"); } catch {}
}
await Bun.sleep(200);
for (let g = 0; g < 100; g++) {
new Array(10000).fill({ x: g });
Bun.gc(true);
}
process.exit(0);
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
// Empty PATH / no EDITOR so auto-detect picks Editor::None and the
// background thread's posix_spawn fails immediately instead of
// launching a real editor.
env: { ...bunEnv, PATH: "", EDITOR: "", VISUAL: "" },
stdio: ["ignore", "ignore", "pipe"],
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
expect(proc.signalCode).not.toBe("SIGPWR");
expect(proc.signalCode).toBeNull();
expect(stderr).toBe("");
expect(exitCode).toBe(0);
},
);
Loading