Don't forward SIGPWR in spawnSync signal handling#31183
Conversation
On Linux, JSC uses SIGPWR (g_wtfConfig.sigThreadSuspendResume) to suspend and resume threads for GC and the libpas scavenger. Bun.openInEditor spawns a detached thread that calls sync::spawn, which installs a process-wide signal-forwarding handler (with SA_RESETHAND) over every signal in FOR_EACH_SIGNAL, including SIGPWR. With multiple concurrent calls, the register/unregister paths race on the shared previous_actions[] array and leave SIGPWR at SIG_DFL, so the next GC/scavenger pthread_kill terminates the process. SIGPWR is not a user-facing signal in Bun and never needs forwarding to a child; drop it from the Linux-only forwarding list. Also fix a use-after-free in Bun.openInEditor when an absolute editor path is supplied: detect_editor aliases path to name, and re-backing name_storage freed the buffer both pointed at while only repointing name.
|
Updated 8:30 AM PT - May 21st, 2026
❌ @robobun, your commit eb61137 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31183That installs a local version of the PR into your bun-31183 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoves SIGPWR from Linux signal-forwarding registration; changes open_in_editor alias handling to reassign edit.path into the process-lifetime dirname_store when name and path share the same buffer; adds a Linux-only concurrent test that runs child scripts calling Bun.openInEditor and forcing GC to verify clean exits. ChangesSIGPWR Signal Handling and Memory Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/open-in-editor-gc.test.ts`:
- Line 2: The test currently creates a tempDir and spawns a subprocess to run a
single inline script; replace that by invoking bun with the -e inline-script
flag and remove the tempDir/cwd setup and any file-write logic; update the call
site that uses bunExe and bunEnv to pass ['-e', '<script>'] (or the existing
inline script string) instead of a working directory, and remove imports/usages
of tempDir and related cwd handling (affecting the test function and any helper
calls around the bun spawn in open-in-editor-gc.test.ts lines ~13-30) so the
test runs the one-file fixture directly via -e.
🪄 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: 4d591d73-4bc0-4840-8a25-275c98e6a598
📒 Files selected for processing (3)
src/jsc/bindings/c-bindings.cppsrc/runtime/api/BunObject.rstest/js/bun/util/open-in-editor-gc.test.ts
| @@ -0,0 +1,44 @@ | |||
| import { test, expect } from "bun:test"; | |||
| import { bunEnv, bunExe, isLinux, tempDir } from "harness"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use -e instead of a temp directory for this single-file subprocess fixture.
This test writes one inline script file only to execute it immediately. Prefer passing that script via -e and drop tempDir/cwd setup.
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: For single-file tests, prefer \-e` over `tempDir``.
Also applies to: 13-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/util/open-in-editor-gc.test.ts` at line 2, The test currently
creates a tempDir and spawns a subprocess to run a single inline script; replace
that by invoking bun with the -e inline-script flag and remove the tempDir/cwd
setup and any file-write logic; update the call site that uses bunExe and bunEnv
to pass ['-e', '<script>'] (or the existing inline script string) instead of a
working directory, and remove imports/usages of tempDir and related cwd handling
(affecting the test function and any helper calls around the bun spawn in
open-in-editor-gc.test.ts lines ~13-30) so the test runs the one-file fixture
directly via -e.
There was a problem hiding this comment.
Intentional — running via -e does not reproduce the crash on release builds, only running from a file does. The module-loading path allocates more heap, which is what gets the libpas scavenger to fire SIGPWR during the idle sleep. Verified: 20/20 SIGPWR terminations from a file, 0/10 via -e with the same script.
There was a problem hiding this comment.
@robobun, understood — thanks for the detailed explanation. The file-based execution is clearly the right approach here; the extra heap pressure from the module-loading path is what makes the scavenger reliably fire SIGPWR, and -e doesn't reproduce that allocation pattern at all. I'll withdraw the suggestion.
🐇 📚 noted
✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 31183
File: test/js/bun/util/open-in-editor-gc.test.ts:2-2
Timestamp: 2026-05-21T11:02:50.349Z
Learning: In `test/js/bun/util/open-in-editor-gc.test.ts` (oven-sh/bun), the subprocess is intentionally run from a file via `[bunExe(), "run.js"]` with a `tempDir` + `cwd`, NOT via `bunExe() -e <script>`. The module-loading path allocates significantly more heap than `-e` inline execution, which is required to trigger the libpas scavenger to fire SIGPWR during the `Bun.sleep(1000)` idle window. Empirically verified: 20/20 SIGPWR terminations with file-based execution vs 0/10 with `-e`. Do not suggest replacing `tempDir`/file execution with `-e` for SIGPWR/GC scavenger regression tests of this class.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 27385
File: test/js/bun/http/tls-keepalive.test.ts:115-140
Timestamp: 2026-02-24T21:02:00.725Z
Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:21:42.189Z
Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:22:55.570Z
Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.
Learnt from: robobun
Repo: oven-sh/bun PR: 28214
File: test/regression/issue/18115.test.ts:1-158
Timestamp: 2026-03-18T15:19:38.407Z
Learning: In Bun test files, when a resource like tempDir is a DisposableString implementing both Symbol.dispose (sync) and Symbol.asyncDispose, prefer plain using over await using. Do not recommend converting to await using for tempDir in Bun test files. This keeps tests idiomatic and avoids unnecessary async disposal. If a resource only supports asyncDispose, use await using.
Learnt from: robobun
Repo: oven-sh/bun PR: 28425
File: test/regression/issue/28422.test.ts:65-79
Timestamp: 2026-03-22T10:12:05.719Z
Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
Learnt from: robobun
Repo: oven-sh/bun PR: 28923
File: test/regression/issue/28921.test.ts:0-0
Timestamp: 2026-04-06T19:19:08.790Z
Learning: In oven-sh/bun tests, prefer `tempDir` (from the `harness` module) over `tempDirWithFiles` when using the `using` statement for automatic cleanup. `tempDirWithFiles(...)` returns a plain `string`, so `using tempDirWithFiles(...)` is effectively a no-op and will not trigger disposal/cleanup. `tempDir` returns a `DisposableString` that implements `Symbol.dispose`, so it will correctly trigger cleanup on scope exit.
Learnt from: robobun
Repo: oven-sh/bun PR: 29050
File: test/regression/issue/29042.test.ts:60-94
Timestamp: 2026-04-08T21:22:00.840Z
Learning: In this repo’s Bun environment, `Bun.RedisClient` does not implement `Symbol.dispose` or `Symbol.asyncDispose`, so you cannot rely on `using` / `await using` for automatic cleanup. When creating a `Bun.RedisClient` in tests, close it explicitly with `try/finally`, calling `client.close()` in the `finally` block.
Learnt from: robobun
Repo: oven-sh/bun PR: 29322
File: test/js/web/workers/worker-terminate-after-exit.test.ts:38-43
Timestamp: 2026-04-15T01:57:52.469Z
Learning: In oven-sh/bun test files (matching `test/**/*.test.ts`), when you spawn a subprocess in a bun:test and you assert on its exit code, follow the CLAUDE.md house style: write `if (exitCode !== 0) { expect(stderr).toBe(""); }` immediately before `expect(exitCode).toBe(0)`. This is intentional so that, on failure, bun:test surfaces the full `stderr` content in the diff output. Do not replace this with a custom/second assertion that formats stderr into the exit-code expectation (e.g., `expect(exitCode, \\`stderr: ${stderr}\\`).toBe(0)` or any single-assertion equivalent).
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: test/js/bun/util/v8-heap-snapshot-large-strings.test.ts:4-152
Timestamp: 2026-04-17T02:55:14.338Z
Learning: In oven-sh/bun, do not enforce the `test/regression/issue/${issueNumber}.test.ts` placement rule based solely on PR descriptions that include a speculative GitHub issue link like “might fix `#NNNNN`” without a confirmed regression (e.g., no verifying stack trace/reproduction). If the issue is not confirmed per CLAUDE.md (“confirmed numbered issue” only), the test should be placed next to the closest related existing test file for the affected feature/module (e.g., alongside `test/js/bun/util/v8-heap-snapshot.test.ts`) and should not be flagged as a guideline violation. Likewise, tests that validate a broader behavioral invariant (e.g., V8-matching 1024-char string truncation in heap snapshots) are not purely issue regressions and should live with the feature’s existing test suite rather than under `test/regression/issue/`.
Learnt from: robobun
Repo: oven-sh/bun PR: 29426
File: test/js/node/tls/node-tls-root-certs-concurrent-init.test.ts:80-82
Timestamp: 2026-04-18T00:50:38.905Z
Learning: In oven-sh/bun Jest/Bun test files under `test/js/` that spawn subprocesses using `bunEnv` from the `harness` module, it’s safe and intentional to assert `expect(stderr).toBe("")` unconditionally. `bunEnv` sets `BUN_DEBUG_QUIET_LOGS=1`, which suppresses ASAN/debug-build stderr noise, so an unexpected stderr value should fail the test and show useful diagnostics. Do not gate `expect(stderr).toBe("")` behind `if (exitCode !== 0)` for these `bunEnv`-based subprocess tests—follow the established pattern used in similar tests (e.g., `test/js/node/tls/test-use-system-ca.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29441
File: test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts:10-14
Timestamp: 2026-04-18T10:36:45.033Z
Learning: In oven-sh/bun test files that spawn subprocesses using `bunEnv`, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty. Use the repo’s established convention: split stderr into lines and filter with `.filter(line => !line.startsWith("WARNING: ASAN interferes"))`, then assert the remaining stderr lines are empty. Do not switch to an alternative like `str.replace(...)`; the filter-based approach is the repo convention. This is safe because `ZigGlobalObject.cpp` emits that warning via `std::call_once`, so at most one matching line appears per process.
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:80-82
Timestamp: 2026-04-21T09:47:19.303Z
Learning: In Bun JavaScript/TS tests under `test/js/bun/**` that run runtime subprocesses by spawning `bunExe()` with `bunEnv`, do not add strict `expect(stderr).toBe("")` assertions. In debug ASAN builds, stderr will include `WARNING: ASAN interferes with JSC signal handlers…` on every JS-process launch and it is not suppressed by `bunEnv` / `BUN_DEBUG_QUIET_LOGS=1`. Use the regression guards that are already effective for this area: assert an exact match on `stdout` and `expect(exitCode).toBe(0)`. If you must validate stderr, follow the repo’s filter-based convention: ignore/filter out lines starting with `"WARNING: ASAN interferes"`. If stdout + exitCode provide sufficient coverage, leaving stderr unchecked is acceptable.
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:133-142
Timestamp: 2026-04-21T09:54:56.748Z
Learning: When testing `bun build` subprocesses in `test/js/bun/**/*.test.ts`, it is acceptable to assert `expect(stderr).toBe("")` (or otherwise expect no stderr noise). `bun build` is compiler-only and does not start a JS VM, so it should not emit the ASAN warning about interfering with JSC signal handlers. Only JS-executing subprocesses (e.g., `bun -e`, running built output like `bun out.js`) are expected to produce that warning, so do not treat empty-stderr assertions as brittle specifically for `bun build` in these tests.
Learnt from: robobun
Repo: oven-sh/bun PR: 29564
File: test/regression/issue/29513.test.ts:51-51
Timestamp: 2026-04-22T02:58:30.645Z
Learning: In oven-sh/bun TypeScript test files, it is acceptable to use `Bun.sleep(0)` specifically as a macrotask barrier to deterministically drain the pending microtask queue before asserting. Do NOT flag `Bun.sleep(0)` as a timing-wait violation. The “do not use setTimeout/Bun.sleep in tests” guideline is intended to prevent load-sensitive wall-clock delays (e.g., `Bun.sleep(100)` or other timing windows). Use `Bun.sleep(0)` only when you need to observe a fully settled Promise/microtask chain (e.g., after deferred resolution and multiple internal `.then()` hops) where a single `await Promise.resolve()` would not advance far enough; `Bun.sleep(0)` resumes in a later macrotask after pending microtasks complete, without relying on elapsed time.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:10.896Z
Learning: In oven-sh/bun code reviews, do not recommend adding standalone regression tests that depend on setting `BUN_JSC_validateExceptionChecks=1` to exercise JSC throw-scope/exception-scope validator paths (e.g., PropertyCallback/reify interactions like `reifyAllStaticProperties`). Per `CLAUDE.md`, tests are expected to pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. Instead, treat this class of validator coverage issue as covered by: (1) the x64-asan CI shard that enables the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that hit pre-existing throw-scope assertion failures unrelated to the change under review. If helpful, add an in-source comment pointing to the specific existing exerciser (e.g., the relevant `tsgo/bun-types` test) to document the intent without relying on the env var.
Learnt from: robobun
Repo: oven-sh/bun PR: 29656
File: test/js/bun/s3/s3-path-double-free.test.ts:49-61
Timestamp: 2026-04-23T23:39:21.333Z
Learning: In Bun test files under `test/js/bun/**/*.test.ts`, prefer `test.each()` over `describe.each()` when each parameter value results in a single `test`/`it` assertion. Using `describe.each()` to wrap a single `test` adds unnecessary nesting. Only use `describe.each()` when you need multiple `test`/`it` blocks per parameter value.
Learnt from: robobun
Repo: oven-sh/bun PR: 29820
File: test/js/node/process/process-execve.test.ts:47-52
Timestamp: 2026-04-28T11:35:58.257Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when a test uses the `tempDir` fixture and spawns a subprocess via `await using proc = Bun.spawn(...)` (i.e., the embedded script runs as a spawned subprocess), do not recommend adding a fixture-level or embedded-script `setTimeout` watchdog to prevent hangs. The `await using` scope exit should terminate the subprocess automatically, and Bun test per-test timeouts already bound execution time. Also, avoid embedded `setTimeout` watchdog patterns that violate Bun’s “no setTimeout in tests” guideline. If the worker/subprocess exits silently without posting, rely on the test’s stdout/exitCode assertions plus Bun’s outer timeout rather than a watchdog, even when the embedded fixture script uses `worker_threads` or other async constructs.
Learnt from: robobun
Repo: oven-sh/bun PR: 29874
File: test/js/web/websocket/websocket-proxy-tunnel-upgrade-leak.test.ts:15-16
Timestamp: 2026-04-28T21:34:23.491Z
Learning: In oven-sh/bun, when a test is intentionally validating native refcount leak detection using Bun debug-only instrumentation (e.g., `BUN_DEBUG_alloc=1` and `[alloc] new(...)/destroy(...)` log lines produced only by debug builds when `Environment.enable_logs` is set), use `test.skipIf(!isDebug)` as the correct/intentional guard. Do not flag this `test.skipIf(!isDebug)` as a guideline violation for this class of tests. The debug-only `[alloc] ...` lines are absent in release and ASAN builds, and there is no equivalent observable system-Bun hook to assert a leak when only debug-build instrumentation exists (so the `USE_SYSTEM_BUN=1` rule in `CLAUDE.md` does not apply in this situation).
Learnt from: robobun
Repo: oven-sh/bun PR: 29876
File: test/js/bun/ffi/cc.test.ts:0-0
Timestamp: 2026-04-29T00:09:18.937Z
Learning: In oven-sh/bun tests, when using the `harness` module’s `tempDir`, prefer the overload that accepts an optional second argument: `tempDir(prefix, fileTree)`, where `fileTree` is an object in the same shape as `tempDirWithFiles` (e.g., `{ "file.c": "..." }`). This creates a disposable temp directory pre-populated with files. If the `tempDir` file-tree overload is available, don’t recommend a separate manual `fs.writeFile`/write step for pre-populating files (e.g., when using `using dir = tempDir("prefix", { ... })`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29876
File: test/js/bun/ffi/cc.test.ts:205-231
Timestamp: 2026-04-29T00:24:38.784Z
Learning: In oven-sh/bun’s Bun test files under test/js/bun/, do not treat explicit per-test timeouts as a guideline violation when the test is an RSS-leak regression that spawns a subprocess and performs many iterations (subprocess-heavy leak tests). For these cases, Bun’s default per-test timeout (5s locally) is insufficient—especially under debug+ASAN where these tests may take ~5–14s—so reviewers should expect and accept an explicit, larger per-test timeout (e.g., 60_000). Concretely, tests like the cc() option-string leak test (test/js/bun/ffi/cc.test.ts) and glob-leak tests (e.g., test/js/bun/glob/leak.test.ts) should be reviewed as exceptions: allow explicit timeouts when the intent is to cover RSS-leak/subprocess-heavy regression workloads.
Learnt from: robobun
Repo: oven-sh/bun PR: 29919
File: test/js/bun/util/filesystem_router.test.ts:613-628
Timestamp: 2026-05-02T00:35:55.819Z
Learning: In oven-sh/bun tests under test/js/bun/**, prefer strict stderr assertions like `expect(stderr).toBe("")` for subprocesses spawned with `bunExe()` when you pass a `bunEnv` that already propagates `ASAN_OPTIONS=allow_user_segv_handler=1` from the parent `bun bd` build environment (this suppresses the `WARNING: ASAN interferes with JSC signal handlers` message). On CI ASAN lanes where `isASAN` is true, `bunEnv` sets `isASAN` explicitly as well—so strict stderr expectations are still safe. Only relax/skip strict stderr assertions (e.g., avoid `toBe("")`) when `ASAN_OPTIONS=allow_user_segv_handler=1` is *not* propagated into the subprocess environment.
Learnt from: robobun
Repo: oven-sh/bun PR: 30115
File: test/js/bun/glob/scan.test.ts:877-882
Timestamp: 2026-05-02T17:49:10.214Z
Learning: In oven-sh/bun regression tests for UAFs tied to Bun’s threadpool/event-loop interaction (e.g., WalkTask pending activity), keep the intended repro timing: use `Bun.sleepSync(N)` inside a spawned subprocess to hold the JS event loop without yielding/draining pending tasks, then trigger `Bun.gc(true)` (after the threadpool task has been given time to complete `run()`), and finally drive the result with the corresponding `for await`/iterator consumption to make the UAF observable. Do not replace `Bun.sleepSync(N)` with `await Bun.sleep(0)` or any other event-loop-yielding construct, since it can drain pending concurrent tasks and cause callbacks/`then()` work to run before the GC call, making the bug unobservable. This “sleepSync → gc(true) → for await” sequence is the correct 3-step UAF repro pattern for this bug class.
Learnt from: robobun
Repo: oven-sh/bun PR: 30142
File: test/js/bun/http/bun-serve-html-abort-leak-fixture.ts:28-38
Timestamp: 2026-05-03T01:29:10.031Z
Learning: In oven-sh/bun tests/fixtures that spawn subprocesses with `BUN_DEBUG_alloc` (or `BUN_DEBUG_ALL`) set to a non-zero value (e.g., `"1"`), the `[alloc]` log scope is effectively enabled at runtime for all `bun.new`/`bun.destroy`-allocated types. Because the runtime check in `src/output.zig` forces `really_disable = false` when `BUN_DEBUG_<tagname>` is not `"0"`, such fixtures may emit `[alloc] new(T)` / `[alloc] destroy(T)` lines even when `T` does not declare `log_allocations = true`. In this context, do not flag missing `log_allocations` declarations as a bug in the test fixture or the involved fixture types.
Learnt from: robobun
Repo: oven-sh/bun PR: 30153
File: test/bundler/plugin-sync-exception-fallback.test.ts:75-91
Timestamp: 2026-05-03T01:53:50.441Z
Learning: In this repo’s Bun test files that use `Bun.spawn`, don’t “parse/assert stdout before checking `exitCode`” when the expected failure mode is a crash (e.g., SIGSEGV or UBSan abort) that may produce empty stdout. Parsing/validating empty stdout first can mask the more useful signal/stderr. Instead, assert the spawned-process result by including `stdout` in the object passed to `toMatchObject` alongside `exitCode`, `signalCode`, and `stderr`, so stdout/stderr/signal all appear together in the failure diff (same pattern as `test/bundler/plugin-error-nested-throw.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 30230
File: test/js/bun/util/bun-file-fd-read.test.ts:109-117
Timestamp: 2026-05-04T06:28:25.345Z
Learning: When reviewing oven-sh/bun JS tests that intentionally exercise Bun’s internal IO-thread behavior (e.g., the `io.zig` epoll registration path used by `ReadFile`/`WriteFile`), it’s acceptable to use `await Bun.sleep(N)` (like 100ms) as a timing window *only* when there is no JS-observable side effect that can be awaited directly (i.e., the relevant internal event has no JS-side condition to wait on). The test should still be considered correct if its semantics are validated across race outcomes (e.g., EPOLLERR/alternate recv path still preserves fail-before/pass-after behavior). Do not flag these `Bun.sleep(N)` calls as flaky/fragile when the author has empirically verified the behavior (multiple iterations) and also confirmed it reliably with `USE_SYSTEM_BUN=1` that the intended target path is exercised and the test fails correctly without the fix.
Learnt from: robobun
Repo: oven-sh/bun PR: 30245
File: test/regression/issue/19650.test.ts:9-30
Timestamp: 2026-05-04T20:27:55.527Z
Learning: In oven-sh/bun test files, prefer using flat `test.concurrent.each([...])` when you want every parameterized test case to run fully concurrently across the entire parameter matrix. By contrast, `describe.each([...])` executes its describe blocks sequentially; while tests inside each describe block may be `test.concurrent`, concurrency is limited to within that block rather than across the whole matrix.
Learnt from: robobun
Repo: oven-sh/bun PR: 30118
File: test/js/node/zlib/zlib-writestate-detached.test.ts:78-90
Timestamp: 2026-05-04T20:37:57.348Z
Learning: In this Bun repository, do not flag code in Bun subprocess fixtures/tests where `console.log(...)` (or similar synchronous stdout/stderr writes) is immediately followed by `process.exit(n)` as a potential output-loss problem. Bun’s `process.exit()` flushes stdout and stderr synchronously before exiting (per the implementation in `src/runtime/node/process/exit.zig`), so `console.log` + `process.exit` is considered a safe, established Bun convention.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.796Z
Learning: When reviewing JS/TS regex literals in Bun test files under `test/js/bun/`, don’t flag `\\` or `\b` as “bad escaping” if they’re intentionally matching literal backslashes used in Windows named-pipe paths (e.g., `\\.\pipe\name`). In JS regex literals, `\\` represents two literal backslashes, `\.` matches a literal dot, and `\b` (backslash-backslash-b) means a literal backslash followed by `b`, not the `\b` word-boundary escape.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.255Z
Learning: When reviewing JavaScript/TypeScript regex literals, treat `\b` as an escaped backslash followed by `b` (i.e., it matches a literal backslash and then `b`), not the regex word-boundary metacharacter. The word-boundary metacharacter is an unescaped `\b` in the source code (i.e., `\b` in the pattern string/literal syntax), which has word-boundary semantics.
So: do not flag `\b` inside a regex as a word-boundary issue by default. Only flag `\b` when the intent is to match a literal backslash+`b` and word-boundary semantics would be incorrect. Example: `/^\\\.\\pipe\\/` (as written) matches the Windows named-pipe prefix `\\.\pipe\`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:467-495
Timestamp: 2026-05-05T15:02:03.877Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when verifying that a test was NOT executed (for example, it was filtered out by `pathIgnorePatterns`), assert the absence of the test name string (e.g., `expect(stderr).not.toContain("explicit test")`) rather than asserting that the filename is absent. Bun may echo the filename in its `"The following filters did not match any test files:"` error output even when no tests ran, so filename-based assertions can be misleading.
Learnt from: robobun
Repo: oven-sh/bun PR: 30306
File: test/js/web/fetch/blob-write.test.ts:88-96
Timestamp: 2026-05-06T01:36:05.893Z
Learning: TempDir must be invoked with two arguments in test harness code: basename: string and filesOrAbsolutePathToCopyFolderFrom: DirectoryTree | string. Calls like tempDir("foo") should be flagged as invalid. tempDirWithFiles("name", {}) is a permitted pattern in existing tests (e.g., test/js/web/fetch/blob-write.test.ts line 55) when the result is assigned with const (not using) and consistent with the file's conventions. Apply this rule to test files across the repository (oven-sh/bun), and do not flag compliant const-based patterns that follow the established usage.
Learnt from: robobun
Repo: oven-sh/bun PR: 30350
File: test/cli/test/bun-test.test.ts:1319-1324
Timestamp: 2026-05-07T06:52:44.159Z
Learning: In oven-sh/bun TypeScript test files under `test/**/*.test.ts`, when the test constructs the snapshot input by intentionally `.filter()`-ing raw stderr to only the reporter-generated status/output lines (e.g., lines matching `/^\((pass|fail|skip|todo)\)/`, `^ ...` explanation lines, and `AssertionError:` lines), do not require `normalizeBunSnapshot` for that snapshot. In this design, the `.filter()` is what stabilizes the snapshot across `Execution.Result` variants; adding `normalizeBunSnapshot` would unnecessarily retain extra output (stack traces, repeated failures block, summaries), making snapshots ~3x larger and more fragile. Accept the local convention of small ad-hoc `.replace()` regex normalization for volatile timing fragments (e.g., stripping `[{d}ms]` and `after {d}ms` timeout text) where applied consistently within the same test suite.
Learnt from: jgoyvaerts
Repo: oven-sh/bun PR: 30410
File: test/js/bun/http/bun-serve-routes.test.ts:721-745
Timestamp: 2026-05-08T20:24:48.518Z
Learning: For this repo’s Bun/CLI tests under `test/js/bun/**`, follow the rule from `CLAUDE.md`: do not add explicit per-test timeouts (e.g., the 3rd argument to `test()`), including in performance/timing or scaling regression tests. Bun already applies its own timeouts, and adding per-test timeouts will likely interfere with the intended measurement. Only suggest adding explicit timeouts if the target file already uses them and they are explicitly required for correctness. The known exceptions are `test/js/bun/ffi/cc.test.ts` and `test/js/bun/glob/leak.test.ts` (RSS-leak, subprocess-heavy tests where timeouts may be necessary).
Learnt from: robobun
Repo: oven-sh/bun PR: 30414
File: test/js/bun/util/throw-bad-toPrimitive.test.ts:17-17
Timestamp: 2026-05-09T01:26:42.041Z
Learning: In oven-sh/bun test files under test/js/bun/**, enforce `bunExe()` + `-e` only for short inline one-liners (where the subprocess entry point is a single-string expression). If the subprocess entry-point is a fixture file (i.e., the entry point requires module-level `import` declarations and/or references `import.meta.dir`), use the established fixture pattern instead: `[bunExe(), path.join(import.meta.dir, "fixture.ts")]`. Do not flag this fixture pattern as a guideline violation (it matches existing usage across the test suite).
Learnt from: majiayu000
Repo: oven-sh/bun PR: 25687
File: test/bundler/issue-25675.test.ts:1-4
Timestamp: 2026-05-16T17:15:07.036Z
Learning: For Bun bundler tests, if a test file imports or uses `itBundled` / `expectBundled`, it must live under `./test/bundler/` (e.g., `test/bundler/**`). These helpers include a runtime guard that checks the call stack for `test/bundler/` and will throw with “All bundler tests must be placed in ./test/bundler/…”. Do not suggest moving such tests to `test/regression/…`, even for issue-specific/regression cases, because they will fail at runtime.
Learnt from: robobun
Repo: oven-sh/bun PR: 30936
File: test/bundler/transpiler/runtime-transpiler.test.ts:225-225
Timestamp: 2026-05-17T19:03:05.577Z
Learning: This repo (oven-sh/bun) does not enforce Biome lint rules in CI because there is no root Biome config (`biome.json` or `.biome*`). Therefore, during code review do not suggest adding `// biome-ignore` (or similar) suppression comments for Biome rule violations.
Additionally, in test files under `test/bundler/transpiler/`, do not “fix” switch-case code by wrapping intentionally-bare (unwrapped) `const` declarations in `{}` blocks when the test is specifically asserting TDZ/const-inlining behavior across sibling cases (e.g., regression tests like issue `#30932`). Adding a `{}` block can interfere with the const-prefix inliner and the single-use substitution pass, causing the test to miss the intended failure mode.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:343-375
Timestamp: 2026-05-21T07:56:03.036Z
Learning: In oven-sh/bun test files (Bun test), both `test.each` and `describe.each` are acceptable idioms for parameterized tests. Do not treat `test.each` as a guideline violation in favor of `describe.each`. Use `test.each` when each parameter entry corresponds to a single test body and no nested `test()` blocks are needed; use `describe.each` when you want grouped/structured test suites per parameter set.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Overlaps with #30956 which has the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #define FOR_EACH_LINUX_ONLY_SIGNAL(M) \ | ||
| M(SIGPOLL); \ | ||
| M(SIGPWR); \ | ||
| M(SIGSTKFLT); |
There was a problem hiding this comment.
🟣 Pre-existing nit, since you're already curating this list: SIGPOLL is an alias of SIGIO (signal 29) on Linux, and SIGIOT is an alias of SIGABRT (signal 6) on all POSIX targets — so Bun__registerSignalsForForwarding() calls sigaction() twice on the same number, the second call overwrites previous_actions[N] with the forwarding handler we just installed, and Bun__unregisterSignalsForForwarding() then "restores" the forwarding lambda instead of the original. Practical impact is low (Bun has no custom SIGABRT/SIGIO handler, and glibc abort() re-raises after the handler returns), but dropping the redundant SIGPOLL here and SIGIOT from FOR_EACH_POSIX_SIGNAL would make the save/restore actually round-trip.
Extended reasoning...
What the bug is
On Linux, SIGPOLL and SIGIO are the same signal number (29 on x86_64/aarch64 — asm-generic/signal.h literally has #define SIGPOLL SIGIO), and SIGIOT and SIGABRT are the same signal number (6 — #define SIGIOT SIGABRT). FOR_EACH_POSIX_SIGNAL lists SIGABRT, SIGIOT, and SIGIO, and FOR_EACH_LINUX_ONLY_SIGNAL additionally lists SIGPOLL. So on Linux, FOR_EACH_SIGNAL expands 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 SIGABRT then SIGIOT.
Why nothing else fixes it up
After Bun__unregisterSignalsForForwarding(), the SignalForwarding Drop in src/spawn/process.rs calls crash_handler::reset_on_posix(), but that only re-installs handlers for SIGSEGV/SIGILL/SIGBUS/SIGFPE (src/crash_handler/lib.rs:1717-1720) — it does not touch SIGABRT or SIGIO. So after the first spawnSync/openInEditor cycle, signals 6 and 29 are left permanently pointing at the forwarding lambda with SA_RESETHAND.
Impact
After one spawnSync completes, Bun__currentSyncPID == 0, so when a SIGABRT or SIGIO arrives the leaked forwarding lambda just stashes it in Bun__pendingSignalToSend and returns — the first delivery is swallowed, and SA_RESETHAND then drops the disposition to SIG_DFL for the second delivery.
In practice this is low impact:
- Bun does not install its own SIGABRT or SIGIO handler at startup (
bun_initialize_processonly handles SIGINT/SIGTERM), so the "lost original" is typically justSIG_DFL. - glibc
abort()re-raises SIGABRT in a loop after a handler returns, soabort()still terminates the process. - An external
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); from FOR_EACH_LINUX_ONLY_SIGNAL and M(SIGIOT); from FOR_EACH_POSIX_SIGNAL. Their canonical names (SIGIO, SIGABRT) are already in the list, so coverage is unchanged and previous_actions[] round-trips correctly. The npm list this was copied from contains both because Node's process.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 SIGPOLL lives, so it's a natural "while you're here".
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
🟣 Note (pre-existing, non-blocking): the underlying race on previous_actions[] described in the PR body is still present — Bun.openInEditor calls Bun__registerSignalsForForwarding/Bun__unregisterSignalsForForwarding from detached std::threads with no synchronization, contradicting the "We only ever use bun.spawnSync on the main thread" comment at line ~912. Concurrent calls can still leave SIGINT/SIGTERM at the forwarding lambda or SIG_DFL instead of onExitSignal, so TTY restoration on Ctrl-C is lost. This PR strictly improves things by removing the process-death case; a follow-up adding a mutex/refcount around register/unregister (or skipping signal forwarding on the openInEditor detached-thread path) would close the rest.
Extended reasoning...
What the bug is
The PR description correctly identifies the root cause of the Fuzzilli crash: "With multiple concurrent openInEditor calls, the register/unregister paths race on the shared previous_actions[] array." The fix, however, only removes SIGPWR from FOR_EACH_LINUX_ONLY_SIGNAL. That eliminates the catastrophic symptom (process terminated by SIGPWR on the next GC), but the data race on previous_actions[NSIG] and Bun__currentSyncPID remains for every other signal in FOR_EACH_SIGNAL (SIGINT, SIGTERM, SIGHUP, SIGQUIT, …).
Code path
Bun.openInEditor → Editor::open() (src/runtime/cli/open.rs:309) spawns a detached std::thread per call → auto_close() (open.rs:449) → sync::spawn() → SignalForwarding::register() (src/spawn/process.rs:3139) → Bun__registerSignalsForForwarding() / Bun__unregisterSignalsForForwarding(). These two C++ functions read and write the process-global static struct sigaction previous_actions[NSIG] with no mutex, and Bun__unregisterSignalsForForwarding() ends with memset(previous_actions, 0, sizeof(previous_actions)).
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 Bun.openInEditor violates it.
Why nothing prevents it
There is no lock, no refcount, and no atomic guard around previous_actions[] or Bun__currentSyncPID. crash_handler::reset_on_posix() only re-installs handlers for SIGSEGV/SIGILL/SIGBUS/SIGFPE — none of which are in the forwarding list — so it does not repair the damage. onExitSignal for SIGINT/SIGTERM is only installed once, in bun_initialize_process(); if it is overwritten and not restored, it is gone for the rest of the process lifetime.
Step-by-step proof (concrete interleaving)
Take the test's own repro: 8 concurrent Bun.openInEditor("0.3", { editor: "/usr/bin/sleep" }) calls. Consider just two threads A and B and signal SIGINT:
- A: register() —
sigaction(SIGINT, &forward, &previous_actions[SIGINT]). Saves the realonExitSignalintoprevious_actions[SIGINT]. SIGINT now → forwarding lambda. - B: register() —
sigaction(SIGINT, &forward, &previous_actions[SIGINT]). The current handler is A's forwarding lambda, soprevious_actions[SIGINT]is overwritten with the forwarding lambda.onExitSignalis now lost. - A: unregister() —
sigaction(SIGINT, &previous_actions[SIGINT], NULL)restores… the forwarding lambda (saved by B). Thenmemset(previous_actions, 0, …)zeroes the whole array. - B: unregister() —
sigaction(SIGINT, &previous_actions[SIGINT], NULL)whereprevious_actions[SIGINT]is now all-zeros, i.e.sa_handler = SIG_DFL. SIGINT is now permanentlySIG_DFL.
After both threads exit, SIGINT/SIGTERM no longer point at onExitSignal. The next Ctrl-C kills the process without running bun_restore_stdio(), leaving the user's terminal in whatever termios state Bun had set (raw mode, etc.). Additionally, Bun__currentSyncPID is a single non-atomic global stomped by both threads, so while both children are alive a delivered signal may be forwarded to the wrong PID.
Impact
Far 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 Bun.openInEditor more than once concurrently," which is a niche path. This is why all verifiers agreed this is pre-existing / nit and should not block the PR: the change here is strictly an improvement, and the SIGPWR removal is independently correct even without the race (a single non-racy call would still override JSC's SIGPWR handler for the duration of the spawn).
How to fix (follow-up)
Either of:
- Add a mutex + nesting refcount around
Bun__registerSignalsForForwarding/Bun__unregisterSignalsForForwardingso only the first register savesprevious_actions[]and only the last unregister restores it (and drop the unconditionalmemset); or - Skip signal forwarding entirely on the
Bun.openInEditordetached-thread path — it's a fire-and-forget editor launch where forwarding Ctrl-C to the child isn't needed.
Also worth updating or removing the stale "main thread only" comment at line ~912, since it documents an invariant that no longer holds.
|
CI failures are unrelated flakes:
None of these touch |
The previous fix repointed edit.path at slot.name_storage, but Editor::open hands that slice to the detached editor thread by raw (ptr, len) without copying, and a subsequent openInEditor call with a different absolute editor drops the old name_storage before the thread reads it. Store the path in dirname_store (process-lifetime arena) like every other detect_editor branch. Also alternate editor paths in the regression test so it exercises the name_storage-replacement branch.
There was a problem hiding this comment.
Thanks — the dirname_store switch and the alternating-editor test cover the cross-call UAF I flagged; LGTM.
Extended reasoning...
Overview
This PR makes two narrow changes plus a regression test:
src/jsc/bindings/c-bindings.cpp: dropsSIGPWRfromFOR_EACH_LINUX_ONLY_SIGNALsoBun__registerSignalsForForwarding()never overrides JSC's GC thread-suspend signal. One macro line removed, four-line explanatory comment added.src/runtime/api/BunObject.rs: in theedit.name.as_ptr() == edit.path.as_ptr()branch ofopen_in_editor,edit.pathis now copied intoFs::FileSystem::instance().dirname_store(process-lifetime arena) instead of into the thread-localslot.name_storageVec. This is the exact fix I suggested in my earlier 🔴 comment and matches every otherdetect_editorbranch (open.rs:580/591/611/622/639).test/js/bun/util/open-in-editor-gc.test.ts: new Linux-only test that runs 5 concurrent subprocesses, each callingBun.openInEditor8× with alternating absolute editor paths (sleep + a symlink), then sleeps and forces GC, asserting clean exit with no signal.
Security risks
None introduced. The change strictly reduces the set of signals whose handlers spawnSync overrides — removing SIGPWR cannot grant new capability, it only stops Bun from clobbering JSC's internal handler. The Rust change moves a borrowed slice from a freeable thread-local Vec to a never-freed process-lifetime arena, eliminating a UAF rather than adding one. No auth, crypto, permissions, or input-validation surface is touched.
Level of scrutiny
Process-wide signal handling and Rust lifetime management both warrant careful review, and they got it: I left two 🟣 pre-existing nits (SIGPOLL/SIGIOT aliasing, and the residual previous_actions[] race) plus one 🔴 on the original UAF fix being incomplete. The author responded to all three — the 🟣 items are explicitly scoped to PR #30956 (which adds the mutex/refcount), and the 🔴 was fixed in the latest revision exactly as recommended. The current diff is now small, follows established patterns in the same module, and is strictly an improvement over main on every axis.
Other factors
- The new test was updated per my feedback to alternate between two absolute editor paths, so the
!eql_long/name_storage-replacement path is actually exercised (it wasn't before). - CI failures on build 56635 were investigated by the author and shown to be pre-existing flakes (
node-http-backpressure,v8-heap-snapshot) unrelated to this change; the new test passed on all platforms. - The acknowledged overlap with #30956 is a coordination question (the author offered to rebase the UAF fix onto it), but this PR is independently correct and landable — the SIGPWR removal and UAF fix don't conflict with the mutex/refcount work there.
On Alpine, /bin/sleep is a BusyBox multi-call binary that selects the applet from argv[0]; naming the symlink 'sleep2' fails with 'applet not found'.
There was a problem hiding this comment.
The dirname_store change resolves the cross-call UAF I flagged — edit.path now has process-lifetime storage matching the other detect_editor branches, and the test exercises the replacement path. Deferring to a human for sign-off on the process-wide signal-handling change and the coordination with #30956.
Extended reasoning...
Overview
Three changes: (1) remove SIGPWR from FOR_EACH_LINUX_ONLY_SIGNAL in src/jsc/bindings/c-bindings.cpp so spawnSync's signal-forwarding setup no longer overrides JSC's GC thread-suspend signal; (2) in src/runtime/api/BunObject.rs::open_in_editor, when detect_editor aliases path to name (absolute editor path), repoint edit.path into FileSystem::instance().dirname_store so the buffer outlives the detached editor thread; (3) a new Linux-only regression test that spawns 5 concurrent subprocesses each calling Bun.openInEditor 8× with two alternating absolute editor paths, sleeps, GCs, and asserts a clean exit.
The author addressed my earlier 🔴 comment exactly as suggested — edit.path now uses the same process-lifetime arena (dirname_store.append_slice(...).expect("unreachable")) that every other detect_editor branch already uses (open.rs:580/591/611/622/639), and the test now alternates editor paths so the !eql_long / name_storage-replacement branch is actually exercised. The two non-blocking nits I left (SIGPOLL/SIGIOT alias double-registration, and the remaining previous_actions[] race for other signals) are intentionally deferred to #30956 per the author's reply.
Security risks
None in the auth/crypto/permissions sense. The change is a stability fix: it removes a signal from a forwarding list and moves a string into longer-lived storage. There is no new attack surface, no user-controlled input reaching new code paths, and no change to privilege or capability boundaries.
Level of scrutiny
Higher than average. Bun__registerSignalsForForwarding / Bun__unregisterSignalsForForwarding mutate process-wide sigaction state that interacts directly with JSC's GC suspend/resume mechanism — getting this wrong terminates the process. The UAF fix involves reasoning about a raw (ptr, len) handed across a detached std::thread boundary. Both are well-explained and the diff is small, but neither is "mechanical".
Other factors
- Feedback loop closed: my blocking comment on the previous revision is resolved by af8feba; eb61137 fixed the Alpine/BusyBox test failure by keeping the symlink basename as
sleepso the multi-call applet resolves. - CI: the robobun status on af8feba shows the new test failing on Alpine 3.23 (fixed by eb61137), plus unrelated pre-existing items (
node-http-backpressure.test.tsSIGKILL flake; macOS build-rust warnings inprocess.rs/crash_handler/lib.rswhich are not touched by this PR). I have not seen a CI run for the tip commit eb61137. - Overlap with #30956: that PR carries the same SIGPWR exclusion plus a mutex/refcount for the remaining race and the SIGPOLL/SIGIOT alias cleanup. Whether to land this first (for the UAF fix it uniquely carries) or fold into #30956 is a coordination call for a human.
- No new bugs were found by the bug-hunting system on this revision.
2 participants
On Linux, JSC uses
SIGPWR(viag_wtfConfig.sigThreadSuspendResume) to suspend and resume threads for GC and the libpas scavenger.Bun.openInEditorspawns a detachedstd::threadthat callssync::spawn, which installs a process-wide signal-forwarding handler (withSA_RESETHAND) over every signal inFOR_EACH_SIGNAL— includingSIGPWR. With multiple concurrentopenInEditorcalls, the register/unregister paths race on the sharedprevious_actions[]array and leaveSIGPWRatSIG_DFL, so the nextpthread_kill(thread, SIGPWR)from GC or the scavenger terminates the process with "Power failure".SIGPWRis not a user-facing signal in Bun and never needs forwarding to a child — drop it from the Linux-only forwarding list.Also fixes a use-after-free in
Bun.openInEditor({ editor: "/absolute/path" }):detect_editoraliasespathtoname, and re-backingname_storagefreed the buffer both pointed at while only repointingname. The stalepathwas then handed to the spawned editor thread as argv[0].Found by Fuzzilli (fingerprint
4e72ca85863f92fa). Repro: