-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Don't forward SIGPWR in spawnSync signal handling #31183
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.
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
Changes from all commits
68ee26c
1864a69
f6438e0
af8feba
eb61137
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -933,9 +933,12 @@ static struct sigaction previous_actions[NSIG]; | |
| M(SIGIO); | ||
|
|
||
| #if OS(LINUX) | ||
| // SIGPWR is intentionally excluded: JSC uses it for GC thread suspend/resume | ||
| // (see wtf/posix/ThreadingPOSIX.cpp). Overriding it here breaks GC and the | ||
| // SA_RESETHAND disposition leaves it at SIG_DFL after one delivery, which | ||
| // kills the process on the next collection. | ||
| #define FOR_EACH_LINUX_ONLY_SIGNAL(M) \ | ||
| M(SIGPOLL); \ | ||
| M(SIGPWR); \ | ||
| M(SIGSTKFLT); | ||
|
Comment on lines
+936
to
942
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Note (pre-existing, non-blocking): the underlying race on Extended reasoning...What the bug isThe PR description correctly identifies the root cause of the Fuzzilli crash: "With multiple concurrent Code path
The comment at c-bindings.cpp:912 — "We only ever use bun.spawnSync on the main thread" — is the invariant this code relies on for safety, and Why nothing prevents itThere is no lock, no refcount, and no atomic guard around Step-by-step proof (concrete interleaving)Take the test's own repro: 8 concurrent
After both threads exit, SIGINT/SIGTERM no longer point at ImpactFar less severe than the SIGPWR case this PR fixes — no crash, no process death. The realistic symptom is "TTY not restored on Ctrl-C after the user calls How to fix (follow-up)Either of:
Also worth updating or removing the stale "main thread only" comment at line ~912, since it documents an invariant that no longer holds. |
||
|
|
||
| #endif | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isLinux, tempDir } from "harness"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win Use This test writes one inline script file only to execute it immediately. Prefer passing that script via Suggested change-import { bunEnv, bunExe, isLinux, tempDir } from "harness";
+import { bunEnv, bunExe, isLinux } from "harness";
@@
- using dir = tempDir("open-in-editor-gc", {
- "run.js": `
+ const script = `
for (let i = 0; i < 8; i++) {
try { Bun.openInEditor("0.3", { editor: ${JSON.stringify(sleep)} }); } catch {}
}
// Wait for the detached editor threads to complete their register /
// unregister cycle, then for the scavenger to fire SIGPWR.
await Bun.sleep(1000);
Bun.gc(true);
console.log("alive");
- `,
- });
+ `;
@@
await using proc = Bun.spawn({
- cmd: [bunExe(), "run.js"],
+ cmd: [bunExe(), "-e", script],
env: bunEnv,
- cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});As per coding guidelines: Also applies to: 13-30 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional — running via
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🐇 📚 noted ✏️ Learnings added
🧠 Learnings used |
||
| import { existsSync, symlinkSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
|
|
||
| // On Linux, JSC uses SIGPWR to suspend/resume threads for GC and the libpas | ||
| // scavenger. Bun.openInEditor spawns a detached thread that goes through | ||
| // bun.spawnSync, whose signal-forwarding setup must not touch SIGPWR or the | ||
| // process is terminated the next time GC/scavenger fires. | ||
| test.skipIf(!isLinux)("Bun.openInEditor does not break GC signal handling", async () => { | ||
| const sleep = ["/usr/bin/sleep", "/bin/sleep"].find(p => existsSync(p)); | ||
| expect(sleep).toBeDefined(); | ||
|
|
||
| using dir = tempDir("open-in-editor-gc", { | ||
| "run.js": ` | ||
| const a = ${JSON.stringify(sleep)}; | ||
| const b = process.argv[2]; | ||
| // Alternate absolute editor paths so the cached editor name_storage is | ||
| // replaced each call while a detached editor thread may still be | ||
| // reading the previous one. | ||
| for (let i = 0; i < 8; i++) { | ||
| try { Bun.openInEditor("0.3", { editor: i % 2 ? b : a }); } catch {} | ||
| } | ||
| // Wait for the detached editor threads to complete their register / | ||
| // unregister cycle, then for the scavenger to fire SIGPWR. | ||
| await Bun.sleep(1000); | ||
| Bun.gc(true); | ||
| console.log("alive"); | ||
| `, | ||
| }); | ||
| // Second absolute path to the same binary so alternating calls take the | ||
| // `!eql_long(prev_name, ...)` branch in open_in_editor. Keep the basename | ||
| // `sleep` so BusyBox (Alpine) resolves the multi-call applet from argv[0]. | ||
| const sleep2 = join(String(dir), "sleep"); | ||
| symlinkSync(sleep!, sleep2); | ||
|
|
||
| const runs = Array.from({ length: 5 }, async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run.js", sleep2], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).toBe(""); | ||
| expect(stdout.trim()).toBe("alive"); | ||
| expect(proc.signalCode).toBeNull(); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| await Promise.all(runs); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 Pre-existing nit, since you're already curating this list:
SIGPOLLis an alias ofSIGIO(signal 29) on Linux, andSIGIOTis an alias ofSIGABRT(signal 6) on all POSIX targets — soBun__registerSignalsForForwarding()callssigaction()twice on the same number, the second call overwritesprevious_actions[N]with the forwarding handler we just installed, andBun__unregisterSignalsForForwarding()then "restores" the forwarding lambda instead of the original. Practical impact is low (Bun has no custom SIGABRT/SIGIO handler, and glibcabort()re-raises after the handler returns), but dropping the redundantSIGPOLLhere andSIGIOTfromFOR_EACH_POSIX_SIGNALwould make the save/restore actually round-trip.Extended reasoning...
What the bug is
On Linux,
SIGPOLLandSIGIOare the same signal number (29 on x86_64/aarch64 —asm-generic/signal.hliterally has#define SIGPOLL SIGIO), andSIGIOTandSIGABRTare the same signal number (6 —#define SIGIOT SIGABRT).FOR_EACH_POSIX_SIGNALlistsSIGABRT,SIGIOT, andSIGIO, andFOR_EACH_LINUX_ONLY_SIGNALadditionally listsSIGPOLL. So on Linux,FOR_EACH_SIGNALexpands the register/unregister macro twice for signal 6 and twice for signal 29 (and the SIGABRT/SIGIOT duplication applies on Darwin/FreeBSD too, since both are in the POSIX list).Step-by-step trace
In
Bun__registerSignalsForForwarding(), taking signal 29 on Linux:REGISTER_SIGNAL(SIGIO)→sigaction(29, &sa, &previous_actions[29])— installs the forwarding lambda and saves the original handler intoprevious_actions[29].REGISTER_SIGNAL(SIGPOLL)→sigaction(29, &sa, &previous_actions[29])— re-installs the same forwarding lambda, but the third argument now reads back the forwarding lambda we installed in step 1, overwriting the saved original.In
Bun__unregisterSignalsForForwarding():UNREGISTER_SIGNAL(SIGIO)→sigaction(29, &previous_actions[29], NULL)— "restores" the forwarding lambda (withSA_RESETHAND), not the original handler.UNREGISTER_SIGNAL(SIGPOLL)→ same thing again.memset(previous_actions, 0, sizeof(previous_actions))— the original handler is now gone for good.The exact same sequence happens for signal 6 via
SIGABRTthenSIGIOT.Why nothing else fixes it up
After
Bun__unregisterSignalsForForwarding(), theSignalForwardingDrop insrc/spawn/process.rscallscrash_handler::reset_on_posix(), but that only re-installs handlers forSIGSEGV/SIGILL/SIGBUS/SIGFPE(src/crash_handler/lib.rs:1717-1720) — it does not touchSIGABRTorSIGIO. So after the firstspawnSync/openInEditorcycle, signals 6 and 29 are left permanently pointing at the forwarding lambda withSA_RESETHAND.Impact
After one
spawnSynccompletes,Bun__currentSyncPID == 0, so when a SIGABRT or SIGIO arrives the leaked forwarding lambda just stashes it inBun__pendingSignalToSendand returns — the first delivery is swallowed, andSA_RESETHANDthen drops the disposition toSIG_DFLfor the second delivery.In practice this is low impact:
bun_initialize_processonly handles SIGINT/SIGTERM), so the "lost original" is typically justSIG_DFL.abort()re-raises SIGABRT in a loop after a handler returns, soabort()still terminates the process.kill -ABRT <pid>or a user-installed SIGIO handler would have its first delivery silently eaten, though.How to fix
Drop the redundant alias entries — remove
M(SIGPOLL);fromFOR_EACH_LINUX_ONLY_SIGNALandM(SIGIOT);fromFOR_EACH_POSIX_SIGNAL. Their canonical names (SIGIO,SIGABRT) are already in the list, so coverage is unchanged andprevious_actions[]round-trips correctly. The npm list this was copied from contains both because Node'sprocess.on(signalName, ...)is keyed by string name, not signal number, so duplicates are harmless there — but here they index a C array by signal number.This is pre-existing (the npm-derived list has always had these aliases) and not introduced or worsened by this PR, but the PR is editing the exact line where
SIGPOLLlives, so it's a natural "while you're here".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed — both this and the mutex/refcount for the remaining race are already in #30956. Keeping this PR scoped to the SIGPWR process-death case plus the UAF; the rest lands with that one.