Skip to content

Fix crash in Blob.writer() when options contain invalid path or fd #30253

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
+23 −1
Closed

Fix crash in Blob.writer() when options contain invalid path or fd #30253

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
14 changes: 13 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 @@ -2974,8 +2974,20 @@
};

if (arguments.len > 0 and arguments.ptr[0].isObject()) {
stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink);

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

View check run for this annotation

Claude / Claude Code Review

FileSink leaked when fromJSWithTag throws JSError 

Pre-existing, but while you're here: the `try` on `fromJSWithTag` at line 2977 can also propagate a `JSError` (e.g. a throwing getter for `highWaterMark`/`path`/`fd` on the options object), in which case the `sink` allocated at line 2954 still leaks. An `errdefer sink.deref();` right after line 2954 would cover both this and the `.err` case you added, in one line.

@claude claude Bot May 4, 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 while you're here: the try on fromJSWithTag at line 2977 can also propagate a JSError (e.g. a throwing getter for highWaterMark/path/fd on the options object), in which case the sink allocated at line 2954 still leaks. An errdefer sink.deref(); right after line 2954 would cover both this and the .err case you added, in one line.

Extended reasoning...

What the bug is

getWriter allocates a ref-counted FileSink at Blob.zig:2954 via jsc.WebCore.FileSink.init(...) (which heap-allocates with bun.new and starts at refcount 1). There is no errdefer sink.deref() between that allocation and the try at line 2977. If fromJSWithTag returns error.JSError, the try propagates it straight out of getWriter and the sink is never released.

The PR correctly adds sink.deref() for the case where fromJSWithTag returns the .err union variant of Start (line 2984), but fromJSWithTag has a second failure mode — a Zig bun.JSError in the error-union — which is propagated by try rather than landing in the switch.

Code path that triggers it

Start.fromJSWithTag for .FileSink (streams.zig:117–150) does:

  • try value.fastGet(globalThis, .highWaterMark) (line ~120)
  • try value.fastGet(globalThis, .path) (line ~125)
  • try value.getTruthy(globalThis, "fd")
  • try path.toSlice(...)

Each of these executes a JS property access on the user-supplied options object. If the options object is a Proxy or has an accessor that throws, the getter throws, fastGet/getTruthy returns error.JSError, and the try in fromJSWithTag propagates it. Back in getWriter, the outer try at line 2977 propagates again, returning before sink is ever deref'd.

Step-by-step proof

Bun.file('/tmp/x').writer({ get highWaterMark() { throw new Error('boom'); } });
  1. Blob.zig:2954 — sink = FileSink.init(...) → heap object, refcount = 1, live_count incremented.
  2. Blob.zig:2956–2968 — input_path is computed; defer input_path.deinit() registered (so the path itself does not leak).
  3. Blob.zig:2976 — arguments.len > 0 and the argument is an object → enter the branch.
  4. Blob.zig:2977 — fromJSWithTag runs value.fastGet(globalThis, .highWaterMark), which invokes the getter, which throws → error.JSError propagates out of fromJSWithTag.
  5. The try at 2977 propagates error.JSError out of getWriter. The switch (lines 2978–2990) is never reached, so the new sink.deref() at 2984 doesn't run.
  6. Only the defer input_path.deinit() from step 2 runs. sink is leaked.

Why existing code doesn't prevent it

There is no errdefer sink.deref() anywhere in this function. All other error paths (lines 2944, 2984, 2995) manually call sink.deref() before returning, but the try at 2977 short-circuits past all of them.

Impact

A native FileSink allocation (plus its associated writer state) is leaked for the lifetime of the process each time this is hit. The trigger is adversarial — a throwing getter or Proxy trap on the options object — so it's unlikely in normal code, but it is the exact same class of bug ("sink not cleaned up on error in this block") that this PR sets out to fix, in the very same lines being edited.

Fix

Add immediately after line 2954:

var sink = jsc.WebCore.FileSink.init(bun.invalid_fd, this.globalThis.bunVM().eventLoop());
errdefer sink.deref();

This covers the try JSError path. With the errdefer in place, the explicit sink.deref() calls at lines 2984 and 2995 become redundant and can be removed (they're followed by error returns), simplifying the function. Note the sink.deref() at line 2944 is in a separate early-return block before line 2954 and would remain.

stream_start.FileSink.input_path = input_path;
switch (stream_start) {
.FileSink => |*file_sink| {
file_sink.input_path.deinit();
file_sink.input_path = input_path;
},
.err => |err| {
sink.deref();
return globalThis.throwValue(try err.toJS(globalThis));
},
else => {
stream_start = .{ .FileSink = .{ .input_path = input_path } };
},
}
}

switch (sink.start(stream_start)) {
Expand Down
10 changes: 10 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,13 @@
// more than that indicates a native leak.
expect(fileSinkInternals.liveCount()).toBeLessThanOrEqual(baseline + 1);
});

it("writer() throws instead of crashing when options has a non-string path", () => {
const file = Bun.file(join(tmpdirSync(), "writer-invalid-path.txt"));
expect(() => file.writer({ path: Int32Array } as any)).toThrow();
});

it("writer() throws instead of crashing when options has an invalid fd", () => {
const file = Bun.file(join(tmpdirSync(), "writer-invalid-fd.txt"));
expect(() => file.writer({ fd: "not-a-number" } as any)).toThrow();
Comment on lines +273 to +279

@coderabbitai coderabbitai Bot May 4, 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 (disposable) instead of tmpdirSync in the newly added tests.

Line 273 and Line 278 add new tmpdirSync() usage; switch these new cases 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() throws instead of crashing when options has a non-string path", () => {
-  const file = Bun.file(join(tmpdirSync(), "writer-invalid-path.txt"));
+  using dir = tempDir("filesink-writer-invalid-path");
+  const file = Bun.file(join(dir, "writer-invalid-path.txt"));
   expect(() => file.writer({ path: Int32Array } as any)).toThrow();
 });

 it("writer() throws instead of crashing when options has an invalid fd", () => {
-  const file = Bun.file(join(tmpdirSync(), "writer-invalid-fd.txt"));
+  using dir = tempDir("filesink-writer-invalid-fd");
+  const file = Bun.file(join(dir, "writer-invalid-fd.txt"));
   expect(() => file.writer({ fd: "not-a-number" } as any)).toThrow();
 });

As per coding guidelines, test/**/*.test.ts: "Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync."

🤖 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 273 - 279, The two new tests
call tmpdirSync() to create temporary directories; replace those calls with the
harness-provided tempDir so the directories are disposable and automatically
cleaned up. Locate the two uses in the tests that create files for file.writer
(the cases checking invalid path and invalid fd) and change tmpdirSync() ->
tempDir (using the harness tempDir utility) so the tests follow the project
guideline for test/**/*.test.ts cleanup semantics.

Comment on lines +274 to +279

@coderabbitai coderabbitai Bot May 4, 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.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Make the throw assertions specific to the expected errno.

Line 274 and Line 279 currently accept any thrown error. Assert code to ensure these tests fail only if the intended validation path regresses.

Proposed assertion strengthening 
-  expect(() => file.writer({ path: Int32Array } as any)).toThrow();
+  expect(() => file.writer({ path: Int32Array } as any)).toThrow(
+    expect.objectContaining({ code: "EINVAL" }),
+  );

-  expect(() => file.writer({ fd: "not-a-number" } as any)).toThrow();
+  expect(() => file.writer({ fd: "not-a-number" } as any)).toThrow(
+    expect.objectContaining({ code: "EBADF" }),
+  );
📝 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
expect(() => file.writer({ path: Int32Array } as any)).toThrow();
});
it("writer() throws instead of crashing when options has an invalid fd", () => {
const file = Bun.file(join(tmpdirSync(), "writer-invalid-fd.txt"));
expect(() => file.writer({ fd: "not-a-number" } as any)).toThrow();
expect(() => file.writer({ path: Int32Array } as any)).toThrow(
expect.objectContaining({ code: "EINVAL" }),
);
});
it("writer() throws instead of crashing when options has an invalid fd", () => {
const file = Bun.file(join(tmpdirSync(), "writer-invalid-fd.txt"));
expect(() => file.writer({ fd: "not-a-number" } as any)).toThrow(
expect.objectContaining({ code: "EBADF" }),
);
🤖 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 274 - 279, Replace the broad
expect(...).toThrow() checks in the two tests that call file.writer(...) with
assertions that capture the thrown error and assert its error code equals the
specific expected errno; specifically, update the test that calls file.writer({
path: Int32Array } as any) to capture the thrown error from file.writer and
assert error.code === 'ERR_INVALID_ARG_TYPE' (or the correct errno used by the
validation), and likewise update the test that calls file.writer({ fd:
"not-a-number" } as any) to capture the thrown error and assert error.code ===
'ERR_INVALID_ARG_TYPE' (or the correct errno for invalid fd); use the
file.writer invocation sites as the locations to change.

});

Check failure on line 280 in test/js/bun/util/filesink.test.ts

View check run for this annotation

Claude / Claude Code Review

New writer() option-validation tests will fail on Windows 

These two new tests will fail on Windows CI: `getWriter` has an `if (Environment.isWindows)` block (Blob.zig:2890-2952) that opens the blob's own path and returns at line 2951 without ever reading `arguments[0]` or calling `fromJSWithTag`, so on Windows `file.writer({ path: Int32Array })` / `file.writer({ fd: "not-a-number" })` succeed and return a valid FileSink instead of throwing. Either gate these with `it.skipIf(isWindows)` or move the option validation above the Windows early-return.
Comment on lines +272 to +280

@claude claude Bot May 4, 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.

🔴 These two new tests will fail on Windows CI: getWriter has an if (Environment.isWindows) block (Blob.zig:2890-2952) that opens the blob's own path and returns at line 2951 without ever reading arguments[0] or calling fromJSWithTag, so on Windows file.writer({ path: Int32Array }) / file.writer({ fd: "not-a-number" }) succeed and return a valid FileSink instead of throwing. Either gate these with it.skipIf(isWindows) or move the option validation above the Windows early-return.

Extended reasoning...

What the bug is

The two new regression tests assert that Bun.file(path).writer({ path: Int32Array }) and Bun.file(path).writer({ fd: "not-a-number" }) throw. The fix that makes them throw lives in the POSIX-only portion of getWriter (the new switch (stream_start) at Blob.zig:2978-2990). On Windows, execution never reaches that code, so the calls succeed and expect(() => ...).toThrow() fails.

Code path that triggers it

In src/runtime/webcore/Blob.zig, getWriter contains:

if (Environment.isWindows) {
    const pathlike = store.data.file.pathlike;   // <-- the BLOB's own path/fd
    ...
    const fd = ... bun.sys.open(pathlike.path.sliceZ(&file_path), O_WRONLY|O_CREAT|O_NONBLOCK, ...) ...;
    var sink = jsc.WebCore.FileSink.init(fd, ...);
    ...
    return sink.toJS(globalThis);                 // line 2951 — early return
}
// POSIX-only from here on
...
if (arguments.len > 0 and arguments.ptr[0].isObject()) {
    stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(...);
    switch (stream_start) { ... .err => ... }     // <-- the new fix
}

The Windows branch reads only store.data.file.pathlike (i.e. the path passed to Bun.file(...)). It never references arguments and never calls fromJSWithTag, so the { path: Int32Array } / { fd: "not-a-number" } option object is completely ignored on Windows.

Why existing code doesn't prevent it

There is no shared options-parsing step before the platform split — the Windows path was written as a self-contained implementation that returns before the POSIX options handling. Nothing in the Windows block inspects arguments[0], so there is nothing that could reject the bad options.

Step-by-step proof on Windows

  1. Test calls Bun.file(join(tmpdirSync(), "writer-invalid-path.txt")) — tmpdirSync() creates a real, existing directory.
  2. .writer({ path: Int32Array }) enters getWriter. store is non-null, not S3, so we hit if (Environment.isWindows).
  3. pathlike is .path pointing at <tmpdir>/writer-invalid-path.txt. bun.sys.open is called with O_WRONLY | O_CREAT, the parent dir exists, so it returns .result with a valid fd.
  4. A FileSink is created, sink.writer.start(fd, true) succeeds, and line 2951 returns sink.toJS(globalThis) — no exception thrown.
  5. Back in the test, expect(() => file.writer({ path: Int32Array } as any)).toThrow() observes no throw → assertion failure.
  6. The same applies to the { fd: "not-a-number" } test for identical reasons.

Impact

Both new tests will fail on Windows CI, blocking the PR (or breaking main if merged). The underlying crash fix is correct for POSIX; only the test gating is wrong.

How to fix

Minimal fix: gate the two new tests with it.skipIf(isWindows)(...) (the file already imports isWindows from harness):

it.skipIf(isWindows)("writer() throws instead of crashing when options has a non-string path", () => { ... });
it.skipIf(isWindows)("writer() throws instead of crashing when options has an invalid fd", () => { ... });

A fuller fix would be to validate arguments[0] (call fromJSWithTag and handle .err) before the if (Environment.isWindows) block so Windows also rejects invalid options, but that's a behavior change beyond the crash fix this PR targets.

Loading