Fix crash in Bun.file().writer() with invalid path/fd options - #30299
Fix crash in Bun.file().writer() with invalid path/fd options#30299robobun wants to merge 1 commit into
Bun.file().writer() with invalid path/fd options#30299Conversation
When an options object passed to Bun.file().writer() contains a non-string path or non-integer fd property, Start.fromJSWithTag returns the .err tag instead of .FileSink. The caller then unconditionally accessed stream_start.FileSink.input_path, triggering a union safety panic. Since the Blob already has its own path/fd, fall back to the default FileSink options with the Blob's input_path when fromJSWithTag does not return .FileSink.
|
Updated 3:50 PM PT - May 5th, 2026
❌ @robobun, your commit c13f82d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30299That installs a local version of the PR into your bun-30299 --bun |
WalkthroughThe PR validates the Start variant returned by Start.fromJSWithTag when constructing stream targets in Blob.zig, robustly handling both FileSink and non-FileSink cases. Test coverage is added for Bun.file().writer() invocation with invalid options parameters. ChangesFile Writer Robustness
🚥 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/filesink.test.ts`:
- Line 278: Replace the tmpdirSync-based temp path with the test harness
tempDir: instead of calling join(tmpdirSync(), "filesink-invalid-options.txt")
update the test's temp-path setup to use the harness-provided tempDir helper
(e.g., use join(tempDir(), "filesink-invalid-options.txt")) so the test uses the
repository test lifecycle/cleanup; change the reference where the const path is
defined in filesink.test.ts accordingly and ensure any necessary harness import
is present.
🪄 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: 9764e7c3-3d3c-42d1-9666-8d2289d66a78
📒 Files selected for processing (2)
src/runtime/webcore/Blob.zigtest/js/bun/util/filesink.test.ts
| ["non-integer fd", { fd: "notanint" }], | ||
| ["arbitrary object", Bun], | ||
| ])("does not crash with %s", async (_, options) => { | ||
| const path = join(tmpdirSync(), "filesink-invalid-options.txt"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use tempDir (harness) instead of tmpdirSync in the new test case.
Please switch this new temp-path setup to tempDir so cleanup/lifecycle follows the repo’s test convention.
Suggested patch
-import { fileDescriptorLeakChecker, isPosix, isWindows, tmpdirSync } from "harness";
+import { fileDescriptorLeakChecker, isPosix, isWindows, tempDir, tmpdirSync } from "harness";
describe("Bun.file().writer() with invalid options", () => {
it.each([
["non-string path", { path: 123 }],
["non-integer fd", { fd: "notanint" }],
["arbitrary object", Bun],
])("does not crash with %s", async (_, options) => {
- const path = join(tmpdirSync(), "filesink-invalid-options.txt");
+ using dir = tempDir("filesink-invalid-options");
+ const path = join(dir, "filesink-invalid-options.txt");
const writer = Bun.file(path).writer(options as any);
writer.write("hello");
await writer.end();
expect(await Bun.file(path).text()).toBe("hello");
});
});As per coding guidelines, “Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const path = join(tmpdirSync(), "filesink-invalid-options.txt"); | |
| import { fileDescriptorLeakChecker, isPosix, isWindows, tempDir, tmpdirSync } from "harness"; | |
| describe("Bun.file().writer() with invalid options", () => { | |
| it.each([ | |
| ["non-string path", { path: 123 }], | |
| ["non-integer fd", { fd: "notanint" }], | |
| ["arbitrary object", Bun], | |
| ])("does not crash with %s", async (_, options) => { | |
| using dir = tempDir("filesink-invalid-options"); | |
| const path = join(dir, "filesink-invalid-options.txt"); | |
| const writer = Bun.file(path).writer(options as any); | |
| writer.write("hello"); | |
| await writer.end(); | |
| expect(await Bun.file(path).text()).toBe("hello"); | |
| }); | |
| }); |
🤖 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` at line 278, Replace the tmpdirSync-based
temp path with the test harness tempDir: instead of calling join(tmpdirSync(),
"filesink-invalid-options.txt") update the test's temp-path setup to use the
harness-provided tempDir helper (e.g., use join(tempDir(),
"filesink-invalid-options.txt")) so the test uses the repository test
lifecycle/cleanup; change the reference where the const path is defined in
filesink.test.ts accordingly and ensure any necessary harness import is present.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-tested fix for the union safety panic; the flagged leak is pre-existing and a one-line follow-up if you want to address it here.
Extended reasoning...
Overview
This PR fixes a debug-mode union safety panic (and release-mode UB) in Blob.getWriter (src/runtime/webcore/Blob.zig:2976-2983). Previously, after calling Start.fromJSWithTag(.FileSink), the code unconditionally accessed stream_start.FileSink.input_path, but fromJSWithTag can return .err when the options object has a non-string path or non-integer fd. The fix adds a tag check and falls back to a default .FileSink variant with the Blob's own input_path when the result isn't .FileSink. Three regression tests are added to test/js/bun/util/filesink.test.ts.
Security risks
None. This is a defensive fix in an options-parsing path. The path/fd from the options object was already being overwritten by the Blob's own pathlike before this change, so no new inputs reach the filesystem layer. The .err variant carries no owned resources, so discarding it in the else branch doesn't leak.
Level of scrutiny
Low. The diff is +5/-1 in production code, the logic is a straightforward tag check, and the fallback preserves prior release-mode behavior (where the union access silently "worked" because the fields happen to overlap). The choice to silently ignore the invalid path/fd rather than throw is reasonable since those options were always overridden by the Blob's own path anyway — throwing would be a behavior regression for code that previously ran in release builds.
Other factors
- The fuzzer fingerprint and minimal repros in the description match the fix.
- Regression tests verify the writer still functions correctly (writes "hello" and reads it back) for all three invalid-option shapes.
- The one inline finding is a pre-existing minor leak (heap-allocated
input_pathfromfromJSWithTagoverwritten withoutdeinit), reachable only via an undocumented and ignored{ path: string }option. It's not introduced or worsened by this PR; flagged only because the PR rewrites that exact line and the fix is a one-liner. It does not block approval.
| if (stream_start == .FileSink) { | ||
| stream_start.FileSink.input_path = input_path; |
There was a problem hiding this comment.
🟣 Pre-existing minor leak (not introduced by this PR, but you're touching the exact line): when fromJSWithTag returns .FileSink with a heap-allocated input_path (i.e. the user passed { path: "…" } and toSlice allocated for UTF-8 conversion), line 2979 overwrites stream_start.FileSink.input_path without calling .deinit() on the previous value, leaking the ZigString.Slice. Adding stream_start.FileSink.input_path.deinit(); before the assignment would close it.
Extended reasoning...
What the bug is
Start.fromJSWithTag(.FileSink) can return a .FileSink variant whose input_path.path is a heap-owning ZigString.Slice. At streams.zig:139, when the options object has a string path property, it does:
.input_path = .{
.path = try path.toSlice(globalThis, globalThis.bunVM().allocator),
},JSValue.toSlice → bun.String.toUTF8(allocator) returns a ZigString.Slice with a non-null allocator whenever conversion is required (e.g. the JS string contains non-Latin1 code points, or is a rope that needs flattening). That slice must be freed via ZigString.Slice.deinit() / PathOrFileDescriptor.deinit().
Back in Blob.getWriter:
stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink);
if (stream_start == .FileSink) {
stream_start.FileSink.input_path = input_path; // <-- overwrite, no deinit
} else { ... }The plain struct assignment on line 2979 drops the previous input_path on the floor without freeing it. The defer input_path.deinit(); at line 2968 only frees the Blob-derived input_path, not the one that came back from fromJSWithTag.
Why existing code doesn't prevent it
There is no deinit anywhere on the fromJSWithTag-returned input_path. sink.start() borrows the path (it dupes / opens it), it does not take ownership. And the defer at 2968 covers a different value.
Step-by-step proof
- User calls
Bun.file("/tmp/x").writer({ path: "unused-😀" }). arguments[0].isObject()→ true, callfromJSWithTag(.FileSink).value.fastGet(.path)returns the JS string"unused-😀";path.isString()→ true.path.toSlice(globalThis, allocator)must transcode UTF-16 → UTF-8 because of the emoji, so it heap-allocates and returns aZigString.Slicewithallocator != null.fromJSWithTagreturns.{ .FileSink = .{ .input_path = .{ .path = <owned slice> } } }.- Back in
getWriter,stream_start == .FileSink→ take the newifbranch. stream_start.FileSink.input_path = input_path;overwrites the owned slice with the Blob's own duped path. The emoji-string allocation is now unreachable.defer input_path.deinit()later frees only the Blob's path. ThefromJSWithTagallocation leaks.
Impact
A small per-call heap leak, only reachable via the undocumented { path: string } option to Bun.file().writer() (which is ignored in favor of the Blob's own path anyway). Low severity, but real.
Pre-existing
Before this PR the line was the unconditional stream_start.FileSink.input_path = input_path;, so the leak already existed. The PR neither introduces nor worsens it; flagging only because the PR rewrites this exact block and the fix is a one-liner.
Fix
if (stream_start == .FileSink) {
stream_start.FileSink.input_path.deinit();
stream_start.FileSink.input_path = input_path;
} else {
stream_start = .{ .FileSink = .{ .input_path = input_path } };
}(The else branch — .err / .empty / .ready — carries no owned resources, so nothing to free there.)
|
Duplicate of #28388 (and 7 others). Closing in favor of the earliest. |
1 participant
What does this PR do?
Fixes a debug-mode panic (union safety check) in
Bun.file().writer(options)when the options object contains a non-stringpathor non-integerfdproperty.Start.fromJSWithTag(.FileSink)can return.errwhen the options object has an invalidpath/fd, butBlob.getWriterunconditionally accessedstream_start.FileSink.input_pathafterward. Since the Blob already provides its own path/fd (which is what gets written to), we now fall back to the defaultFileSinkoptions with the Blob'sinput_pathin that case.How did you verify your code works?
Minimal repros that panicked before and now work:
Added regression tests to
test/js/bun/util/filesink.test.ts.Found by Fuzzilli (fingerprint
a0ee9cd4fa588613).