Skip to content
72 changes: 42 additions & 30 deletions src/runtime/shell/states/Cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ pub struct SubprocExec {
pub(crate) child: *mut ShellSubprocess,
pub(crate) buffered_closed: BufferedIoClosed,
/// NodeId-arena backrefs so the legacy `&mut self` subprocess callbacks
/// (`buffered_output_close` / `on_exit`) can hand a [`Yield`] back to the
/// trampoline. The `Cmd` lives inside `interp.nodes`, so we stash the
/// indices and
/// return `Yield::Next(this_id)` for the caller (`PipeReader::run_yield`)
/// to drive.
/// (`buffered_input_close` / `buffered_output_close` / `on_exit`) can hand
/// a [`Yield`] back to the trampoline. The `Cmd` lives inside
/// `interp.nodes`, so we stash the indices and return
/// `Yield::Next(this_id)` for the caller (`PipeReader::run_yield` /
/// `ShellSubprocess::on_static_pipe_writer_done`) to drive.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) interp: *mut Interpreter,
pub(crate) this_id: NodeId,
}
Expand Down Expand Up @@ -939,11 +939,17 @@ impl Cmd {
}
}

/// Mark the subprocess's buffered stdin as closed.
pub(crate) fn buffered_input_close(&mut self) {
if let Exec::Subproc(sub) = &mut self.exec {
sub.buffered_closed.close_stdin();
}
/// Mark the subprocess's buffered stdin as closed. The stdin writer can be
/// the last of the three to close (the child exits without draining its
/// stdin, so the write only fails once the exit and the stdout/stderr EOFs
/// have been processed), in which case this close is what finishes the
/// command. A no-op once `deinit` has taken `exec`.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn buffered_input_close(&mut self) -> Yield {
let Exec::Subproc(sub) = &mut self.exec else {
return Yield::suspended();
};
sub.buffered_closed.close_stdin();
self.finish_if_done()
}

/// Mark the subprocess's buffered stdout/stderr as closed (flushing the
Expand All @@ -958,27 +964,33 @@ impl Cmd {
OutKind::Stdout => self.buffered_output_close_stdout(err),
OutKind::Stderr => self.buffered_output_close_stderr(err),
}
if self.has_finished() {
// Set `state = Done` and hand the Yield back to the caller
// (`PipeReader::run_yield`), which drives the trampoline with the
// `*mut Interpreter` it already holds, landing in `Cmd::next` →
// `CmdState::Done` → `interp.child_done(...)`.
self.state = CmdState::Done;
let (interp, this_id) = match &self.exec {
Exec::Subproc(sub) => (sub.interp, sub.this_id),
// Only the subprocess path calls this; builtin output goes
// through `Builtin::done` → `on_exec_done`.
_ => return Yield::suspended(),
};
// Same gate as `on_exit`: `exec.interp` stays null until the spawn
// returns, so a Yield run here would reach `Cmd::deinit` and free the
// `ShellSubprocess` still on the spawn frame. `transition_to_exec` resumes.
if interp.is_null() {
return Yield::suspended();
}
return Yield::Next(this_id);
self.finish_if_done()
}

/// Shared tail of the stdio close callbacks: once the exit code and every
/// piped stdio are in, set `state = Done` and hand a Yield back to the
/// caller (`PipeReader::finish_after_state_set` /
/// `ShellSubprocess::on_static_pipe_writer_done`), which drives the
/// trampoline, landing in `Cmd::next` → `CmdState::Done` →
/// `interp.child_done(...)`.
fn finish_if_done(&mut self) -> Yield {
if !self.has_finished() {
return Yield::suspended();
}
Yield::suspended()
self.state = CmdState::Done;
let (interp, this_id) = match &self.exec {
Exec::Subproc(sub) => (sub.interp, sub.this_id),
// Only the subprocess path calls this; builtin output goes
// through `Builtin::done` → `on_exec_done`.
Comment thread
robobun marked this conversation as resolved.
Outdated
_ => return Yield::suspended(),
};
// Same gate as `on_exit`: `exec.interp` stays null until the spawn
// returns, so a Yield run here would reach `Cmd::deinit` and free the
// `ShellSubprocess` still on the spawn frame. `transition_to_exec` resumes.
if interp.is_null() {
return Yield::suspended();
}
Yield::Next(this_id)
}

fn buffered_output_close_stdout(&mut self, err: Option<bun_sys::SystemError>) {
Expand Down
25 changes: 20 additions & 5 deletions src/runtime/shell/subproc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,16 +299,28 @@
unsafe { &mut *self.process }
}

/// Signal the owning `Cmd` that buffered stdin has closed and drive the
/// resulting `Yield`: when the exit and the stdout/stderr closes were
/// processed first, this is what finishes the command (mirrors
/// `PipeReader::finish_after_state_set` for the outputs). The trampoline
/// may reach `Cmd::deinit`, which frees `self`, so the caller must not
/// touch `self` afterwards and nothing here does either.
pub(crate) fn on_static_pipe_writer_done(&mut self) {
log!(
"Subproc(0x{:x}) onStaticPipeWriterDone(cmd={})",
std::ptr::from_mut(self) as usize,
self.cmd_parent.id
);
// SAFETY: cmd_parent backref resolves to the owning Cmd which outlives
// the subprocess (freed only in `Cmd::deinit` after all stdio closes).
unsafe { self.cmd_parent.cmd_mut() }.buffered_input_close();
let handle = self.cmd_parent;
// SAFETY: cmd_parent backref resolves to the owning Cmd, which outlives
// the subprocess (the subprocess is freed by that Cmd's `deinit`). The
// `&mut Cmd` ends before the trampoline below re-enters the arena.
let y = unsafe { handle.cmd_mut() }.buffered_input_close();
// `ParentRef: Deref<Target = Interpreter>`; the interpreter owns the
// Cmd that owns `self`, so it outlives this callback. `&mut self` is
// dead by NLL here — `run` may free `self` via `Cmd::deinit`.
y.run(&handle.interp);
}

Check failure on line 323 in src/runtime/shell/subproc.rs

View check run for this annotation

Claude / Claude Code Review

on_close_io holds &mut self across a trampoline that can free self

`on_static_pipe_writer_done` (and its caller `on_close_io`) take `&mut self`, and the trait shim at `StaticPipeWriterProcess::on_close_io` materialises that borrow via `(*this).on_close_io(kind)`; but the PR's new `y.run(&handle.interp)` can synchronously reach `Cmd::deinit` → `drop(heap::take(child))`, deallocating `*self` while both `&mut self` argument protectors are still on the stack — UB under Stacked/Tree Borrows. The "`&mut self` is dead by NLL here" comment conflates borrow-checker live
Comment thread
robobun marked this conversation as resolved.
Outdated

pub(crate) fn has_exited(&self) -> bool {
self.proc().has_exited()
Expand Down Expand Up @@ -403,16 +415,19 @@
self.stdin = Writable::Ignore;
}
Writable::Buffer(_) => {
self.on_static_pipe_writer_done();
// RefPtr has no Drop — move it out before reassigning so the
// create ref is actually released.
// create ref is actually released. This does not free the
// writer we are being called from: `start()`'s ref is only
// released after this callback returns.
if let Writable::Buffer(buffer) =
core::mem::replace(&mut self.stdin, Writable::Ignore)
{
// SAFETY: single-threaded; sole borrow of the payload.
unsafe { buffer_mut(&buffer) }.source.detach();
buffer.deref();
}
// Last: this may finish the Cmd, whose `deinit` frees `self`.
self.on_static_pipe_writer_done();
}
_ => {}
},
Expand Down
56 changes: 56 additions & 0 deletions test/js/bun/shell/bunshell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3109,6 +3109,62 @@ describe("stdin redirect from a zero-length buffer delivers EOF to the spawned c
});
});

describe("stdin redirect whose pipe is closed after the command has exited", () => {
// `< ${buf}` streams the bytes into the child over a pipe. A child that
// exits without reading its stdin leaves that write pending, and it only
// fails (and the stdin side of the command only closes) once the pipe's
// read end is gone. The child below hands its stdin to a helper process
// that outlives it without reading from it, so the exit code and the
// stdout/stderr EOFs are always processed first and the stdin close is
// what has to complete the command.
const SIZE = 1 << 20; // bigger than any pipe buffer: the write never drains
const childCode = `
const holder = Bun.spawn({
cmd: [process.execPath, "-e", "setTimeout(() => {}, 500)"],
stdin: "inherit",
stdout: "ignore",
stderr: "ignore",
detached: true,
});
holder.unref();
console.log("child stdout");
console.error("child stderr");
process.exitCode = 3;
`;
const cases: Array<[string, () => Buffer | Blob]> = [
["Buffer", () => Buffer.alloc(SIZE, "a")],
["Blob", () => new Blob([Buffer.alloc(SIZE, "a")])],
];

test.concurrent.each(cases)("%s", async (_name, input) => {
const result = await $`${BUN} -e ${childCode} < ${input()}`.quiet();
expect({
stdout: result.stdout.toString(),
stderr: result.stderr.toString(),
exitCode: result.exitCode,
}).toEqual({ stdout: "child stdout\n", stderr: "child stderr\n", exitCode: 3 });
});

test.concurrent("inside a pipeline", async () => {
const upper = "process.stdout.write((await Bun.stdin.text()).toUpperCase())";
const result = await $`${BUN} -e ${childCode} < ${Buffer.alloc(SIZE, "a")} | ${BUN} -e ${upper}`.quiet();
expect({
stdout: result.stdout.toString(),
stderr: result.stderr.toString(),
exitCode: result.exitCode,
}).toEqual({ stdout: "CHILD STDOUT\n", stderr: "child stderr\n", exitCode: 0 });
});

test.concurrent("as the left side of ||", async () => {
const result = await $`${BUN} -e ${childCode} < ${Buffer.alloc(SIZE, "a")} || echo failed`.quiet();
expect({
stdout: result.stdout.toString(),
stderr: result.stderr.toString(),
exitCode: result.exitCode,
}).toEqual({ stdout: "child stdout\nfailed\n", stderr: "child stderr\n", exitCode: 0 });
});
});

test("output redirect buffer for an external command stays attached until the command finishes", async () => {
// `> ${buf}` for an external (non-builtin) command stores the buffer and
// copies the child's stdout into it as chunks arrive across event-loop
Expand Down