Fix FileSink.start() crash when called without path/fd on an open writer by robobun · Pull Request #30953 · oven-sh/bun

robobun · 2026-05-18T00:16:28Z

What

Fixes a debug assertion failure (fd != Fd::INVALID in src/sys/Error.rs) when calling .start() on a FileSink created by Bun.file(path).writer() with an options object that does not include a path or fd property.

const writer = Bun.file("/tmp/out.txt").writer();
writer.start({}); // panic: assertion failed: fd != Fd::INVALID

Why

Start::from_js_with_tag::<FileSink> returns Start::FileSink { input_path: Fd(Fd::INVALID), .. } when the options object has neither path nor fd. FileSink::start() then unconditionally called setup(), which called dup_with_flags(Fd::INVALID, 0). The fcntl fails with EBADF, and when building the error via .with_fd(Fd::INVALID) we hit the debug assertion.

In Blob::get_writer the invalid-fd placeholder is always overwritten with the real file path/fd before start() is called, but the JS-exposed .start() has no such override.

How

Add a match guard in FileSink::start() to skip setup() when the incoming input_path is the invalid-fd placeholder — the writer is already configured, so we only update done/started/signal state as before. Calls with a real path or fd still re-run setup().

Found by Fuzzilli.

When calling .start() on a FileSink created by Bun.file(path).writer() with an options object that contains neither a 'path' nor an 'fd' property (e.g. .start({}) or .start({ highWaterMark: N })), the options were converted to Start::FileSink with input_path = Fd::INVALID, which was then passed to setup() -> open_for_writing -> dup(Fd::INVALID), tripping a debug assertion in sys::Error::with_fd. Skip the setup() call when the resulting input_path is the invalid-fd placeholder so the existing file setup is preserved.

robobun · 2026-05-18T00:16:38Z

^{Updated 11:19 PM PT - May 17th, 2026}

✅ @robobun, your commit 42d8725d6b414f4674f46f45c3089061a2403a23 passed in Build #55618! 🎉

🧪 To try this PR locally:

bunx bun-pr 30953

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

bun-30953 --bun

coderabbitai · 2026-05-18T00:20:55Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b29e97a4-f34f-4dd8-b91d-6c2d45f0fa63

📥 Commits

Reviewing files that changed from the base of the PR and between 8994f8f and 42d8725.

📒 Files selected for processing (3)

src/runtime/webcore/Blob.rs
src/runtime/webcore/FileSink.rs
src/runtime/webcore/streams.rs

Walkthrough

This PR adds a guard to FileSink::start that skips setup when the file descriptor is invalid, proceeding directly to state initialization. A new test confirms this codepath does not crash and file writes still persist correctly when start is called without a valid descriptor.

Changes

FileSink invalid file descriptor handling

Layer / File(s)	Summary
Option alias and JS parsing updates `src/runtime/webcore/streams.rs`	`FileSinkOptions` is now a type alias to `crate::webcore::file_sink::Options`. JS StartTag parsing constructs `FileSinkOptions` with `..Default::default()` to fill remaining fields.
Invalid file descriptor guard and validation `src/runtime/webcore/FileSink.rs`, `test/js/bun/util/filesink.test.ts`	`FileSink::start` now skips `self.setup(&file)` when `file.input_path` is `Fd::INVALID`, resetting state and starting the signal directly. A test verifies calling `writer.start()` without providing `path`/`fd` does not throw and subsequent writes persist to the file.
Blob callsites: default-fill FileSink options `src/runtime/webcore/Blob.rs`	Non-Windows Blob codepaths constructing `streams::Start::FileSink(...)` now include `..Default::default()` so any additional `Options` fields are filled from `Default`.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main fix: preventing a crash in FileSink.start() when called without path/fd on an open writer.
Description check	✅ Passed	The description comprehensively covers all required sections: What (the bug), Why (root cause), and How (the fix), with code examples and context.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.}

Jarred-Sumner · 2026-05-18T00:20:57Z

+                    crate::webcore::PathOrFileDescriptor::Fd(Fd::INVALID)
+                ) =>
+            {
                // PORT NOTE: `streams::FileSinkOptions` mirrors `file_sink::Options`


They should use the same options type? Can you also change that?

Done in 42d8725 — streams::FileSinkOptions is now a type alias for file_sink::Options, so start() passes the options straight to setup() without the bridge.

coderabbitai

Actionable comments posted: 1

🤖 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`:
- Line 273: Replace the tmpdirSync() usage when constructing path with the
harness tempDir two-arg overload: instead of const path = join(tmpdirSync(),
"filesink-restart.txt"); call tempDir with two arguments to create an isolated
disposable directory (e.g., tempDir(<test/context identifier>,
"filesink-restart")) and use join(tempDir(...), "filesink-restart.txt"); update
the import/usage to reference tempDir and remove tmpdirSync.

🪄 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: ef800f5c-7845-4d9a-81b2-d64bda15cdf3

📥 Commits

Reviewing files that changed from the base of the PR and between 80a06a8 and 8994f8f.

📒 Files selected for processing (2)

src/runtime/webcore/FileSink.rs
test/js/bun/util/filesink.test.ts

claude

LGTM — small, well-tested guard to skip setup() when .start() is called without a path/fd, fixing a fuzzer-found debug assertion.

Extended reasoning...

Overview

This PR adds a single match guard in FileSink::start() (src/runtime/webcore/FileSink.rs) so that when the incoming streams::Start::FileSink carries the placeholder PathOrFileDescriptor::Fd(Fd::INVALID) — which Start::from_js_with_tag produces when the JS options object has neither path nor fd (streams.rs:281-284) — the setup() call is skipped and only the done/started/signal tail runs. A regression test in test/js/bun/util/filesink.test.ts exercises .start({}) and .start({ highWaterMark }) on an already-open writer and verifies it still writes correctly.

Security risks

None. No new inputs are accepted, no resources are opened, no privilege boundaries are touched. The change strictly narrows the set of inputs that reach setup()/open_for_writing; the skipped case was previously a guaranteed EBADF (release) or debug-assert panic. There is no fd/path injection surface here.

Level of scrutiny

Low-to-moderate. The diff is 5 lines of logic in one match arm plus a test. The root-cause analysis in the PR description is precise and verifiable against streams.rs and the Zig original (FileSink.zig:439-456, which has the same unguarded setup() call — the Rust port's stricter with_fd debug assertion is what surfaced this). The only behavior change for the non-crashing release path is that .start({}) on an open writer now succeeds (preserving the existing fd/poll) instead of returning EBADF, which is the intended semantics and what the new test asserts.

Other factors

Found by Fuzzilli; the fix is the minimal guard at the right layer (JS-exposed .start() is the only path that doesn't override the placeholder — Blob::get_writer always overwrites it).
The fall-through to _ => {} is the same arm already taken for non-FileSink Start variants, so no new code path is introduced.
Test covers both the crash case and that the writer remains functional afterward.
No outstanding reviewer comments; bug hunter found nothing.

Replace the duplicate FileSinkOptions struct in streams.rs with a type alias to file_sink::Options so FileSink::start() can pass the options straight through to setup() without field-by-field bridging.

Error.fd defaults to Fd::INVALID and to_system_error() / Display already treat it as 'no fd attached' via fd_unwrap_valid(). Calling with_fd(Fd::INVALID) is therefore a no-op, but the debug_assert! turned it into a hard crash in debug/fuzz builds whenever a syscall wrapper (dup, fcntl, close, read/write, ...) is reached with the invalid-fd sentinel — something user input can drive through several PathOrFileDescriptor paths. #30953 fixed one such path in FileSink::start; Fuzzilli keeps hitting others under REPRL state that doesn't reproduce standalone. Drop the assertion so these cases surface as the proper EBADF/JS error instead of a panic. Release behaviour is unchanged.

The #30953 guard short-circuits these args before setup() so this test is smoke coverage, not the direct with_fd regression guard (that's the Rust unit test in src/sys/Error.rs).

github-actions Bot added the claude label May 18, 2026

Jarred-Sumner reviewed May 18, 2026

View reviewed changes

coderabbitai Bot reviewed May 18, 2026

View reviewed changes

Comment thread test/js/bun/util/filesink.test.ts

claude Bot reviewed May 18, 2026

View reviewed changes

Unify streams::FileSinkOptions with file_sink::Options

42d8725

Replace the duplicate FileSinkOptions struct in streams.rs with a type alias to file_sink::Options so FileSink::start() can pass the options straight through to setup() without field-by-field bridging.

Jarred-Sumner merged commit 9ecb985 into main May 18, 2026
75 of 76 checks passed

Jarred-Sumner deleted the farm/75717719/filesink-start-invalid-fd branch May 18, 2026 01:59

coderabbitai Bot mentioned this pull request May 18, 2026

streams: truncate chunk size to i51/i52 before clamping #30995

Closed

MaurerAnton mentioned this pull request May 20, 2026

[tracking] Zig/C++ fork: Rust→Zig port progress #31136

Closed

robobun mentioned this pull request May 21, 2026

sys: drop with_fd debug assertion on Fd::INVALID #31165

Open

coderabbitai Bot mentioned this pull request Jun 25, 2026

Remove stale signal.is_dead() assertion in pipe_readable_stream_to_blob #32657

Merged

robobun mentioned this pull request Aug 25, 2026

Windows: FileSink.start({ path }) on a live writer: hand the writer a libuv fd and close the old source #40462

Open

Navigation Menu

Fix FileSink.start() crash when called without path/fd on an open writer - #30953

Fix FileSink.start() crash when called without path/fd on an open writer#30953
Jarred-Sumner merged 2 commits into
mainfrom
farm/75717719/filesink-start-invalid-fd

robobun commented May 18, 2026

Uh oh!

robobun commented May 18, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 18, 2026 •

edited

Loading

Uh oh!

Jarred-Sumner May 18, 2026

Uh oh!

robobun May 18, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented May 18, 2026

What

Why

How

Uh oh!

robobun commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Jarred-Sumner May 18, 2026

Choose a reason for hiding this comment

Uh oh!

robobun May 18, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented May 18, 2026 •

edited

Loading

coderabbitai Bot commented May 18, 2026 •

edited

Loading