Fix crash in Bun.file().writer() with invalid path/fd options - #30277
Closed
robobun wants to merge 1 commit into
Closed
Fix crash in Bun.file().writer() with invalid path/fd options#30277robobun wants to merge 1 commit into
Bun.file().writer() with invalid path/fd options#30277robobun wants to merge 1 commit into
Conversation
When the options object passed to .writer() contains a non-string path or a non-integer fd, streams.Start.fromJSWithTag returns an .err variant. The caller unconditionally accessed the .FileSink field afterward, triggering a union-tag safety panic in debug builds and undefined behavior in release builds. Handle the .err case by throwing a proper JS error, and deinit the allocated path from fromJSWithTag before overwriting it with the blob's own path.
Collaborator
Author
Contributor
WalkthroughThe PR improves error handling in ChangesWriter Error Handling & Validation
🚥 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}
Contributor
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/filesink.test.ts`:
- Around line 210-215: Replace the tmpdirSync() call in the test block that
verifies file.writer() error handling with the harness-provided tempDir() so the
temporary directory is created and cleaned deterministically; locate the test
named "writer() throws on invalid options instead of crashing" (uses isWindows
and calls Bun.file(...).writer(...)) and change the path construction from
path.join(tmpdirSync(), "test.txt") to use tempDir() (ensuring tempDir is
available in the test scope per other tests) so it follows the test suite
convention.
🪄 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: 31f998ff-5cee-4de2-a54e-fcaf605be7a3
📒 Files selected for processing (2)
src/runtime/webcore/Blob.zigtest/js/bun/util/filesink.test.ts
Comment on lines
+210
to
+215
| it.skipIf(isWindows)("writer() throws on invalid options instead of crashing", () => { | ||
| const file = Bun.file(path.join(tmpdirSync(), "test.txt")); | ||
| expect(() => file.writer({ path: 123 })).toThrow(expect.objectContaining({ code: "EINVAL" })); | ||
| expect(() => file.writer({ fd: "not a number" })).toThrow(expect.objectContaining({ code: "EBADF" })); | ||
| expect(() => file.writer({ fd: -9999.5 })).toThrow(expect.objectContaining({ code: "EBADF" })); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use tempDir for the new test instead of tmpdirSync.
Line 211 adds another tmpdirSync() call in a .test.ts file. Please use tempDir here so cleanup is deterministic and consistent with test conventions.
Proposed change
it.skipIf(isWindows)("writer() throws on invalid options instead of crashing", () => {
- const file = Bun.file(path.join(tmpdirSync(), "test.txt"));
+ using dir = tempDir("filesink-invalid-options");
+ const file = Bun.file(path.join(dir, "test.txt"));
expect(() => file.writer({ path: 123 })).toThrow(expect.objectContaining({ code: "EINVAL" }));
expect(() => file.writer({ fd: "not a number" })).toThrow(expect.objectContaining({ code: "EBADF" }));
expect(() => file.writer({ fd: -9999.5 })).toThrow(expect.objectContaining({ code: "EBADF" }));
});-import { fileDescriptorLeakChecker, isPosix, isWindows, tmpdirSync } from "harness";
+import { fileDescriptorLeakChecker, isPosix, isWindows, tempDir, tmpdirSync } from "harness";As per coding guidelines: test/**/*.test.ts: “Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync.”
🤖 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/filesink.test.ts` around lines 210 - 215, Replace the
tmpdirSync() call in the test block that verifies file.writer() error handling
with the harness-provided tempDir() so the temporary directory is created and
cleaned deterministically; locate the test named "writer() throws on invalid
options instead of crashing" (uses isWindows and calls
Bun.file(...).writer(...)) and change the path construction from
path.join(tmpdirSync(), "test.txt") to use tempDir() (ensuring tempDir is
available in the test scope per other tests) so it follows the test suite
convention.
Collaborator
Author
There was a problem hiding this comment.
This test matches the existing convention in this file — every other test here uses tmpdirSync() (lines 47, 58, 199). More importantly, this test never creates any files: .writer() throws on the invalid options before any I/O happens, so there's nothing to clean up. (Also, the proposed diff is missing tempDir's required second argument.)
Contributor
There was a problem hiding this comment.
@robobun, you're right on all three points — the file-level convention is tmpdirSync, no files are ever created so there's nothing to clean up, and my proposed diff was indeed missing the second argument to tempDir. I'll withdraw the suggestion. Thanks for the clear explanation.
✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 30277
File: test/js/bun/util/filesink.test.ts:210-215
Timestamp: 2026-05-05T10:39:19.660Z
Learning: In `test/js/bun/util/filesink.test.ts`, the established convention is to use `tmpdirSync()` (from `harness`) for temporary directories, not `tempDir`. Do not flag `tmpdirSync()` usage in this file as a guideline violation. Additionally, when a test calls `.writer()` solely to assert that it throws on invalid options (before any I/O occurs), no temporary directory cleanup is needed, making `tempDir` unnecessary.
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\`.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — small, targeted fix that replaces a union-tag panic with a proper JS error, plus tests.
Extended reasoning...
Overview
This PR touches src/runtime/webcore/Blob.zig (the getWriter function backing Bun.file().writer()) and adds a regression test in test/js/bun/util/filesink.test.ts. The change replaces an unconditional stream_start.FileSink.input_path = ... with a switch on the union tag, so that when streams.Start.fromJSWithTag returns .err (invalid path/fd option types) the function throws a JS error instead of triggering a Zig union-tag safety panic / release-mode UB. It also deinits the slice that fromJSWithTag allocated before overwriting it with the blob's own path, fixing a small leak that already existed on the happy path.
Security risks
None. This is pure error-path hardening on a JS-facing API; the only behavioral change is converting a crash into a thrown EINVAL/EBADF error. No new file/fd access, no auth or crypto.
Level of scrutiny
Low. The diff is ~12 lines of straightforward tag-switching that mirrors the existing .err handling patterns immediately above (lines 2934-2948) and below (lines 2993-2999) in the same function — sink.deref() then globalThis.throwValue(...). The .FileSink and else arms preserve the previous semantics for the non-error case. The added test exercises all three invalid-option shapes the fuzzer hit.
Other factors
The bug hunter flagged a pre-existing nit: the try on fromJSWithTag itself can leak sink if a JS getter throws. That try was already present before this PR and is only reachable via adversarial throwing getters, so it's a reasonable follow-up rather than a blocker. The core crash fix is correct and well-tested.
| @@ -2975,7 +2975,19 @@ pub fn getWriter( | |||
|
|
|||
| if (arguments.len > 0 and arguments.ptr[0].isObject()) { | |||
| stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink); | |||
Contributor
There was a problem hiding this comment.
🟡 Nit: the try on this line can also early-return (e.g. { get highWaterMark() { throw 1 } }), in which case sink from line 2954 is leaked — there's no errdefer sink.deref(). Since this PR is already adding sink.deref() for the .err branch right below, an errdefer sink.deref(); after line 2954 would cover this path (and the existing manual sink.deref() calls could then be dropped). Pre-existing, but it's the same hardening this PR is doing.
Extended reasoning...
What the bug is
FileSink.init (FileSink.zig:553) heap-allocates the sink via bun.new with a ref count and increments live_count; it must be deref()'d to be freed. At Blob.zig:2954 the sink is created, but there is no errdefer sink.deref() afterward. The very next failure point that uses try is line 2977:
stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink);fromJSWithTag (streams.zig:74) returns bun.JSError!Start and internally does try value.fastGet(globalThis, .highWaterMark), try value.fastGet(globalThis, .path), try value.getTruthy(globalThis, "fd"), and try path.toSlice(...). Any of these propagate error.JSError if a JS getter throws or toSlice fails. When that happens, try returns from getWriter without ever calling sink.deref(), leaking the FileSink.
Step-by-step proof
Bun.file('/tmp/x').writer({ get highWaterMark() { throw 1 } })- Blob.zig:2954 —
sink = FileSink.init(...)heap-allocates a FileSink,live_count+= 1. - Blob.zig:2976 —
arguments[0].isObject()is true. - Blob.zig:2977 —
fromJSWithTagcallsvalue.fastGet(globalThis, .highWaterMark), which invokes the throwing getter → returnserror.JSError. trypropagateserror.JSErrorout ofgetWriter. Noerrdeferruns forsink. The onlydeferisinput_path.deinit()(line 2968).sinkis never deref'd → leaked.
Why existing code doesn't prevent it
The PR adds sink.deref() for the .err union variant returned by fromJSWithTag (line 2980), and there's an existing sink.deref() in the sink.start error branch (line 2995). But neither covers the Zig-error (error.JSError) path on line 2977, which bypasses the switch entirely via try.
Impact
Small heap leak per call, only triggerable with adversarial throwing getters on the options object. Not a correctness or crash issue. However, this PR is specifically hardening invalid-options handling in this exact block and explicitly adds sink.deref() one line below — so the omission on the adjacent try path is an incomplete fix of the same logical change.
Fix
Add errdefer sink.deref(); immediately after line 2954. This covers the try fromJSWithTag path and any future try-paths in this block. Optionally, the manual sink.deref() calls in the .err branches (lines 2980, 2995) could then become plain returns relying on the errdefer, though leaving them as-is is also fine since those branches don't return a Zig error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fuzzer fingerprint:
2b96d44db8b7b9baWhat
Bun.file(...).writer(options)crashed with a union-tag safety panic whenoptionscontained a non-stringpathor non-integerfd:Why
streams.Start.fromJSWithTagreturns a.errvariant of theStartunion when thepath/fdoptions are the wrong type.Blob.getWriterthen unconditionally accessedstream_start.FileSink.input_path, which panics in debug builds when the active tag is.err(and is UB in release).Fix
Switch on the returned tag: throw a proper JS error for
.err, and only overwriteinput_pathwhen the tag is.FileSink. Also deinit the allocated path slice returned byfromJSWithTagbefore overwriting it with the blob's own path to avoid a small leak.