Fix crash in Bun.file().writer() when options has non-string path - #30294
Fix crash in Bun.file().writer() when options has non-string path#30294robobun wants to merge 1 commit into
Bun.file().writer() when options has non-string path#30294Conversation
fromJSWithTag can return .err (e.g. when the options object has a non-string path property), but getWriter unconditionally accessed .FileSink afterwards. In debug this panics; in release the sink is never set up and writes silently go nowhere. Since the blob's own input_path is always used (any path/fd in the options is overwritten), ignore the parsed result when it is not a .FileSink and keep the already-initialized stream_start. Also deinit the parsed input_path before overwriting it to avoid leaking when a string path option was provided.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
WalkthroughThe ChangesFileSink Initialization Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 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-220: Replace the tmpdirSync() usage with the harness-provided
tempDir helper in this test: find the test case containing tmpdirSync() creating
variable x and change it to call tempDir() (or the appropriate harness tempDir
factory) and use its returned path to compute dest and create Bun.file(dest);
keep the rest of the test (options definition, file.writer(options), write/end,
and assertion) unchanged so cleanup and guideline compliance are preserved.
- Around line 210-220: Add a companion regression test that mirrors the existing
invalid-path case but sets a non-integer/non-number fd on the options object to
ensure file.writer handles bad fd values safely: in the same test file create an
options object and use Object.defineProperty(options, "fd", { enumerable: true,
value: some non-integer like Uint32Array }) then call file.writer(options),
write/end the stream and assert the file contents are correct; reference the
existing writer() usage and options object setup to keep the test consistent
with the "path" variant.
🪄 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: 3dc380b3-bb72-429a-8e3e-cec797081a69
📒 Files selected for processing (2)
src/runtime/webcore/Blob.zigtest/js/bun/util/filesink.test.ts
| it("writer() does not crash when options object has a non-string 'path' property", async () => { | ||
| const x = tmpdirSync(); | ||
| const dest = path.join(x, "test.txt"); | ||
| const file = Bun.file(dest); | ||
| const options = {}; | ||
| Object.defineProperty(options, "path", { enumerable: true, value: Uint32Array }); | ||
| const writer = file.writer(options); | ||
| await writer.write("hello"); | ||
| await writer.end(); | ||
| expect(await Bun.file(dest).text()).toBe("hello"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use tempDir instead of tmpdirSync in this new test.
Line 211 introduces a new tmpdirSync() call in a .test.ts file; please switch this case to tempDir for guideline compliance and automatic cleanup.
♻️ Proposed change
-import { fileDescriptorLeakChecker, isPosix, isWindows, tmpdirSync } from "harness";
+import { fileDescriptorLeakChecker, isPosix, isWindows, tempDir, tmpdirSync } from "harness";
@@
it("writer() does not crash when options object has a non-string 'path' property", async () => {
- const x = tmpdirSync();
- const dest = path.join(x, "test.txt");
+ using x = tempDir("filesink-writer-non-string-path");
+ const dest = path.join(x, "test.txt");
const file = Bun.file(dest);
const options = {};
Object.defineProperty(options, "path", { enumerable: true, value: Uint32Array });
const writer = file.writer(options);
await writer.write("hello");
await writer.end();
expect(await Bun.file(dest).text()).toBe("hello");
});As per coding guidelines: test/**/*.test.ts: “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.
| it("writer() does not crash when options object has a non-string 'path' property", async () => { | |
| const x = tmpdirSync(); | |
| const dest = path.join(x, "test.txt"); | |
| const file = Bun.file(dest); | |
| const options = {}; | |
| Object.defineProperty(options, "path", { enumerable: true, value: Uint32Array }); | |
| const writer = file.writer(options); | |
| await writer.write("hello"); | |
| await writer.end(); | |
| expect(await Bun.file(dest).text()).toBe("hello"); | |
| }); | |
| it("writer() does not crash when options object has a non-string 'path' property", async () => { | |
| using x = tempDir("filesink-writer-non-string-path"); | |
| const dest = path.join(x, "test.txt"); | |
| const file = Bun.file(dest); | |
| const options = {}; | |
| Object.defineProperty(options, "path", { enumerable: true, value: Uint32Array }); | |
| const writer = file.writer(options); | |
| await writer.write("hello"); | |
| await writer.end(); | |
| expect(await Bun.file(dest).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` around lines 210 - 220, Replace the
tmpdirSync() usage with the harness-provided tempDir helper in this test: find
the test case containing tmpdirSync() creating variable x and change it to call
tempDir() (or the appropriate harness tempDir factory) and use its returned path
to compute dest and create Bun.file(dest); keep the rest of the test (options
definition, file.writer(options), write/end, and assertion) unchanged so cleanup
and guideline compliance are preserved.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a companion regression case for invalid non-integer fd options.
This PR fixes both invalid path and invalid fd option handling, but the new test only covers path. Please add the fd variant to lock both paths.
🧪 Suggested companion test
+it("writer() does not crash when options object has a non-integer 'fd' property", async () => {
+ using x = tempDir("filesink-writer-non-int-fd");
+ const dest = path.join(x, "test.txt");
+ const file = Bun.file(dest);
+ const options = {};
+ Object.defineProperty(options, "fd", { enumerable: true, value: "not-an-int" });
+ const writer = file.writer(options);
+ await writer.write("hello");
+ await writer.end();
+ expect(await Bun.file(dest).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` around lines 210 - 220, Add a companion
regression test that mirrors the existing invalid-path case but sets a
non-integer/non-number fd on the options object to ensure file.writer handles
bad fd values safely: in the same test file create an options object and use
Object.defineProperty(options, "fd", { enumerable: true, value: some non-integer
like Uint32Array }) then call file.writer(options), write/end the stream and
assert the file contents are correct; reference the existing writer() usage and
options object setup to keep the test consistent with the "path" variant.
|
Duplicate of #28388 (and several others). Closing. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-reasoned fix for a fuzzer-found union-tag crash, with a regression test.
Extended reasoning...
Overview
This PR changes 6 lines in Blob.getWriter (src/runtime/webcore/Blob.zig) plus adds one regression test in test/js/bun/util/filesink.test.ts. Previously, fromJSWithTag(..., .FileSink) could return .{ .err = ... } when the options object had a non-string path or non-integer fd, and the caller unconditionally accessed stream_start.FileSink.input_path, panicking in debug and silently misbehaving in release. The fix stores the result in a local, checks the union tag, and only adopts it when it is .FileSink; it also deinit()s the parsed input_path before overwriting it (closing a separate pre-existing leak when a string path option was supplied). I confirmed PathOrFileDescriptor.deinit() is a no-op for the .fd variant, so the new deinit is safe in all return shapes.
Security risks
None. This is defensive handling of malformed user-supplied options to a local file writer; no auth, crypto, or network surface is touched.
Level of scrutiny
Low. The change is mechanical (guard a tagged-union access with a tag check), the rationale in the PR description matches the code in fromJSWithTag, and the behavioral choice — ignore an invalid path/fd in options because the blob's own path always overwrites it anyway — is sound and strictly better than the prior crash. The regression test exercises the exact fuzzer repro and asserts the write actually lands on disk.
Other factors
The bug-hunt surfaced a pre-existing FileSink leak when a getter on the options object throws during fromJSWithTag (no errdefer sink.deref() covers the try at line 2977). That's adjacent and worth a follow-up, but it's not introduced or worsened by this PR, so it doesn't block. No CODEOWNERS apply to these paths and there are no outstanding reviewer comments.
| if (arguments.len > 0 and arguments.ptr[0].isObject()) { | ||
| stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink); | ||
| stream_start.FileSink.input_path = input_path; | ||
| var parsed = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink); |
There was a problem hiding this comment.
🟣 Pre-existing, but since this PR is hardening this exact call against malformed options: if a getter on the options object throws (e.g. Object.defineProperty(opts, 'highWaterMark', {get(){throw 0}})), the try here propagates error.JSError and the heap-allocated sink from line 2954 is never released. Adding errdefer sink.deref(); right after the FileSink.init(...) call would close the leak.
Extended reasoning...
What the bug is
In Blob.getWriter (src/runtime/webcore/Blob.zig:2954), sink is heap-allocated via jsc.WebCore.FileSink.init(bun.invalid_fd, ...). FileSink.init (FileSink.zig:553-563) calls bun.new(FileSink, ...), initializes ref_count = 1, and increments live_count. Twenty-three lines later, line 2977 does try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink). If that try propagates, there is no errdefer sink.deref() in scope — the only cleanup registered between allocation and the try is defer input_path.deinit() at line 2968. The FileSink (and its writer/buffers) leak for the rest of the process, and live_count is never decremented.
Code path that triggers it
fromJSWithTag (streams.zig:70-143) returns bun.JSError!Start. The .FileSink branch calls 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 the user-supplied options object has a throwing getter or a Proxy trap that throws. That error propagates straight out of getWriter via the try at line 2977.
Why nothing catches it
All other error paths in this function explicitly call sink.deref() before returning (e.g. lines 2944, 2987), but those are reached via switch on a result value, not via Zig error propagation. The one try in this region has no matching errdefer. The sink is not yet attached to a JS wrapper (sink.toJS(globalThis) only happens at line 2993), so GC cannot reclaim it either.
Step-by-step proof
const opts = {}; Object.defineProperty(opts, 'highWaterMark', { get() { throw new Error('boom'); } });Bun.file('/tmp/x').writer(opts);getWriterreaches line 2954 →sink = FileSink.init(...)heap-allocates,ref_count = 1,live_count++.- Line 2956-2968:
input_pathis built and adefer input_path.deinit()is registered. - Line 2976:
arguments[0].isObject()is true. - Line 2977:
fromJSWithTagcallsopts.highWaterMarkgetter → throws → returnserror.JSError. trypropagates.defer input_path.deinit()runs (path is freed). Noerrdeferforsinkexists.sinkleaks.- Repeating in a loop grows
fileSinkInternals.liveCount()unboundedly.
Impact
Per-call heap leak of a FileSink struct plus its embedded writer state. Reachable only with adversarial/unusual options objects (throwing getters / Proxy traps), so it is not a correctness problem for normal code, but it is exactly the class of fuzzer-discovered malformed-options input this PR is hardening against — Fuzzilli will hit this with the same generator that produced the non-string path crash.
Fix
Add one line immediately after line 2954:
var sink = jsc.WebCore.FileSink.init(bun.invalid_fd, this.globalThis.bunVM().eventLoop());
errdefer sink.deref();This is safe because the only normal-return path after this point is return sink.toJS(globalThis) at line 2993, which does not go through errdefer, and the existing explicit sink.deref() at line 2987 returns via globalThis.throwValue(...) which itself returns error.JSError — but that path already deref'd once and then the errdefer would deref again. To avoid the double-deref on the sink.start error path, either drop the explicit sink.deref() at line 2987 (letting the errdefer handle it) or scope the errdefer more tightly around just the fromJSWithTag call. The simplest correct change is:
if (arguments.len > 0 and arguments.ptr[0].isObject()) {
var parsed = jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink) catch |err| {
sink.deref();
return err;
};
...
}Relationship to this PR
The try predates this PR, so the leak is pre-existing. However, the PR rewrites this exact line to harden against fuzzer-found malformed options, and a throwing getter is the immediately-adjacent fuzzer case on the same property accesses, so it seems worth closing here while the author is already touching the line.
1 participant
What
Fixes a debug panic / release misbehavior in
Bun.file().writer(options)when the options object has apathproperty that is not a string (or anfdproperty that is not an integer).Why
streams.Start.fromJSWithTag(..., .FileSink)returns.{ .err = ... }when the options object has a non-stringpathor non-integerfd.getWriterthen unconditionally accessedstream_start.FileSink.input_path, which:.err,FileSink.startfalls throughelse => {}without opening the file, and writes silently go nowhereHow
Since the blob's own
input_pathis always used here (anypath/fdfrom the options object is immediately overwritten), only apply the parsed options when the result is.FileSink. Otherwise keep the already-initializedstream_start. Alsodeinitthe parsedinput_pathbefore overwriting it to avoid leaking an allocated slice when a stringpathoption is provided.Repro
Found by Fuzzilli. Fingerprint:
19c486a5819df798