Skip to content

spawn: don't forward SIGPWR on Linux #30983

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
+31 −2
Closed

spawn: don't forward SIGPWR on Linux #30983

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
4 changes: 3 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,12 @@
M(SIGIO);

#if OS(LINUX)
// SIGPWR is intentionally excluded: JSC uses it for GC thread suspend/resume
// on Linux (see WTF/wtf/posix/ThreadingPOSIX.cpp). Replacing that handler here
// makes the next GC-sent SIGPWR terminate the process.
#define FOR_EACH_LINUX_ONLY_SIGNAL(M) \
M(SIGPOLL); \
M(SIGPWR); \
M(SIGSTKFLT);

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

View check run for this annotation

Claude / Claude Code Review

SIGPOLL/SIGIO and SIGIOT/SIGABRT aliasing corrupts previous_actions[] 

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`,
Comment on lines 939 to 941
Copy link
Copy Markdown
Contributor

@claude claude Bot May 18, 2026

Choose a reason for hiding this comment

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

🟣 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:

  1. REGISTER_SIGNAL(SIGABRT) → sigaction(6, &sa, &previous_actions[6]) — installs the forwarding handler, saves the original handler into previous_actions[6].
  2. 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 into previous_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

  1. User calls process.on('SIGIO', handler) — libuv installs a handler for signal 29.
  2. 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. ✗
  3. 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, ...).
  4. Something sends SIGIO to the process. Forwarding lambda runs with Bun__currentSyncPID == 0, sets Bun__pendingSignalToSend = 29, returns. SA_RESETHAND resets signal 29 to SIG_DFL. The user's handler never fires.
  5. The next SIGIO hits SIG_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.


#endif

Expand Down
2 changes: 1 addition & 1 deletion src/jsc/bindings/wtf-bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ extern "C" int uv_tty_reset_mode(void)

static void uv__tty_make_raw(struct termios* tio)
{
assert(tio != NULL);
ASSERT(tio != NULL);

#if defined __sun || defined __MVS__
/*
Expand Down
27 changes: 27 additions & 0 deletions test/js/bun/spawn/spawnSync.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,31 @@ describe("spawnSync", () => {
it.skipIf(!isPosix)("should use spawnSync optimizations when possible", () => {
expect([join(import.meta.dir, "spawnSync-counters-fixture.ts")]).toRun();
});

// On Linux, JSC uses SIGPWR to suspend/resume threads for GC. The spawnSync
// signal-forwarding table used to include SIGPWR, so a GC that fired while
// (or after) spawnSync ran would terminate the process with signal 30.
it.skipIf(process.platform !== "linux")("does not clobber the GC thread-suspend signal handler", () => {
const result = Bun.spawnSync({
cmd: [
bunExe(),
"-e",
`
for (let i = 0; i < 50; i++) {
Bun.spawnSync({ cmd: ["true"] });
Bun.gc(true);
}
for (let i = 0; i < 50; i++) {
Bun.spawnSync({ cmd: ["true"] });
}
Bun.gc(true);
`,
],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
});
expect(result.signalCode).toBeFalsy();
expect(result.exitCode).toBe(0);
});
});
Loading