Skip to content

Fix crash in Bun.file().writer() when options has non-string path #30294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

Fix crash in Bun.file().writer() when options has non-string path #30294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/runtime/webcore/Blob.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2974,8 +2974,12 @@
};

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);

Check notice on line 2977 in src/runtime/webcore/Blob.zig

View check run for this annotation

Claude / Claude Code Review

FileSink leaks if fromJSWithTag throws (e.g. throwing getter on options) 

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.

@claude claude Bot May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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

  1. const opts = {}; Object.defineProperty(opts, 'highWaterMark', { get() { throw new Error('boom'); } });
  2. Bun.file('/tmp/x').writer(opts);
  3. getWriter reaches line 2954 → sink = FileSink.init(...) heap-allocates, ref_count = 1, live_count++.
  4. Line 2956-2968: input_path is built and a defer input_path.deinit() is registered.
  5. Line 2976: arguments[0].isObject() is true.
  6. Line 2977: fromJSWithTag calls opts.highWaterMark getter → throws → returns error.JSError.
  7. try propagates. defer input_path.deinit() runs (path is freed). No errdefer for sink exists. sink leaks.
  8. 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.

if (parsed == .FileSink) {
parsed.FileSink.input_path.deinit();
parsed.FileSink.input_path = input_path;
stream_start = parsed;
}
}

switch (sink.start(stream_start)) {
Expand Down
12 changes: 12 additions & 0 deletions test/js/bun/util/filesink.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,18 @@ it("write result is not cumulative", async () => {
await util.promisify(fs.close)(fd);
});

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");
});
Comment on lines +210 to +220

@coderabbitai coderabbitai Bot May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.


if (isWindows) {
it("ENOENT, Windows", () => {
expect(() => Bun.file("A:\\this-does-not-exist.txt").writer()).toThrow(
Expand Down
Loading