Skip to content

Blob.writer(): don't access .FileSink when Start.fromJSWithTag returns .err #30254

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

Blob.writer(): don't access .FileSink when Start.fromJSWithTag returns .err #30254

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: 7 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,13 @@ pub fn getWriter(

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 == .err) {
sink.deref();
return globalThis.throwValue(try stream_start.err.toJS(globalThis));
}
if (stream_start == .FileSink) {
stream_start.FileSink.input_path = input_path;
}
Comment on lines +2982 to +2984

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Release previous input_path before overwriting .FileSink.input_path

If options include a valid path, fromJSWithTag allocates it; this assignment overwrites that value without deinit, leaking that allocation.

Proposed fix 
         if (stream_start == .FileSink) {
+            stream_start.FileSink.input_path.deinit();
             stream_start.FileSink.input_path = input_path;
         }
📝 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
if (stream_start == .FileSink) {
stream_start.FileSink.input_path = input_path;
}
if (stream_start == .FileSink) {
stream_start.FileSink.input_path.deinit();
stream_start.FileSink.input_path = input_path;
}
🤖 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 `@src/runtime/webcore/Blob.zig` around lines 2982 - 2984, The assignment to
stream_start.FileSink.input_path in fromJSWithTag overwrites a previously
allocated value and leaks; before setting stream_start.FileSink.input_path =
input_path, check if stream_start == .FileSink and if the existing
stream_start.FileSink.input_path is non-null/initialized, release it using the
appropriate deinit/free for that type (e.g., call the allocator free or the
value's deinit method), then assign the new input_path; ensure you use the same
allocator/cleanup method used when the path was allocated to avoid double-free
issues.

}

switch (sink.start(stream_start)) {
Expand Down
7 changes: 7 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 @@ -219,6 +219,13 @@ if (isWindows) {
});
}

it.skipIf(isWindows)("writer() with invalid path/fd options throws instead of crashing", () => {
const file = path.join(tmpdirSync(), "test.txt");

@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 fixture instead of tmpdirSync() in this new test

Please switch this new temp-path setup to tempDir from harness for consistent cleanup semantics in test files.

Proposed refactor 
-  const file = path.join(tmpdirSync(), "test.txt");
+  using dir = tempDir("filesink-invalid-options");
+  const file = path.join(dir, "test.txt");

Also update imports:

-import { fileDescriptorLeakChecker, isPosix, isWindows, tmpdirSync } from "harness";
+import { fileDescriptorLeakChecker, isPosix, isWindows, tempDir, tmpdirSync } from "harness";

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
const file = path.join(tmpdirSync(), "test.txt");
using dir = tempDir("filesink-invalid-options");
const file = path.join(dir, "test.txt");
🤖 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 223, Replace the use of
tmpdirSync() when constructing the test path with the test harness tempDir
fixture: import tempDir from the test harness at top of the file, call tempDir()
within the test to get a temporary directory, and use path.join(tempDir(),
"test.txt") in place of path.join(tmpdirSync(), "test.txt"); ensure any existing
tmpdirSync or fs.mkdtempSync import/usages are removed so cleanup is handled by
the harness.

expect(() => Bun.file(file).writer({ path: 123 })).toThrow(expect.objectContaining({ code: "EINVAL" }));
expect(() => Bun.file(file).writer({ fd: "hello" })).toThrow(expect.objectContaining({ code: "EBADF" }));
expect(() => Bun.file(file).writer({ fd: 2 ** 53 })).toThrow(expect.objectContaining({ code: "EBADF" }));
});

// When a write to a pollable fd returns `.pending`, FileSink takes a
// `must_be_kept_alive_until_eof` ref on itself so it survives until the
// buffered data is drained. If the write later fails (e.g. EPIPE because the
Expand Down
Loading