Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
17 changes: 15 additions & 2 deletions src/jsc/bindings/c-bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -937,11 +937,14 @@
#if OS(LINUX) || OS(DARWIN) || OS(FREEBSD)
#include <signal.h>
#include <pthread.h>
#include <mutex>

// Note: We only ever use bun.spawnSync on the main thread.
// Bun.openInEditor runs spawnSync on detached threads, so register/unregister can overlap.
Comment thread
claude[bot] marked this conversation as resolved.
extern "C" int64_t Bun__currentSyncPID = 0;
static int Bun__pendingSignalToSend = 0;
static struct sigaction previous_actions[NSIG];
static std::mutex signalForwardingLock;
static int signalForwardingDepth = 0;

Check warning on line 947 in src/jsc/bindings/c-bindings.cpp

View check run for this annotation

Claude / Claude Code Review

static std::mutex diverges from WTF::Lock convention used by every other static lock in src/jsc/bindings/

nit: `static std::mutex signalForwardingLock` is the only `std::mutex` in all of `src/*.cpp` — every other namespace-scope static lock in `src/jsc/bindings/` (`BunDebugger.cpp:34`, `JSSQLStatement.cpp:256`, `NodeSqlite.cpp:867`, `ScriptExecutionContext.cpp:66`, `TextEncodingRegistry.cpp:121`) uses `WTF::Lock`, and REVIEW.md says "WTF:: containers over std:: in C++ bindings". `WTF::Lock` is also constexpr-init with a trivial destructor, so it never runs a static destructor during `exit()` while o
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

// npm's signal list minus SIGIOT/SIGPOLL (aliases of SIGABRT/SIGIO; listing both would overwrite previous_actions[N]).
// https://github.com/npm/cli/blob/fefd509992a05c2dfddbe7bc46931c42f1da69d7/workspaces/arborist/lib/signals.js#L26-L57
Expand Down Expand Up @@ -1004,6 +1007,10 @@

extern "C" void Bun__registerSignalsForForwarding()
{
std::lock_guard<std::mutex> lock(signalForwardingLock);
if (signalForwardingDepth++ != 0)
return;

Bun__pendingSignalToSend = 0;
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
Expand All @@ -1028,17 +1035,23 @@
#undef REGISTER_SIGNAL
}

extern "C" void Bun__unregisterSignalsForForwarding()
// Returns true when this was the outermost caller and the previous dispositions were restored.
extern "C" bool Bun__unregisterSignalsForForwarding()
{
Bun__currentSyncPID = 0;

std::lock_guard<std::mutex> lock(signalForwardingLock);
if (--signalForwardingDepth != 0)
return false;

#define UNREGISTER_SIGNAL(SIG) \
if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \
}

FOR_EACH_SIGNAL(UNREGISTER_SIGNAL)
memset(previous_actions, 0, sizeof(previous_actions));
#undef UNREGISTER_SIGNAL
return true;
}

#endif
Expand Down
5 changes: 3 additions & 2 deletions src/spawn/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2866,11 +2866,12 @@
}
}
#[cfg(unix)]
impl Drop for SignalForwarding {
fn drop(&mut self) {
Bun__unregisterSignalsForForwarding();
bun_crash_handler::reset_on_posix();
if Bun__unregisterSignalsForForwarding() {
bun_crash_handler::reset_on_posix();
}
}

Check warning on line 2874 in src/spawn/process.rs

View check run for this annotation

Claude / Claude Code Review

reset_on_posix() runs after signalForwardingLock is released, so an outermost Drop can stomp a concurrent register()

🟡 The bool-gate in da8ae772c7 fixes the inner-caller stomp, but the *outermost* caller's `reset_on_posix()` still runs after `Bun__unregisterSignalsForForwarding()` has released `signalForwardingLock`, so it can interleave with a fresh `register()` on another thread and overwrite its forwarding lambda for `SIGABRT`/`SIGTRAP` with the crash handler. Since the outermost unregister already restores `previous_actions[SIGABRT]`/`[SIGTRAP]` (the crash handler, installed at startup) and `SIGSEGV`/`SIGI
Comment thread
claude[bot] marked this conversation as resolved.
}

/// TTY job-control bridge for `--no-orphans` `bun run`. We put the script
Expand Down
3 changes: 2 additions & 1 deletion src/spawn_sys/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ pub mod ffi {
/// Install SIGINT/SIGTERM/… handlers that record the signal for
/// forwarding to [`Bun__currentSyncPID`].
pub safe fn Bun__registerSignalsForForwarding();
pub safe fn Bun__unregisterSignalsForForwarding();
/// Returns true if this was the outermost call (handlers were restored).
pub safe fn Bun__unregisterSignalsForForwarding() -> bool;

// macOS p_puniqueid descendant tracker — see NoOrphansTracker.cpp.
pub safe fn Bun__noOrphans_begin(kq: c_int, root: pid_t);
Expand Down
47 changes: 47 additions & 0 deletions test/js/bun/util/open-in-editor-gc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,50 @@ test.skipIf(!isLinux)("Bun.openInEditor does not break GC signal handling", asyn

await Promise.all(runs);
});

// Each detached editor thread goes through bun.spawnSync's signal-forwarding
// register/unregister, which swaps every forwarded signal's disposition in a
// process-global table. With many threads overlapping, an unsynchronized
// table ends up restoring SIG_DFL (or the forwarding handler itself) instead
// of the handler that was installed before the burst, so a JS signal
// listener registered beforehand silently stops working or kills the process.
test.skipIf(!isLinux)("Bun.openInEditor bursts do not drop previously installed signal handlers", async () => {
const sleep = ["/usr/bin/sleep", "/bin/sleep"].find(p => existsSync(p));
expect(sleep).toBeDefined();

using dir = tempDir("open-in-editor-signal-table", {
"run.js": `
let fired = false;
process.on("SIGUSR2", () => { fired = true; });

let spawned = 0;
for (let i = 0; i < 64; i++) {
try { Bun.openInEditor("0.1", { editor: ${JSON.stringify(sleep)} }); spawned++; } catch {}
}
if (spawned === 0) { console.log("no editor threads spawned"); process.exit(2); }

// Let every detached thread finish its sleep and unregister.
await Bun.sleep(500);
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

process.kill(process.pid, "SIGUSR2");
const deadline = Date.now() + 2000;
while (!fired && Date.now() < deadline) await Bun.sleep(5);
console.log(fired ? "ok" : "handler lost");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "run.js"],
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("ok");
expect(proc.signalCode).toBeNull();
expect(exitCode).toBe(0);
});
Loading