Skip to content

Fix crash in Blob.writer() when options parsing returns an error#28736

Open
robobun wants to merge 2 commits intomainfrom
farm/8bbb1b14/fix-blob-writer-err-union
Open

Fix crash in Blob.writer() when options parsing returns an error#28736
robobun wants to merge 2 commits intomainfrom
farm/8bbb1b14/fix-blob-writer-err-union

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented Apr 1, 2026

What

Blob.getWriter panics with "access of union field 'FileSink' while field 'err' is active" when the writer options object has an invalid path (non-string) or fd (non-integer).

Root cause

Start.fromJSWithTag can return .err (e.g. EINVAL for non-string path, EBADF for invalid fd), but the code at line 2811 unconditionally accesses stream_start.FileSink.input_path without checking which union variant is active.

Fix

Switch on the result of fromJSWithTag:

  • .FileSink → set input_path as before
  • .err → clean up the sink and throw the error to JS
  • other → reset to default FileSink options

Repro

const f = Bun.file("/dev/null");
f.writer({ path: 42 }); // panics instead of throwing

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, push a new commit or reopen this pull request to trigger a review.

@github-actions github-actions bot added the claude label Apr 1, 2026
@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 1, 2026

Updated 3:36 AM PT - Apr 1st, 2026

@autofix-ci[bot], your commit 15b9273 has 4 failures in Build #43088 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 28736

That installs a local version of the PR into your bun-28736 executable, so you can run:

bun-28736 --bun

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 1, 2026

Walkthrough

getWriter in Blob.zig now switches on Start.fromJSWithTag(): it updates input_path only for .FileSink, throws immediately for .err, and replaces other variants with a new .FileSink. Added a test verifying invalid writer option shapes throw.

Changes

Cohort / File(s) Summary
Writer parsing and error handling
src/bun.js/webcore/Blob.zig
Updated getWriter to branch on Start.fromJSWithTag() result: mutate input_path for .FileSink, convert-and-throw on .err, and construct a .FileSink for other variants.
Invalid options test
test/js/bun/util/filesink.test.ts
Extended test imports and added a subprocess test that ensures Bun.file(...).writer() throws for invalid option shapes ({ path: 42 }, { fd: "invalid" }).
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main fix: preventing a crash in Blob.writer() when options parsing returns an error.
Description check ✅ Passed The description provides all required sections with clear explanations of what the issue is, the root cause, the fix, and a reproduction case.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@km-anthropic
Copy link
Copy Markdown
Collaborator

@claude review

@robobun robobun force-pushed the farm/8bbb1b14/fix-blob-writer-err-union branch 4 times, most recently from c21a4dd to ce3ef2e Compare April 1, 2026 09:47
Blob.getWriter unconditionally accessed the .FileSink field of the
Start union after calling fromJSWithTag, but that function can return
.err (e.g. when path is not a string or fd is invalid). Accessing the
wrong union field causes a panic in debug builds.

Handle the .err case by throwing the error to JavaScript, and reset
to default FileSink options for any other unexpected variant.
@robobun robobun force-pushed the farm/8bbb1b14/fix-blob-writer-err-union branch from ce3ef2e to 37d1d02 Compare April 1, 2026 09:47
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bun.js/webcore/Blob.zig`:
- Around line 2811-2820: The platform-specific writer setup performs option
parsing/validation after switching on stream_start, causing invalid writer
options (e.g., { path: 42 } or { fd: "invalid" }) to be allowed on some
platforms; move the Start.fromJSWithTag / option parsing logic (the validation
that turns JS options into the .FileSink variant) out of the platform-specific
branches so that stream_start is validated/constructed (and any err handled via
the .err arm returning globalThis.throwValue(try err.toJS(globalThis))) before
the OS split, or duplicate that same validation in the Windows branch; ensure
the .err case for Start.fromJSWithTag (or the function that parses writer
options) is invoked and handled prior to assigning stream_start for FileSink so
Blob.writer() throws consistently across platforms.
🪄 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: 76b446ff-f2ad-40cc-88fd-159795b24d67

📥 Commits

Reviewing files that changed from the base of the PR and between 7afdea1 and 15b9273.

📒 Files selected for processing (2)
  • src/bun.js/webcore/Blob.zig
  • test/js/bun/util/filesink.test.ts

Comment on lines +2811 to +2820
switch (stream_start) {
.FileSink => |*fs| fs.input_path = input_path,
.err => |err| {
sink.deref();
return globalThis.throwValue(try err.toJS(globalThis));
},
else => {
stream_start = .{ .FileSink = .{ .input_path = input_path } };
},
}
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 | 🟠 Major

Validate writer options before the platform split.

This hunk fixes the Start.fromJSWithTag(..., .FileSink) error-union path, but the new regression test is platform-agnostic and exercises invalid { path: 42 } / { fd: "invalid" } calls through Bun.file(...).writer(...). Please hoist the option parsing/validation before the OS-specific writer setup, or mirror it in the Windows branch, so Blob.writer() throws consistently on all supported platforms. (github.com)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/webcore/Blob.zig` around lines 2811 - 2820, The platform-specific
writer setup performs option parsing/validation after switching on stream_start,
causing invalid writer options (e.g., { path: 42 } or { fd: "invalid" }) to be
allowed on some platforms; move the Start.fromJSWithTag / option parsing logic
(the validation that turns JS options into the .FileSink variant) out of the
platform-specific branches so that stream_start is validated/constructed (and
any err handled via the .err arm returning globalThis.throwValue(try
err.toJS(globalThis))) before the OS split, or duplicate that same validation in
the Windows branch; ensure the .err case for Start.fromJSWithTag (or the
function that parses writer options) is invoked and handled prior to assigning
stream_start for FileSink so Blob.writer() throws consistently across platforms.

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 1, 2026

The CI failures are darwin-only test infrastructure issues — all 13 non-darwin test steps pass (7 Linux configs including ASAN + 3 Windows configs). These same darwin failures affect all concurrent PRs (e.g. #43081, #43083). The fix and test are verified correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants