Skip to content

Fix crash in Bun.file().writer() with invalid path/fd options #30299

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() with invalid path/fd options #30299

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
6 changes: 5 additions & 1 deletion src/runtime/webcore/Blob.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2975,7 +2975,11 @@

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;
if (stream_start == .FileSink) {
stream_start.FileSink.input_path = input_path;

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

View check run for this annotation

Claude / Claude Code Review

Pre-existing leak: allocated input_path overwritten without deinit 

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.
Comment on lines +2978 to +2979

@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 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

  1. User calls Bun.file("/tmp/x").writer({ path: "unused-😀" }).
  2. arguments[0].isObject() → true, call fromJSWithTag(.FileSink).
  3. value.fastGet(.path) returns the JS string "unused-😀"; path.isString() → true.
  4. path.toSlice(globalThis, allocator) must transcode UTF-16 → UTF-8 because of the emoji, so it heap-allocates and returns a ZigString.Slice with allocator != null.
  5. fromJSWithTag returns .{ .FileSink = .{ .input_path = .{ .path = <owned slice> } } }.
  6. Back in getWriter, stream_start == .FileSink → take the new if branch.
  7. 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.
  8. defer input_path.deinit() later frees only the Blob's path. The fromJSWithTag allocation 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.)

} else {
stream_start = .{ .FileSink = .{ .input_path = input_path } };
}
}

switch (sink.start(stream_start)) {
Expand Down
14 changes: 14 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 @@ -268,3 +268,17 @@ it.skipIf(!isPosix)("does not leak native FileSink when a pending write fails (E
// more than that indicates a native leak.
expect(fileSinkInternals.liveCount()).toBeLessThanOrEqual(baseline + 1);
});

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

@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 (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.

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

const writer = Bun.file(path).writer(options as any);
writer.write("hello");
await writer.end();
expect(await Bun.file(path).text()).toBe("hello");
});
});
Loading