Add regression test for Bun.file().writer() with invalid path/fd in options - #30246
Add regression test for Bun.file().writer() with invalid path/fd in options#30246robobun wants to merge 1 commit into
Conversation
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughFixes a potential crash in ChangesWriter Options 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: 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 `@src/runtime/webcore/Blob.zig`:
- Around line 2977-2982: The parsed stream_start from
jsc.WebCore.streams.Start.fromJSWithTag may own a .FileSink.input_path that must
be freed before you overwrite it; update the branch handling around stream_start
and .FileSink so you either deinit the existing stream_start.FileSink.input_path
before assigning input_path, or construct a new .FileSink object for the
fallback and assign that to stream_start without mutating the originally parsed
value. Ensure you reference stream_start and .FileSink and properly release any
owned path (deinit/free the previous input_path) before replacing to avoid
leaking the owned allocation.
In `@test/js/bun/util/filesink.test.ts`:
- Around line 272-301: Replace the use of tmpdirSync() in the test
"Bun.file().writer() ignores invalid path/fd in options instead of crashing"
(the local variable dir) with the harness-provided tempDir helper so the test
directory is disposable and auto-cleaned; change the dir assignment to call
tempDir() and add/import tempDir from the test harness, keeping the rest of the
test (join(dir, "a.txt") etc.) unchanged.
🪄 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: e8ae4241-c3e2-4b9b-a04a-f6a99dfefc76
📒 Files selected for processing (2)
src/runtime/webcore/Blob.zigtest/js/bun/util/filesink.test.ts
|
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}
There was a problem hiding this comment.
LGTM — straightforward union-tag check to fix a fuzzer-found panic; the inline notes are a pre-existing leak and a nit on garbage input, neither blocking.
Extended reasoning...
Overview
This PR changes 4 lines in src/runtime/webcore/Blob.zig (getWriter) and adds a regression test in test/js/bun/util/filesink.test.ts. Previously, after calling Start.fromJSWithTag(..., .FileSink), the code unconditionally accessed stream_start.FileSink.input_path. But fromJSWithTag returns a .err variant when options.path is not a string or options.fd is not an integer (streams.zig:127-150), causing a safe-build panic on inactive-union-field access. The fix checks the tag and falls back to a fresh .FileSink with the Blob's own path when the result isn't .FileSink. Three test cases cover the fuzzer repro (file.fd = Int16Array; file.writer(file)), explicit non-integer fd, and non-string path.
Security risks
None. This is a defensive tag check on a tagged union in a local-filesystem write path. No auth, crypto, parsing of untrusted network data, or permission logic is touched. The change strictly narrows behavior from a panic to the same write the Blob would have done with no options.
Level of scrutiny
Low. The diff is tiny, mechanical, and obviously correct: it replaces an unchecked union field access with a tag check + fallback to the exact same default that's already initialized at line 2970-2974. The fallback semantics (ignore irrelevant path/fd in options, since the Blob already carries its target) match the PR description and existing intent — the old code already overwrote input_path unconditionally.
Other factors
The bug-hunting system surfaced two findings, both posted inline: (1) a pre-existing leak where a valid options.path slice from fromJSWithTag is overwritten without .deinit() — this predates the PR and is unchanged by it; (2) a nit that highWaterMark is dropped in the .err fallback — only reachable with contrived garbage input that previously panicked, so any non-crashing behavior is strictly an improvement. Neither blocks this panic fix. The PR has clear test coverage that exercises the exact crash and verifies writes succeed afterward.
There was a problem hiding this comment.
LGTM — the leak fix and tempDir switch in 91379ca address the earlier feedback; the remaining highWaterMark note is a non-blocking nit.
Extended reasoning...
Overview
This PR fixes a fuzzer-found panic in Blob.getWriter (src/runtime/webcore/Blob.zig:2976-2983) where stream_start.FileSink was accessed without checking that fromJSWithTag actually returned the .FileSink variant rather than .err. The fix guards the union access, adds a .deinit() on the parsed input_path before overwriting it (closing a small pre-existing leak), and falls back to a fresh .FileSink when the parse failed. A regression test in test/js/bun/util/filesink.test.ts exercises four shapes of bad/overridden path/fd options.
Security risks
None. This only affects how invalid option types are handled when constructing a file writer for a Blob that already has its own path/fd; no new inputs reach the filesystem layer that didn't before.
Level of scrutiny
Low. The Zig change is 5 lines, mechanical (tag check + deinit + fallback struct), and strictly defensive — the previous behavior was a safe-build panic on garbage input. The test follows existing patterns in the same file. I verified that join(dir, ...) works with the tempDir return value because Bun's validateString (src/jsc/JSType.zig isStringLike) accepts DerivedStringObject, so the DisposableString passes.
Other factors
Commit 91379ca addressed both pieces of prior feedback: the parsed input_path is now freed before being overridden, and the test uses tempDir instead of tmpdirSync. My one outstanding inline note — that highWaterMark is dropped when the fallback branch is taken — only applies to inputs that previously crashed and is purely a buffer-sizing hint, so it does not block.
|
Updated 6:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit b7aff16 has some failures in 🧪 To try this PR locally: bunx bun-pr 30246That installs a local version of the PR into your bun-30246 --bun |
|
CI status: builds #56882 and #56887 (before/after retrigger) both fail only on |
91379ca to
2df71ae
Compare
robobun
changed the title
There was a problem hiding this comment.
Thanks for dropping the Blob.zig hunk — now this is a clean test-only regression guard for the Rust impl. LGTM.
Extended reasoning...
Overview
After commit 0d24439 dropped the dead-code Blob.zig edit (per my prior review), this PR now touches only test/js/bun/util/filesink.test.ts: it imports tempDir from harness and adds one regression test verifying that Bun.file(path).writer({...}) ignores invalid path/fd option types instead of panicking. The behavior under test already ships in src/runtime/webcore/Blob.rs:1935-1943, so this is purely a regression guard with no runtime/source changes.
Security risks
None. Test-only change; no auth, crypto, permissions, or input-handling code is modified.
Level of scrutiny
Low. The diff adds ~40 lines of test code that write to a disposable temp directory and assert file contents. CI build #55665 ran on commit 2df71ae (which already contained this exact test, on top of the Rust rewrite) and the only failure was an unrelated v8-heap-snapshot.test.ts SIGKILL on one Linux shard. I verified that Bun's path.join accepts DerivedStringObject (JSValue.rs:281 → JSType.rs:794-798 includes DerivedStringObject in is_string_like), so join(dir, ...) works with the DisposableString returned by tempDir even though Node.js would reject it — and CI confirms the test passes.
Other factors
All prior review feedback has been addressed: CodeRabbit's input_path.deinit() and tempDir suggestions were applied in 2df71ae (both threads resolved), and my dead-code observation was addressed in 0d24439. My remaining unresolved nit about highWaterMark in the .err fallback targeted Blob.zig which is no longer compiled, so it's moot. The bug-hunting system found no issues in the current revision.
0d24439 to
73b5d85
Compare
There was a problem hiding this comment.
LGTM — thanks for dropping the Blob.zig hunk; test-only regression guard now.
Extended reasoning...
Overview
This PR now touches a single file, test/js/bun/util/filesink.test.ts, adding one regression test ("Bun.file().writer() ignores invalid path/fd in options instead of crashing") and importing tempDir from the harness. No production code is modified. The test covers four variants of garbage path/fd values in the options object passed to Bun.file(p).writer(opts) and asserts the write still lands at the Blob's own path.
What changed since my last review
On 2026-05-18 I flagged that the src/runtime/webcore/Blob.zig change was dead code (the shipped implementation is Blob.rs, which already handles the non-FileSink variant). The author dropped that hunk and retitled/redescribed the PR as a test-only regression guard, which is exactly what I suggested. The earlier CodeRabbit nits (use tempDir instead of tmpdirSync; deinit the parsed input_path) are also resolved — the former is reflected in the diff and the latter is moot now that the Zig change is gone. All prior inline comments on the PR are marked resolved.
Security risks
None. This is a test-file-only addition that writes to a disposable temp directory created via the harness tempDir helper. It does not touch auth, crypto, network, or any production code path.
Level of scrutiny
Low. Test-only, ~40 lines, follows existing patterns in the same file (e.g. the neighboring start() without path/fd test), uses the recommended using dir = tempDir(...) idiom for cleanup, and join(dir, ...) works because tempDir returns a string & Disposable. The as any casts are intentional to feed type-invalid inputs that exercise the runtime's defensive path.
Other factors
No CODEOWNERS entry covers this file. CI on an earlier revision (0d24439) was green with only an unrelated agent-queue expiry; the latest build (#56882 on 73b5d85) is in progress. The behavior under test already ships in Blob.rs, so the test is purely a guard against future regression and carries no functional risk.
There was a problem hiding this comment.
LGTM — thanks for dropping the Blob.zig hunk; test-only regression guard now.
Extended reasoning...
Overview
This PR now touches a single file, test/js/bun/util/filesink.test.ts, adding one regression test (~39 lines) that exercises Bun.file().writer(options) with invalid fd/path option types. No production code is modified — the underlying fix already shipped in Blob.rs as part of the Rust rewrite, and this test guards against regression.
Prior feedback addressed
On 2026-05-18 I flagged that the src/runtime/webcore/Blob.zig edit was dead code (Zig sources are non-compiled porting references post-Rust-rewrite) and recommended keeping only the test. That hunk has since been removed; the PR is now purely additive test coverage. The CodeRabbit nit about tempDir vs tmpdirSync was also applied, and the join(dir, ...) pattern with a DisposableString matches existing usage (e.g. test/js/bun/shell/leak.test.ts). All prior inline comments are resolved.
Security risks
None. Test-only change writing single-byte files into an auto-cleaned temp directory.
Level of scrutiny
Low. This is an additive regression test in the test suite with no runtime impact. The four sub-cases are straightforward write-then-read assertions following the same patterns as adjacent tests in the file.
Other factors
CI build #56882 shows one failure in AsyncLocalStorage-tracking.test.ts (segfault on macOS 14 x64), which is unrelated to this change. The earlier build on this branch (#55674) was green. The bug-hunting system found no issues in the current diff.
|
CI status on b7aff16 (build #95621): all four failures are flagged flaky by the CI tooling and are unrelated to this test-only change ( |
a376ddb to
5c490f9
Compare
There was a problem hiding this comment.
LGTM — thanks for dropping the dead Blob.zig hunk and switching to tempDir; this is now a clean test-only regression guard.
Extended reasoning...
Overview
The PR now touches a single file, test/js/bun/util/filesink.test.ts, adding one ~38-line regression test that exercises Bun.file(path).writer(options) with four malformed/irrelevant path/fd option shapes (non-integer fd via self-reference, explicit string fd, numeric path, and a string path that should be overridden by the Blob's own path). Each case writes a byte and asserts the file content. The earlier src/runtime/webcore/Blob.zig change has been removed per prior feedback — the Rust port (Blob.rs) already handles the non-FileSink variant, so this PR is purely a regression guard for behavior that already ships.
Security risks
None. This is a test-only change that runs in the test harness's temp directory, performs no network/auth/crypto operations, and does not alter any shipped runtime code.
Level of scrutiny
Low. Test-only additions to an existing test file, following the established patterns in that file (using dir = tempDir(...), Bun.file(...).writer(), await writer.end()). The tempDir helper returns a disposable String subclass, so join(dir, ...) and the using cleanup both work as intended. The worst-case failure mode is a flaky/failing test, not a runtime regression.
Other factors
- All prior review feedback is addressed and marked resolved: the dead
Blob.zigedit was dropped, the CodeRabbittempDirnit was applied, and the title/description were updated to reflect the test-only scope. - CI (robobun) reports the only failure is the pre-existing, unrelated macOS x64
AsyncLocalStorage-tracking.test.ts > fs-watchsegfault;filesink.test.tsitself passes 43/43 on that shard. - The bug-hunting system found no issues in the current diff.
- I have not previously approved this PR; my earlier comments were on the since-removed
Blob.zighunk.
…ptions The Zig implementation used to panic accessing the FileSink union field when Start.fromJSWithTag returned the err variant for a non-string path or non-integer fd in the options object. The Rust port already checks the variant before accessing it, so this adds coverage to guard against regression.
5c490f9 to
b7aff16
Compare
There was a problem hiding this comment.
LGTM — test-only regression guard now that the Blob.zig hunk is dropped.
What was reviewed:
- Confirmed
Blob.rs:1913-1920still has theif let streams::Start::FileSinkfallback the test guards, and that a validoptions.pathis overridden by the Blob's own path (4th sub-case). join(dir, ...)on atempDirDisposableStringmatches existing usage (e.g.patch.test.ts), so no arg-type issue.- Prior nits (
tempDirovertmpdirSync, deadBlob.zigedit) are addressed; all earlier inline comments resolved.
Extended reasoning...
Overview
The PR now touches a single file, test/js/bun/util/filesink.test.ts, adding one it() block with four sub-cases exercising Bun.file(path).writer(options) when options carries a non-integer fd, a non-string path, or a valid string path that should be overridden by the Blob's own path. Earlier revisions also edited src/runtime/webcore/Blob.zig, but that hunk was dropped after my 2026-05-18 review flagged it as dead reference code post-Rust-rewrite; the diff is now purely additive test coverage.
Security risks
None. The test writes single-byte files into a per-test tempDir that is disposed via using, reads them back, and asserts exact contents. No network, no external processes, no security-sensitive code paths.
Level of scrutiny
Low. This is a small test-only addition guarding behavior that already ships in src/runtime/webcore/Blob.rs (verified at lines 1913-1920: the if let streams::Start::FileSink(ref mut opts) check with an else fallback that reconstructs FileSink from the Blob's own input_path). The test follows harness conventions: using dir = tempDir(...), exact .toBe(...) assertions on file contents rather than bare not.toThrow(), and no sleeps or hardcoded ports. I checked that join(dir, ...) on the DisposableString return of tempDir is an established pattern in the suite (test/js/bun/patch/patch.test.ts uses it extensively), so Bun's path.join accepts the boxed String.
Other factors
All four of my prior inline comments and both CodeRabbit comments are marked resolved and were on lines no longer in the diff. The two robobun CI comments report the test file passing on all shards with unrelated flakes elsewhere; binary size delta is 0 KB as expected for test-only. No human reviewers have outstanding comments. The PR description accurately reflects the current scope (regression guard for the Rust impl, no source change).
1 participant
What
Adds a regression test for
Bun.file(path).writer(options)when the options object has a non-stringpathor non-integerfdproperty.Why
In the Zig implementation,
Start.fromJSWithTag(..., .FileSink)returned a.errvariant for these inputs, andBlob.getWriteraccessed.FileSinkunconditionally, panicking withaccess of union field 'FileSink' while field 'err' is active.The Rust port (
Blob.rs) already checksif let streams::Start::FileSink(...)before accessing the variant and falls back to the Blob's own path otherwise, so the crash no longer reproduces on main. This PR adds test coverage to guard against regression.Fuzzer fingerprint:
defc7abf146bf165