Restore Bun's signal handlers across dlopen() - #29844
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughPreserves Bun's POSIX signal-handler state across dynamic library loads by snapshotting handlers immediately before dlopen/std.DynLib.open and restoring them after the load attempt; adds POSIX-gated C entry points and wrappers invoked from the FFI/dlopen path and introduces tests exercising Go c-shared library behavior. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 9 minutes and 48 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/c-bindings.cpp`:
- Around line 953-1002: The global snapshot arrays bun_dlopen_saved_actions and
bun_dlopen_saved_valid are not thread-safe; serialize save/restore around dlopen
by adding a process-wide lock (e.g. a static std::mutex like bun_dlopen_mutex)
and acquiring a lock_guard at the start of Bun__saveSignalHandlersForDlopen and
Bun__restoreSignalHandlersAfterDlopen so the save → dlopen → restore sequence
cannot interleave across threads; ensure to include <mutex> and keep the lock
held while the caller performs dlopen (or document that callers must hold it
across the dlopen call), or alternatively implement a per-call snapshot stack
protected by the same mutex to avoid overwrites of the arrays.
- Around line 978-997: The change-detection misses differences in the
blocked-signal mask (sa_mask); update the comparison logic around
bun_dlopen_saved_actions and current (the sigaction struct read by sigaction) to
also consider sa_mask when deciding if a handler was changed. Specifically, when
computing changed for both the SA_SIGINFO and non-SA_SIGINFO branches, add a
comparison of current.sa_mask vs bun_dlopen_saved_actions[sig].sa_mask (e.g.,
memcmp or equivalent on sigset_t) so any differences in sa_mask trigger
restoration via sigaction(sig, &bun_dlopen_saved_actions[sig], nullptr); keep
the existing comparisons of sa_handler/sa_sigaction and sa_flags.
In `@test/regression/issue/29843.test.ts`:
- Around line 134-136: Remove the empty-stderr assertions for subprocesses
spawned with bunExe()/bunEnv: specifically delete or skip the
expect(stderr).toBe("") checks around the Promise.all result (variables stdout,
stderr, exitCode from proc.stdout.text(), proc.stderr.text(), proc.exited) and
leave the existing stdout and exitCode assertions intact; also apply the same
removal to the similar assertions around lines covering the other case (the
block noted at 191-196) so tests don't fail on debug ASAN JSC signal-handler
warnings printed to stderr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb27aaeb-72de-49e1-afcf-f3bf782fad14
📒 Files selected for processing (4)
src/bun.js/api/ffi.zigsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/c-bindings.cpptest/regression/issue/29843.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find bugs, but this introduces a process-wide policy of silently reverting signal handlers installed by any dlopen'd library (native addons included) — that's a behavior/design judgment with potential to break addons that legitimately install handlers, so it deserves a human look.
Extended reasoning...
Overview
This PR wraps both dlopen() call sites (bun:ffi in src/bun.js/api/ffi.zig and process.dlopen in src/bun.js/bindings/BunProcess.cpp) with new Bun__saveSignalHandlersForDlopen / Bun__restoreSignalHandlersAfterDlopen helpers in src/bun.js/bindings/c-bindings.cpp. The helpers snapshot all sigactions before the load and, for any signal that wasn't SIG_DFL pre-load, restore Bun's handler if the library changed it. A new regression test compiles a C shared library that mimics Go c-shared's constructor-time sigaction() storm and asserts via /proc/self/status and a SIGUSR1 round-trip that Bun's dispositions survive.
Security risks
None directly — this is defensive signal-state restoration. No new attack surface, no untrusted input parsing, no auth/permissions changes.
Level of scrutiny
High. This is a process-wide behavior change to signal handling that applies to every native addon and FFI library, not just Go c-shared. The policy "silently revert any handler the library installed for a signal Bun was already handling" is a design judgment with non-obvious tradeoffs:
- Native addons that intentionally install
SIGTERM/SIGINT/crash handlers will have them silently undone (Bun installsonExitSignalforSIGTERM/SIGINTwhen stdio is a TTY, so those slots are non-SIG_DFL). - For the motivating Go case, restoring Bun's
SIGSEGV/SIGBUS/SIGFPEhandlers over Go's means Go's nil-pointer-deref → panic recovery no longer works inside the loaded library — segfaults will hit Bun's crash handler instead. That may be the right call, but it's a tradeoff a maintainer should sign off on. - The global snapshot buffers aren't thread-safe (CodeRabbit also flagged this); both
process.dlopenandbun:ffican be invoked from workers.
Other factors
The implementation itself is straightforward and well-commented, the tests are thorough, and the immediate fix (preserving SIGPIPE=SIG_IGN) is clearly correct. CodeRabbit raised three points (thread safety, sa_mask comparison, stderr assertions in ASAN builds) that are worth a maintainer's eye but aren't blockers on their own. The reason I'm deferring rather than approving is the breadth of the policy change, not code quality.
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
|
@claude[bot] thanks for the design review. On the key concerns: Restoring over Go's SEGV/BUS/FPE: Yes — if a Go c-shared library segfaults inside Go code, Bun's crash handler catches it instead of Go's runtime recovering into a panic. That's the intentional tradeoff: the motivating case is that Bun itself (JSC JIT, our own native code, other addons) segfaults need reliable backtraces, and we can't give that up globally just because one loaded library installed a SEGV handler. Users who genuinely want Go's recover-from-segfault in cgo will typically bridge through an HTTP sidecar or subprocess anyway (per the issue reporter's own workaround). Silently reverting addon handlers: The Thread safety + sa_mask + stderr flakiness: Addressed in 4289c39 per the coderabbit thread — |
There was a problem hiding this comment.
The implementation looks sound and well-tested, but this changes process-global signal handling and holds a mutex across the entire dlopen() call (serialising all FFI/native-addon loads process-wide, including across workers) — worth a human sanity-check on that tradeoff and on the "library keeps SIG_DFL signals" policy before merging.
Extended reasoning...
Overview
This PR snapshots and restores sigaction state around both dlopen paths (bun:ffi in src/bun.js/api/ffi.zig and process.dlopen in src/bun.js/bindings/BunProcess.cpp), implemented via two new C functions in src/bun.js/bindings/c-bindings.cpp. It fixes a real bug (#29843) where Go c-shared libraries clobber Bun's SIGPIPE/crash/user signal handlers. Two regression tests are added.
Security risks
No injection/auth/data-exposure concerns. However, signal handling is runtime-critical: it governs crash reporting (SEGV/ILL/BUS/FPE), SIGPIPE behaviour for the networking stack, and Ctrl-C/SIGTERM delivery. A bug here could mask crashes, deadlock dlopen, or change how the process responds to termination signals after loading a native library.
Level of scrutiny
High. This is not a mechanical change — it:
- Introduces a
std::mutexthat is locked inBun__saveSignalHandlersForDlopen()and unlocked inBun__restoreSignalHandlersAfterDlopen(), held for the full duration of the foreign library's constructor. Both call sites pair save/restore correctly (Zigdefer, straight-line C++ around a non-throwingdlopen), but this serialises every dlopen across all workers and means a slow/hung library constructor blocks all other native loads. - Encodes a policy decision: signals that were
SIG_DFLpre-dlopen are left to the loaded library. That's necessary for Go's SIGURG, but it also means Go keeps SIGHUP/SIGINT/SIGTERM/SIGCHLD when Bun hadn't yet installed handlers for them — a human familiar with Bun's signal model should confirm that's the intended tradeoff. - Forcibly reverts handlers the loaded runtime (e.g. Go) believes it owns, which could in principle affect that runtime's own signal-dependent behaviour.
Other factors
CodeRabbit's earlier feedback (thread-safety, sa_mask comparison, ASAN stderr in tests) was addressed in 4289c39. The remaining inline finding is a minor test-convention nit (explicit { timeout: 30_000 }). The change is well-reasoned and well-commented, but the combination of process-global signal mutation + cross-function mutex + behavioural policy is exactly the kind of thing that benefits from a maintainer's eyes rather than bot approval.
Moved the cc compilation to beforeAll so each test body is just the
subprocess spawn, which fits the default per-test timeout with headroom
on ASAN CI lanes. Removes the explicit { timeout: 30_000 } options I
added in the previous commit — test/CLAUDE.md is clear that tests
should rely on the runner built-in timeout.
Flagged by claude[bot] on PR #29844.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29843.test.ts`:
- Around line 163-199: The test currently exercises bun:ffi dlopen but does not
invoke the BOM/compat path; update the test in issue/29843.test.ts to also call
the JS wrapper Process_functionDlopen (process.dlopen()) so the BunProcess.cpp
compat layer is exercised: either add a sibling subprocess case that runs the
same fixture but uses process.dlopen(libPath, { version: ... }) or replace the
existing fixture with one that calls process.dlopen() and performs the same
signal delivery/assertion, ensuring the test references process.dlopen() and
thereby covers the Process_functionDlopen path.
- Around line 24-79: Replace the use of tmpdirSync in the test fixture setup
with harness's tempDir pattern: declare a tempDir handle in the outer scope
(next to libPath), call tempDir("issue-29843-") inside beforeAll and use its
.path when writing/compiling the C file and building libPath, and add an
afterAll that calls the tempDir handle's dispose() to clean up; remove
tmpdirSync import and references, and keep function names unchanged (beforeAll,
afterAll, libPath).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac50fb60-0936-42b5-819d-a0dfbd3e83b5
📒 Files selected for processing (2)
src/bun.js/bindings/c-bindings.cpptest/regression/issue/29843.test.ts
5b29605 to
5759bfd
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/cli/run/die-with-parent.test.ts:19— Nit: this module-leveltempDir()is never disposed — there's nousingand noafterAll(() => fixture[Symbol.dispose]()), so the directory is left inos.tmpdir()after the file finishes (the same file correctly usesusing dir = tempDir(...)at line ~245). A one-lineafterAllwould match what was done for29843.test.tsin e0eb3df earlier in this PR.Extended reasoning...
What
tempDir()(test/harness.ts:281) returns aDisposableStringwhose only cleanup path is its[Symbol.dispose]/[Symbol.asyncDispose]method, which callsfs.rmSync(path, { recursive: true, force: true }). There is no process-exit auto-cleanup; if the disposer is never invoked, the directory stays on disk.At line 19,
const fixture = tempDir("die-with-parent", { ... })is declared at module scope with a plainconst— nousing, andafterAllis not even imported (line 1 only pulls inexpect, test). Nothing in the file ever callsfixture[Symbol.dispose](), so the four small fixture files (grandchild.js,child.js,child-nonbun.js,clean-exit.js) are left underos.tmpdir()every time this test file runs.Why it's inconsistent
This is the only module-level
const x = tempDir(...)in the entiretest/tree — every other call site either usesusinginside a test/describe body or pairs an outer-scope handle with an explicitafterAlldispose. The same file correctly doesusing dir = tempDir("die-with-parent-bunfig", {...})inside the bunfig test at line ~245, so the file is internally inconsistent about its own cleanup.More to the point, this exact convention was already enforced once on this PR: CodeRabbit comment 3157698259 flagged
tmpdirSyncin29843.test.tsand the fix in e0eb3df switched it totempDir(...)plusafterAll(async () => { await dir[Symbol.asyncDispose](); }). This file should follow the same pattern.Step-by-step
- Test runner loads
die-with-parent.test.ts. Module evaluation hits line 19 →tempDir("die-with-parent", {...})→tempDirWithFiles→fs.mkdtempSynccreates e.g./tmp/die-with-parent_abc123/and writes 4 fixture files into it. - The returned
DisposableStringis stored infixture. Its[Symbol.dispose]wouldrmSyncthe directory, but nothing ever calls it. - All tests run, referencing
String(fixture)for paths. - The test file completes. No
afterAllis registered forfixture. The runner moves on;/tmp/die-with-parent_abc123/remains on disk until the OS tmp cleaner reclaims it.
Impact
Negligible in practice — a handful of tiny JS files left in OS tmp per CI run, eventually swept by the OS. Not a correctness issue. Flagging as a nit for consistency with the harness convention and with the cleanup already applied to
29843.test.tsin this same PR.Fix
Add
afterAllto thebun:testimport on line 1 and append:afterAll(() => fixture[Symbol.dispose]());
after the
fixturedeclaration (or move thetempDircall into abeforeAllpaired with anafterAlldispose, as in29843.test.ts). - Test runner loads
-
🟡
test/bundler/bun-build-api.test.ts:1172-1177— Nit: per rootCLAUDE.md("Assert the exit code last"),expect(exitCode).toBe(0)should follow the stdout-derived assertions. Here it runs at line 1173 beforeJSON.parse(stdout.trim())/expect(growth).toBeLessThan(...)(1174–1177); same intest/js/web/fetch/fetch-redirect.test.ts:95beforeJSON.parse(stdout)/expect(secondHalfMiB).toBeLessThan(12)(97–104). The other new tests in this PR (node-tls-connect, transform-stream-leak, html-rewriter-leak, performance-observer-leak, zlib-onerror-reentrancy, fs.watch) already putexitCodelast, and 29843.test.ts was fixed for the same reason in cef0d0f — these two are the outliers.Extended reasoning...
What
Root
CLAUDE.mddocuments the subprocess-test convention twice:When spawning processes, tests should
expect(stdout).toBe(...)BEFOREexpect(exitCode).toBe(0). This gives you a more useful error message on test failure.and, in the canonical example block:
// Assert the exit code last.
Two of the new leak tests in this PR run
expect(exitCode).toBe(0)before the assertions derived from stdout:test/bundler/bun-build-api.test.ts:1173—expect(exitCode).toBe(0)precedesconst { growth } = JSON.parse(stdout.trim())(1174) andexpect(growth).toBeLessThan(400 * 1024 * 1024)(1177).test/js/web/fetch/fetch-redirect.test.ts:95—expect(exitCode).toBe(0)precedesconst { rss0, rss1, rss2 } = JSON.parse(stdout.trim())(97) andexpect(secondHalfMiB).toBeLessThan(12)(104).
Why it matters
This is a diagnostic-quality concern, not a correctness bug — the tests pass and fail in exactly the same scenarios either way. The convention exists so that when the subprocess misbehaves, the bun:test failure diff surfaces the content (the RSS/growth JSON the child printed, or the lack thereof) rather than a bare "expected 0, received N". For these leak tests in particular, the regression manifests as a number (e.g.
growth: 6.3e8,secondHalfMiB ≈ 21) — surfacing that number in the failure message is the actionable signal.In both files
expect(stderr).toBe("")already runs first (1172 / 94), which partially mitigates the concern (a crashing subprocess would surface its stderr first). But the stdout-derived growth/RSS assertion still comes afterexitCode, which is what the documented convention is about.Step-by-step proof
Take
bun-build-api.test.tsand suppose the subprocess exits non-zero (e.g.Bun.buildthrows inside the fixture, exit code 1) with empty stderr but partial JSON on stdout:- Line 1171 collects
stdout = '{"before":...'(truncated),stderr = "",exitCode = 1. - Line 1172
expect(stderr).toBe("")passes. - Line 1173
expect(exitCode).toBe(0)fails first → bun:test reports "expected 0, received 1" and stops. - The
JSON.parseSyntaxError naming what was actually printed (1174), and the growth value (1177), never run.
With the convention applied (exitCode last), step 4 runs first and the failure diff shows the actual stdout content / growth number, which is what you'd want when triaging a CI failure on a leak test.
The same trace applies to
fetch-redirect.test.ts: if the subprocess prints{"rss0":...,"rss1":...,"rss2":...}and then exits non-zero, the current ordering reports "expected 0, received 1" instead of surfacingsecondHalfMiB ≈ 21.Internal consistency
This PR has already accepted and applied this exact feedback: inline-comment 3158109207 flagged the same pattern in
test/regression/issue/29843.test.ts, and the author fixed it in cef0d0f ("Moved expect(exitCode).toBe(0) to after the stdout/JSON assertions"). The other new tests added in this PR —node-tls-connect.test.ts,transform-stream-leak.test.ts,performance-observer-leak.test.ts,html-rewriter-leak.test.ts,zlib-onerror-reentrancy.test.ts,fs.watch.test.ts— all already putexpect(exitCode).toBe(0)last. These two files are the outliers within the PR.Fix
Move
expect(exitCode).toBe(0);to be the last assertion in each test:// bun-build-api.test.ts expect(stderr).toBe(""); const { growth } = JSON.parse(stdout.trim()); expect(growth).toBeLessThan(400 * 1024 * 1024); expect(exitCode).toBe(0);
// fetch-redirect.test.ts expect(stderr).toBe(""); const { rss0, rss1, rss2 } = JSON.parse(stdout.trim()); const secondHalfMiB = (rss2 - rss1) / 1024 / 1024; expect(secondHalfMiB).toBeLessThan(12); expect(exitCode).toBe(0);
|
CI on build #48998 (and prior #48896, #48966) is hitting the same repo-wide flake pattern every merged PR is seeing — fetch-http2-client ASAN timeout, Windows serve-stream-reject-flush-leak / rspack / websocket-server / dev-and-prod HMR, bake stress, bun-install-registry, astro. None reference #29843, dlopen, sigaction, or signal-handling code. Representative recent merges hitting the identical flakes: #29899 at build #48934, #29915 at build #48944, #29901 at build #48919. This PR is POSIX-only ( Stopping further retrigger loops. Flagging for maintainer — merge decision looks blocked on repo-wide flake burden, not anything in this PR. |
Loading a Go `-buildmode=c-shared` library via bun:ffi dlopen() (or
process.dlopen) hung Prisma's MariaDB queries. Go's c-shared init
bulk-installs its own sigaction handlers for SIGURG, SIGPIPE, SIGCHLD,
SIGHUP, SIGINT, SIGTERM, SIGABRT, SIGSEGV, SIGILL, SIGBUS, SIGFPE, SIGTRAP,
SIGQUIT — clobbering:
- Bun's SIGPIPE = SIG_IGN (the networking stack depends on this)
- Bun's crash handlers on SEGV/ILL/BUS/FPE
- any process.on("SIG…") handlers the user registered
Snapshot sigactions before dlopen and restore them afterwards. A signal
whose pre-dlopen action was SIG_DFL is intentionally left alone, so the
loaded library can still claim signals Bun doesn't manage — most notably
SIGURG, which Go needs for goroutine preemption.
Wrapped both entry points: FFI.open in src/bun.js/api/ffi.zig and
Process_functionDlopen in src/bun.js/bindings/BunProcess.cpp. The
implementation lives in c-bindings.cpp alongside the pre-existing
Bun__registerSignalsForForwarding pattern.
Fixes #29843
- Serialise save→dlopen→restore under std::mutex so worker threads calling bun:ffi dlopen or process.dlopen concurrently can't corrupt the global sigaction snapshot (coderabbit #1). - Compare sa_mask alongside sa_handler/sa_sigaction/sa_flags when deciding whether the loaded library changed a signal's disposition — a mask-only change would have slipped through otherwise (coderabbit #2). - Drop expect(stderr).toBe("") from the two subprocess tests: debug ASAN builds emit a JSC 'useWasmFaultSignalHandler will be disabled' line on stderr whenever a run touches WASM, which was making the tests flaky on ASAN CI lanes (coderabbit #3). - Bump per-test timeout to 30s to absorb debug-build subprocess startup cost on slower CI agents.
Moved the cc compilation to beforeAll so each test body is just the
subprocess spawn, which fits the default per-test timeout with headroom
on ASAN CI lanes. Removes the explicit { timeout: 30_000 } options I
added in the previous commit — test/CLAUDE.md is clear that tests
should rely on the runner built-in timeout.
Flagged by claude[bot] on PR #29844.
- Address claude[bot]'s finding that the top-level beforeAll unconditionally spawns cc, which fails ENOENT on Windows CI even when both tests are skipIf'd. Moved everything inside describe.skipIf(!isPosix) so the hook only runs on platforms that have a C compiler. - Switched fixture tempdir from tmpdirSync to harness's tempDir and added an afterAll dispose — per test/CLAUDE.md the latter is preferred (coderabbit). - Added a third test that exercises process.dlopen() in addition to bun:ffi dlopen, so the BunProcess.cpp path is regression-covered alongside the ffi.zig one (coderabbit). The library isn't a real node addon so process.dlopen throws — but the constructor runs before the throw, which is all we need to verify signal-state restoration. - Added a comment in c-bindings.cpp acknowledging the narrow race flagged by claude[bot]: the mutex serialises save/restore against each other but not against Bun's other sigaction callers (process.on, SigintWatcher, TTY exit handler). The fix touches every sigaction caller and belongs in a follow-up.
V8-style native addons (NODE_MODULE macro) have their Init function invoked synchronously from inside dlopen() via node_module_register() in v8/node.cpp:100-103 — that's arbitrary user code that may require() another .node addon, re-entering Process_functionDlopen on the same thread. With a non-recursive std::mutex that meant a deadlock on the inner save/lock. Added a thread_local depth counter so nested calls are no-ops: only the outermost save snapshots the sigactions and only the outermost restore reverts them, leaving the mutex locked across the whole (possibly nested) dlopen tree on the calling thread. A recursive_mutex alone wouldn't suffice — the inner save would overwrite the outer snapshot and the inner restore would memset it before the outer restore runs. Flagged by claude[bot].
Moves expect(exitCode).toBe(0) after the stdout-derived assertions in the
two subprocess tests that were violating it (the sigactions test and the
new process.dlopen test). Matches the root CLAUDE.md guidance: a crashed
subprocess now surfaces the diagnostic JSON (ignLost: [13] for a SIGPIPE
regression) before the bare exit-code mismatch.
The SIGUSR1 test was already following the convention via its combined
expect({ exitCode, tail }).toEqual({...}) assertion.
Flagged by claude[bot].
5759bfd to
d0e20dc
Compare
|
Note: this PR fixes a real problem (Go clobbering SIGPIPE/crash handlers) but doesn't address the SIGPWR storm in #31158 / #29843. The SIGPWR issue is a separate mechanism: Go's Both bugs are triggered by Go dlopen, both cause hangs, but they're independent:
This PR is still valuable for the SIGPIPE protection — just noting it doesn't cover the SIGPWR case. |
|
@gogakoreli thanks for the clarification. You're right — my restore logic bails early when the handler pointer is unchanged, so a Go (Worth noting: the sa_mask comparison I added in 4289c39 covers the mask case but not the sa_flags case — my change-detection reads |
|
Correction to my previous reply — I re-read the actual committed code and my change-detection DOES check Where it falls down is the ordering: if JSC installs its SIGPWR handler for thread-suspend after my restore window closes (e.g. WASM loaded post-dlopen), or if Go's flag-drift happens outside a save/restore pair, my PR simply doesn't see it. That's the gap #31161 closes. Your table is right; my fix is scoped to the save→dlopen→restore window, not the whole process lifetime of SIGPWR handling. |
|
Closing: this PR predates the Rust rewrite and modifies source files that no longer exist on If the underlying issue is still present, it will need a fresh fix against the current tree. |
2 participants
What
Save and restore
sigaction()state around the two dlopen paths in Bun (bun:ffi dlopen()andprocess.dlopen()) so that libraries which install their own signal handlers at load time can't steal signals Bun relies on.Why
Fixes #29843. Loading a Go
-buildmode=c-sharedlibrary viabun:ffidlopen caused Prisma 7's MariaDB queries to hang the event loop. Go's c-shared init bulk-installssigactionhandlers for SIGURG, SIGPIPE, SIGCHLD, SIGHUP, SIGINT, SIGTERM, SIGABRT, SIGSEGV, SIGILL, SIGBUS, SIGFPE, SIGTRAP, SIGQUIT as part of the Go runtime startup. This overwrites:SIGPIPE = SIG_IGN(Bun's networking stack depends on writes to closed sockets returningEPIPErather than killing the process)SEGV/ILL/BUS/FPEprocess.on("SIG…")handler the user registeredReading
/proc/self/statusbefore/after a minimal dlopen confirmed it:Bit 13 (SIGPIPE) dropped out of the ignore mask — that's the specific handler whose loss breaks the MariaDB driver path.
How
Bun__saveSignalHandlersForDlopen/Bun__restoreSignalHandlersAfterDlopeninsrc/bun.js/bindings/c-bindings.cppsnapshot every interceptable signal'ssigactionbefore dlopen and compare after. For each signal whose action the loaded library changed, restore Bun's — unless the previous action wasSIG_DFL, in which case the library is free to install whatever it needs (so e.g. Go keeps its SIGURG handler for goroutine preemption).Wrapped both call sites:
FFI.openinsrc/bun.js/api/ffi.zigProcess_functionDlopeninsrc/bun.js/bindings/BunProcess.cppAfter the fix, SigIgn is identical across dlopen and the only handler the loaded library gets to keep is SIGURG (Go's preemption signal) — which is exactly what we want.
Tests
test/regression/issue/29843.test.ts:__attribute__((constructor))replicates Go's c-shared signal-install (no Go toolchain needed). Loads it viabun:ffiand verifies by reading/proc/self/statusthat no signal was lost from Bun's ignore or caught masks, and the only SigCgt additions are signals that wereSIG_DFLpre-dlopen.process.on("SIGUSR1", …)handler, dlopens the library, then self-sends SIGUSR1 — must still fire the JS handler (without the fix, the test times out).Both tests fail on main (
ignLost: [13]for SIGPIPE, and the SIGUSR1 test hangs until timeout) and pass with the fix.