Fix crash in Blob.writer() when options contain invalid path or fd - #30253
Fix crash in Blob.writer() when options contain invalid path or fd#30253robobun wants to merge 1 commit into
Conversation
Blob.writer() would crash with a union field access panic when the options object contained a `path` property that was not a string or an `fd` property that was not a valid file descriptor. In these cases Start.fromJSWithTag returns the .err variant, but getWriter unconditionally accessed .FileSink on the result.
|
Updated 11:45 AM PT - May 4th, 2026
❌ @robobun, your commit 0bd0065 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30253That installs a local version of the PR into your bun-30253 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
WalkthroughThe ChangesgetWriter Safety and Error Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 3/5 reviews remaining, refill in 13 minutes and 46 seconds. 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 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.
- Around line 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.
🪄 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: e889f097-f471-41ef-8be4-a77e8271171b
📒 Files selected for processing (2)
src/runtime/webcore/Blob.zigtest/js/bun/util/filesink.test.ts
| 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(); |
There was a problem hiding this comment.
🛠️ 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.
| 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(); |
There was a problem hiding this comment.
🧹 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.
| 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.
|
Duplicate of #28388. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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(); | ||
| }); |
There was a problem hiding this comment.
🔴 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
- Test calls
Bun.file(join(tmpdirSync(), "writer-invalid-path.txt"))—tmpdirSync()creates a real, existing directory. .writer({ path: Int32Array })entersgetWriter.storeis non-null, not S3, so we hitif (Environment.isWindows).pathlikeis.pathpointing at<tmpdir>/writer-invalid-path.txt.bun.sys.openis called withO_WRONLY | O_CREAT, the parent dir exists, so it returns.resultwith a valid fd.- A
FileSinkis created,sink.writer.start(fd, true)succeeds, and line 2951 returnssink.toJS(globalThis)— no exception thrown. - Back in the test,
expect(() => file.writer({ path: Int32Array } as any)).toThrow()observes no throw → assertion failure. - 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.
| @@ -2975,7 +2975,19 @@ pub fn getWriter( | |||
|
|
|||
| if (arguments.len > 0 and arguments.ptr[0].isObject()) { | |||
| stream_start = try jsc.WebCore.streams.Start.fromJSWithTag(globalThis, arguments[0], .FileSink); | |||
There was a problem hiding this comment.
🟡 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'); } });- Blob.zig:2954 —
sink = FileSink.init(...)→ heap object, refcount = 1,live_countincremented. - Blob.zig:2956–2968 —
input_pathis computed;defer input_path.deinit()registered (so the path itself does not leak). - Blob.zig:2976 —
arguments.len > 0and the argument is an object → enter the branch. - Blob.zig:2977 —
fromJSWithTagrunsvalue.fastGet(globalThis, .highWaterMark), which invokes the getter, which throws →error.JSErrorpropagates out offromJSWithTag. - The
tryat 2977 propagateserror.JSErrorout ofgetWriter. Theswitch(lines 2978–2990) is never reached, so the newsink.deref()at 2984 doesn't run. - Only the
defer input_path.deinit()from step 2 runs.sinkis 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.
1 participant
What does this PR do?
Fixes a crash in
Blob.prototype.writer()when the options object contains apathproperty that is not a string, or anfdproperty that is not a valid file descriptor.streams.Start.fromJSWithTag(.FileSink)returns the.errvariant of theStartunion in these cases, butgetWriterunconditionally accessed.FileSinkon the result. In debug/ASAN builds this triggered a safety-check panic; in release builds it was undefined behavior.The fix handles the
.errvariant by throwing a proper JS error (and cleaning up the allocated sink), and also frees theinput_pathreturned byfromJSWithTagbefore overwriting it with the blob's own path to avoid a small leak when a stringpathoption is passed.How did you verify your code works?
Added regression tests in
test/js/bun/util/filesink.test.tscovering both the invalidpathand invalidfdcases. All existing FileSink tests continue to pass.Found by Fuzzilli (fingerprint
0e27c6a3000551ff).