Skip to content

shell: classify Bun.file(fd) as a descriptor redirect so builtins write to it - #46796

Open
robobun wants to merge 2 commits into
robobun/1d2c3231/shell-response-file-redirectfrom
robobun/0300749e/shell-builtin-file-blob-redirect
Open

robobun wants to merge 2 commits into
robobun/1d2c3231/shell-response-file-redirectfrom
robobun/0300749e/shell-builtin-file-blob-redirect

Conversation

@robobun

@robobun robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #46687 (the base branch). Merge #46307 first.

Problem

  • A builtin's echo x > ${Bun.file(fd)} or > ${Bun.stdout} exits 0 and writes nothing. The docs list Bun.file(fd) as an output target.
  • Builtin::init_redirections stores it as BuiltinIO::Blob, whose write arm only returns Ok(buf.len()) (src/runtime/shell/Builtin.rs:370).
  • The Response check at :684 is inverted. A new Blob body is lost. A Bun.file(fd) body throws Cannot redirect stdout/stderr to an immutable blob. Expected a file.

Fix

  • Interpreter::redirect_target classifies a Bun.file(fd), bare or as a Response body, as the new RedirectTarget::Fd. Both redirect setups use a duplicate of it.
  • BuiltinIO::Blob is removed. A builtin refuses any other Blob or body as an output, unread.
  • Verified: test/js/bun/shell/file-io.test.ts (42 new tests, 30 fail on the base) and 24 other shell suites.
  • Self-reviewed: 21 concerns raised, 19 addressed. Notes names the other two.

Background

Downsides

  • Without shell: write one pipe slot per POLLOUT to a blocking stdout or stderr (Linux) #46307, a builtin write larger than the free room of a blocking pipe waits in write(2) on the JS thread (Linux). Before: nothing written.
  • A builtin's > ${new Response(blob)} now throws. A descriptor that is not open gives exit 1.
  • A command that worked before runs at most 37 more instructions. A spawned Bun.file(fd) adds dup and close. .text grows 256 bytes.
Notes

How to land it

Repro (bun 1.4.2, main 6292c65, and the head of #46687):

import { $ } from "bun";
import { closeSync, openSync, readFileSync } from "node:fs";

const fd = openSync("out.txt", "w");
const builtin = await $`echo from-builtin > ${Bun.file(fd)}`;
const spawned = await $`sh -c 'echo from-spawned' > ${Bun.file(fd)}`;
closeSync(fd);
console.log(builtin.exitCode, spawned.exitCode, JSON.stringify(readFileSync("out.txt", "utf8")));
// before: 0 0 "from-spawned\n"
// after:  0 0 "from-builtin\nfrom-spawned\n"

Behaviour, before and after (Linux x64, release builds of cd5cbee and of this PR)

Redirect Route Before After
>, >>, 2>, &> to ${Bun.file(fd)} builtin nothing written, exit code unchanged written
the same to ${new Response(Bun.file(fd))} builtin throws the "immutable blob" error written, bodyUsed is true
the same to ${new Response(new Blob(["x"]))} builtin exit 0, output lost, bodyUsed is true throws the "immutable blob" error, body unread
< ${Bun.file(fd)}, the descriptor is a pipe builtin cat reads nothing reads the pipe
< ${Bun.file(fd)}, the descriptor is a regular file builtin cat on POSIX (experimental flag) exit 0, reads nothing exit 1, as cat < file and cat file there (#35337)
&> ${Bun.file(fd)} spawned stderr is lost both streams
&> ${new Response(Bun.file(fd))} spawned throws Blobs are immutable, and cannot be used for stdout/stderr both streams
> or < to ${Bun.file(fd)}, the descriptor is not open builtin exit 0 bun: Bad file descriptor: <fd>, exit 1
the same, bare or in a Response spawned bun: Bad file descriptor: /usr/bin/sh, exit 1, body used bun: Bad file descriptor: <fd>, exit 1, body unread

Every other cell of a spawned command is the same before and after. The bare > ${new Blob(["x"])} threw before and throws now.

What the shell does with the descriptor

  • The shell does not close it. A redirect reads or writes a duplicate (dup) and closes the duplicate when the command ends. The type JsFd gives out a duplicate and the number, not the descriptor.
  • The duplicate shares the file offset. > does not truncate and >> does not seek. A spawned command did the same before.
  • A builtin does not change the status flags. open_for_writing_impl would set O_NONBLOCK, and a duplicate shares that flag with the descriptor of the caller and with each process that holds it. A spawned command clears O_NONBLOCK on the descriptor, as each child stdio does since process.stdout/stderr: clear O_NONBLOCK for spawned children, don't drop console output on EAGAIN #43868. That is not new.
  • The duplicate comes first. A descriptor that is not open fails the command before the Response body is taken.
  • The writer has the settings of the stdout writer of the shell (setup_io_before_run, interpreter.rs:1262): shell_dup, then is_pollable. So > ${Bun.stdout} writes as a command with no redirect does.

Blocking pipe, in full

The target is the blocking write end of a FIFO, passed as Bun.file(wfd). The only reader is on the JS thread. The FIFO holds 65,536 bytes. Linux 6.1. The last column is a debug build of this branch merged with #46307.

Command Before After After, with #46307
builtin echo, 65,536 bytes exit 0, 0 bytes arrive exit 0, all bytes arrive exit 0, all bytes arrive
builtin echo, 65,537 bytes exit 0, 0 bytes arrive does not return exit 0, all bytes arrive
builtin echo, 200,001 bytes exit 0, 0 bytes arrive does not return exit 0, all bytes arrive
builtin echo, 1,000,001 bytes exit 0, 0 bytes arrive does not return exit 0, all bytes arrive
builtin echo, 200,001 bytes, write end opened with O_NONBLOCK exit 0, 0 bytes arrive exit 0, all bytes arrive exit 0, all bytes arrive
the same, after a spawned command got that descriptor exit 0, 0 bytes arrive does not return exit 0, all bytes arrive
spawned sh, 200,001 bytes exit 0, all bytes arrive exit 0, all bytes arrive not run

Left out on purpose, with the owner

Other open PRs on the same lines

Weighed and not done

Cost (release builds of cd5cbee and of this PR, Linux x64)

  • .text: 82,258,761 to 82,259,017 bytes (+256).
  • Type sizes are the same: BuiltinIO 64, BuiltinInput 56, Builtin 280, Cmd 176, RedirectTarget 40.
  • Instructions on the JS thread from the entry to the return of Cmd::transition_to_exec, callees included (gdb single-step, warm), for a builtin:
Command Before After
echo hi 1,101 1,100
echo hi > path 2,069 2,065
echo hi < path 1,418 1,416
echo hi > ${u8} 1,560 1,573
echo hi < ${blob} 1,446 1,483
echo hi < ${new Response("x")} 1,813 1,756
echo hi < ${Bun.file(fd)} 1,446 1,417
echo hi > ${Bun.file(fd)} 1,294 (writes nothing) 1,813
  • For a spawned command the count covers transition_to_exec, redirect_target and ShellSubprocess::ref only, because a single-step across vfork does not end. /bin/true 1,002 to 1,006. > path 1,107 to 1,116. > ${u8} 1,092 to 1,113. < ${blob} 1,155 to 1,188. > ${Bun.file(fd)} 1,140 to 1,148. < ${Bun.file(fd)} 1,113 to 1,123.
  • mimalloc calls per builtin command (malloc/free) are the same for the first seven rows: 6/7, 9/11, 8/9, 5/7, 7/8, 9/10, 7/8. > ${Bun.file(fd)} goes from 6/8 to 8/10.
  • Syscalls on the JS thread per builtin command are the same for each command that has no Bun.file(fd). < ${Bun.file(fd)} goes from 5 to 7 (dup, close). > ${Bun.file(fd)} goes from 5 to 10 (dup, fstat, ioctl, write, close, no F_SETFL).
  • For a spawned command the script counts descriptor syscalls only. They are the same for each command that has no Bun.file(fd). > goes from 11 to 13 and < from 14 to 16 (dup, close).
  • perf, valgrind and strace are not in the container. gdb scripts made the counts.

Tests

  • file-io.test.ts, block "redirect to a Bun.file(fd)": 42 tests and 4 todo rows. The debug build (ASAN) gives 69 pass, 4 todo, 0 fail for the file. On a release build of cd5cbee 30 of the 42 fail.
  • The 12 that pass on the base are controls: the spawned cells that worked before, cat < on POSIX, and the builtin refusal of a Response whose .body was read.
  • Each output cell runs for a builtin (echo, basename) and for a spawned twin that writes the same bytes, for >, >>, 2> and both streams of &>, with the bare file and with the Response.
  • One fixture counts /dev/fd entries over 20 rounds of nine redirects and compares the status flags of three descriptors with the flags at open (Linux). One fixture fills a FIFO and reads it on the JS thread.
  • One fixture writes 200,001 bytes to a FIFO write end with O_NONBLOCK and reads them on the JS thread. The same fixture with a blocking write end is a test.todo row that names shell: write one pipe slot per POLLOUT to a blocking stdout or stderr (Linux) #46307. With shell: write one pipe slot per POLLOUT to a blocking stdout or stderr (Linux) #46307 merged in, that row passes.
  • The rows for a descriptor that is not open are POSIX only. The Windows text was not measured.
  • Other suites, debug build: bunshell.test.ts (510 pass), yield, bunshell-instance, exec, throw, shell-worker-terminate-leak, bunshell-file, lazy, shelloutput, epipe, pipeline_stack, assignments-in-pipeline, shell-pipe-read-fault, shell-write-fault, shell-blocking-pipe, and the command suites cat, echo, basename, rm, mv, which, seq, yes, ls. One test in exec and one in ls go over their 5 s limit in a full debug run and pass alone.
  • BUN_JSC_validateExceptionChecks=1 passes on the new block. cargo clippy and rustfmt are clean. cargo check -p bun_runtime passes for x86_64-pc-windows-msvc and aarch64-apple-darwin.
  • Windows and macOS were not run locally.

Self-review


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/file-io.test.ts

…, for builtins too

`Interpreter::redirect_target` classifies a `Bun.file(fd)`, bare or as
the Blob body of a Response, as `RedirectTarget::Fd`. The builtin and
the spawned redirect setup both duplicate the descriptor, so a
descriptor that is not open fails the command before the body is taken.

A builtin had stored such a target as `BuiltinIO::Blob`, which accepted
every write and stored nothing. That kind is removed. What is left in
the object arm of a builtin is readable as stdin and refused as an
output, also when it is the body of a Response.
@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:49 PM PT - Oct 24th, 2026

✅ @robobun, your commit 57b5968ca927d9ae327d6196ae653ac90d76285a passed in Build #132199! 🎉


🧪   To try this PR locally:

bunx bun-pr 46796

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

bun-46796 --bun

@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: the fix is in this PR, #46796. It is stacked on #46687. Merge #46307 first (see Downsides in the PR body).

How it was reproduced (Linux x64, on bun 1.4.2, on main 6292c65 and on the head of #46687):

import { $ } from "bun";
import { closeSync, openSync, readFileSync } from "node:fs";

const fd = openSync("out.txt", "w");
await $`echo from-builtin > ${Bun.file(fd)}`;
await $`sh -c 'echo from-spawned' > ${Bun.file(fd)}`;
closeSync(fd);
console.log(JSON.stringify(readFileSync("out.txt", "utf8")));
// before: "from-spawned\n"
// after:  "from-builtin\nfrom-spawned\n"

Both commands exit 0. In test/js/bun/shell/file-io.test.ts, 30 of the 42 new tests fail on a build of the base branch. All 42 pass on this branch.

CI on 57b5968: no job failed. file-io.test.ts has no failure on any lane that ran: Linux, Windows x64 and aarch64, macOS aarch64. The annotated failures are seven other test files, and each passed when CI ran it again. Two macOS x64 test jobs wait for an agent.

@claude claude Bot left a comment

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.

Beyond the inline findings, I also checked ownership of the dup'd descriptor on every path in both redirect setups: the Fd arm dups before consume_body, and the dup is handed straight to CowFd (closed in deref) or to IOReader/IOWriter (which own and close it), with the only early returns coming before the dup, so no descriptor leaks on the EBADF or ambiguous-redirect paths. The ruled-out wrong-direction Bun.stdin/Bun.stdout case and the dup-lands-on-a-closed-stdio-number case were examined too and are pre-existing behaviour of the spawned path, not introduced here.

Extended reasoning...

The change touches the shell interpreter's redirect classifier (src/runtime/shell/interpreter.rs), the builtin redirect setup (src/runtime/shell/Builtin.rs), and the spawned-command redirect setup (src/runtime/shell/states/Cmd.rs), adding a RedirectTarget::Fd variant that dups a JS-owned descriptor, removing the no-op BuiltinIO::Blob output arm, and adding a 41-case test matrix. It touches no auth, crypto or injection surface; the sensitive surface is fd ownership and blocking I/O on the JS thread. Two confirmed findings (a JS-thread hang on a blocking pipe with more than the free room, and a builtin cat < Bun.file(fd) on a regular file now exiting 1) already mark this for human review, so this note only records the fd-ownership audit that came back clean.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/runtime/shell/Builtin.rs
Comment thread src/runtime/shell/Builtin.rs
A nonblocking descriptor takes the whole write while a reader on the
same thread drains the pipe. The blocking descriptor is a todo row:
on Linux that write waits in write(2), and the one-slot write of
#46307 ends the wait.
@robobun

robobun commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Replies are on the two review threads. In short:

@claude claude Bot left a comment

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.

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

  • 🔴 src/runtime/shell/Builtin.rs:605 — A builtin writing more than a blocking pipe's free room to a Bun.file(fd) can hang the JS thread for good when the only…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

@robobun

robobun commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

I resolved the thread at src/runtime/shell/Builtin.rs:605, so that the review does not count it again. The finding is correct, and this PR does not fix it: the bound on a write to a blocking pipe is #46307, in PosixPipeWriter::try_write. This branch merged with #46307 passes the probes, and the thread has the numbers. The PR body names the limit in its first Downsides line and says to merge #46307 first.

This branch has not been deployed

No deployments
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.

1 participant