Blob.writer(): don't access .FileSink when Start.fromJSWithTag returns .err - #30254
Blob.writer(): don't access .FileSink when Start.fromJSWithTag returns .err#30254robobun wants to merge 1 commit into
Conversation
WalkthroughThe PR fixes error handling in the non-Windows FileSink setup path within ChangesFileSink Error Handling & Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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 `@src/runtime/webcore/Blob.zig`:
- Around line 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.
In `@test/js/bun/util/filesink.test.ts`:
- 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.
🪄 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: 28371a33-2681-4c95-9208-25a2384a1579
📒 Files selected for processing (2)
src/runtime/webcore/Blob.zigtest/js/bun/util/filesink.test.ts
| if (stream_start == .FileSink) { | ||
| stream_start.FileSink.input_path = input_path; | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| } | ||
|
|
||
| it.skipIf(isWindows)("writer() with invalid path/fd options throws instead of crashing", () => { | ||
| const file = path.join(tmpdirSync(), "test.txt"); |
There was a problem hiding this comment.
🛠️ 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.
| 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.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
1 participant
What
Bun.file(path).writer({ path: <non-string> })or.writer({ fd: <non-integer> })would trip a debug-build safety check:Why
streams.Start.fromJSWithTag(..., .FileSink)returns aStartunion. When the options object has apaththat isn't a string, or anfdthat isn't a valid integer, it returns the.errvariant (EINVAL / EBADF).Blob.getWriterthen unconditionally wrote tostream_start.FileSink.input_path, which is the wrong active union field.Fix
Check the tag before accessing
.FileSink. When.erris returned, clean up the sink and throw the error to JS (matching howsink.start()errors are already handled just below).Found by Fuzzilli. Fingerprint:
5f85f9ffc48a209e