Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions src/runtime/shell/Builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ pub struct Builtin {
pub stdin: BuiltinInput,
pub stdout: BuiltinIO,
pub stderr: BuiltinIO,
/// First error latched by [`Builtin::write_no_io`]; [`Builtin::done`]
/// folds it into the exit code so a builtin whose only failure is a write
/// error (`> ${buf}` too small) does not exit 0.
pub write_err: Option<bun_sys::Error>,
/// Scratch for `fmt_error_arena`. One outstanding error string at a time.
pub err_buf: Vec<u8>,
pub impl_: Impl,
Expand Down Expand Up @@ -382,16 +386,18 @@ impl BuiltinIO {
// stored cursor is u32.
let idx = *i as usize;
let total = arraybuf.array_buffer.byte_len as usize;
if idx >= total {
let write_len = total.saturating_sub(idx).min(buf.len());
if write_len > 0 {
let dst = &mut arraybuf.slice_mut()[idx..idx + write_len];
dst.copy_from_slice(&buf[..write_len]);
*i = i.saturating_add(write_len as u32);
}
if write_len < buf.len() {
return Err(bun_sys::Error::from_code(
bun_sys::E::ENOSPC,
bun_sys::Tag::write,
));
Comment thread
robobun marked this conversation as resolved.
}
let write_len = (total - idx).min(buf.len());
let dst = &mut arraybuf.slice_mut()[idx..idx + write_len];
dst.copy_from_slice(&buf[..write_len]);
*i = i.saturating_add(write_len as u32);
Ok(write_len)
}
BuiltinIO::Blob(_) | BuiltinIO::Ignore => Ok(buf.len()),
Expand Down Expand Up @@ -522,6 +528,7 @@ impl Builtin {
stdin,
stdout,
stderr,
write_err: None,
err_buf: Vec::new(),
impl_: Self::make_impl(kind),
}));
Expand Down Expand Up @@ -845,6 +852,33 @@ impl Builtin {

/// Finish the builtin with `exit_code` and signal the owning Cmd.
pub fn done(interp: &Interpreter, cmd: NodeId, exit_code: ExitCode) -> Yield {
// A `write_no_io` error that the builtin itself did not handle
// (discarded `let _ = write_no_io(...)`) fails the command here,
// mirroring the async `on_io_writer_chunk` error path.
let exit_code = match (exit_code, Self::of_mut(interp, cmd).write_err.take()) {
(0, Some(e)) => {
let msg = Self::task_error_to_string(interp, cmd, Self::kind_of(interp, cmd), &e)
.to_vec();
match &Self::of(interp, cmd).stderr {
// An fd stderr would need an async enqueue, but per-builtin
// `on_io_writer_chunk` state machines have already advanced
// past the point that can handle a completion. Tee the
// message into the JS-side capture buffer so `r.stderr`
// carries the diagnostic; the terminal write is skipped.
BuiltinIO::Fd(fd) => {
// SAFETY: see `OutFd::captured_mut`.
if let Some(buf) = unsafe { fd.captured_mut() } {
buf.extend_from_slice(&msg);
}
}
_ => {
let _ = Self::write_no_io(interp, cmd, IoKind::Stderr, &msg);
}
}
1
}
(code, _) => code,
};
// Output is written through immediately in `write_no_io`, so there
// is nothing to flush here.
Cmd::on_exec_done(interp, cmd, exit_code)
Expand Down Expand Up @@ -887,7 +921,10 @@ impl Builtin {
/// Write `buf` to stdout/stderr without going through IOWriter (the
/// stream is a captured buffer / arraybuffer / blob / /dev/null).
///
/// Returns `Err(ENOSPC)` when an ArrayBuffer target is already full.
/// Returns `Err(ENOSPC)` when an ArrayBuffer target cannot hold all of
/// `buf` (after writing what fits); the error is also latched on the
/// [`Builtin`] so [`Builtin::done`] fails the command even when the
/// caller discards the result.
/// **WARNING**: caller must have checked `needs_io() == None` first.
pub fn write_no_io(
interp: &Interpreter,
Expand All @@ -911,7 +948,13 @@ impl Builtin {
IoKind::Stdin => return Ok(0),
};
// SAFETY: `shell` is `cmd_node.base.shell`, live for the Cmd's lifetime.
unsafe { out.write_no_io_to(shell, buf) }
let r = unsafe { out.write_no_io_to(shell, buf) };
if let Err(e) = &r {
if me.write_err.is_none() {
me.write_err = Some(e.clone());
}
}
r
}

/// Shell exec env of the owning Cmd.
Expand Down
32 changes: 28 additions & 4 deletions src/runtime/shell/states/Cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ impl Cmd {
pub struct SubprocExec {
pub child: *mut ShellSubprocess,
pub buffered_closed: BufferedIoClosed,
/// Set by [`Cmd::buffered_output_close`] when a `> ${buf}` target could
/// not hold the subprocess's full output. Folded into the exit code at
/// [`CmdState::Done`] (only when the process itself exited 0) so the
/// subprocess's own nonzero exit is never replaced.
pub redirect_overflow: bool,
/// 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
Expand Down Expand Up @@ -282,8 +287,14 @@ impl Cmd {
}
CmdState::WaitingWriteErr => return Yield::suspended(),
CmdState::Done => {
let exit = interp.as_cmd(this).exit_code.unwrap_or(0);
let parent = interp.as_cmd(this).base.parent;
let me = interp.as_cmd(this);
let exit = match (me.exit_code.unwrap_or(0), &me.exec) {
(0, Exec::Subproc(sub)) if sub.redirect_overflow => {
bun_sys::E::ENOSPC as ExitCode
}
(code, _) => code,
};
let parent = me.base.parent;
return interp.child_done(parent, this, exit);
}
}
Expand Down Expand Up @@ -570,6 +581,7 @@ impl Cmd {
interp.as_cmd_mut(this).exec = Exec::Subproc(Box::new(SubprocExec {
child: core::ptr::null_mut(),
buffered_closed,
redirect_overflow: false,
interp: core::ptr::null_mut(),
this_id: this,
}));
Expand Down Expand Up @@ -934,7 +946,13 @@ impl Cmd {
&mut self,
kind: OutKind,
err: Option<bun_sys::SystemError>,
overflow: bool,
) -> Yield {
if overflow {
if let Exec::Subproc(sub) = &mut self.exec {
sub.redirect_overflow = true;
}
}
match kind {
OutKind::Stdout => self.buffered_output_close_stdout(err),
OutKind::Stderr => self.buffered_output_close_stderr(err),
Expand Down Expand Up @@ -966,7 +984,10 @@ impl Cmd {
debug_assert!(matches!(self.exec, Exec::Subproc(_)));
log!("cmd close buffered stdout");
if let Some(e) = err {
self.exit_code = Some(e.errno.unsigned_abs() as ExitCode);
if self.exit_code.unwrap_or(0) == 0 {
self.exit_code = Some(e.errno.unsigned_abs() as ExitCode);
}
e.deref();
Comment thread
robobun marked this conversation as resolved.
}
let redirect = self.ast_node().redirect;
let Exec::Subproc(sub) = &mut self.exec else {
Expand Down Expand Up @@ -1002,7 +1023,10 @@ impl Cmd {
debug_assert!(matches!(self.exec, Exec::Subproc(_)));
log!("cmd close buffered stderr");
if let Some(e) = err {
self.exit_code = Some(e.errno.unsigned_abs() as ExitCode);
if self.exit_code.unwrap_or(0) == 0 {
self.exit_code = Some(e.errno.unsigned_abs() as ExitCode);
}
e.deref();
}
let redirect = self.ast_node().redirect;
let Exec::Subproc(sub) = &mut self.exec else {
Expand Down
35 changes: 24 additions & 11 deletions src/runtime/shell/subproc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,7 @@ impl Readable {
.buffered_output = BufferedOutput::ArrayBuffer {
buf: core::mem::take(array_buffer),
i: 0,
overflow: false,
};
Readable::Pipe(pipe)
}
Expand Down Expand Up @@ -1258,6 +1259,7 @@ impl Readable {
.buffered_output = BufferedOutput::ArrayBuffer {
buf: core::mem::take(array_buffer),
i: 0,
overflow: false,
};
Readable::Pipe(pipe)
}
Expand Down Expand Up @@ -1444,6 +1446,10 @@ pub enum BufferedOutput {
ArrayBuffer {
buf: jsc::array_buffer::ArrayBufferStrong,
i: u32,
/// Set by [`Self::append`] when the target buffer cannot hold the
/// full chunk; read by [`PipeReader::try_signal_done_to_cmd`] to fail
/// the command instead of exiting 0 with truncated output.
overflow: bool,
},
}

Expand Down Expand Up @@ -1474,19 +1480,25 @@ impl BufferedOutput {
BufferedOutput::Bytelist(b) => {
let _ = b.append_slice(bytes); // OOM/capacity: fire-and-forget
}
BufferedOutput::ArrayBuffer { buf, i } => {
BufferedOutput::ArrayBuffer { buf, i, overflow } => {
let array_buf_slice = buf.slice_mut();
let idx = *i as usize;
// TODO: We should probably throw error here?
if idx >= array_buf_slice.len() {
return;
let length = array_buf_slice.len().saturating_sub(idx).min(bytes.len());
if length > 0 {
array_buf_slice[idx..idx + length].copy_from_slice(&bytes[..length]);
*i += u32::try_from(length).expect("int cast");
}
if length < bytes.len() {
*overflow = true;
}
let length = (array_buf_slice.len() - idx).min(bytes.len());
array_buf_slice[idx..idx + length].copy_from_slice(&bytes[..length]);
*i += u32::try_from(length).expect("int cast");
}
}
}

#[inline]
pub fn overflowed(&self) -> bool {
matches!(self, BufferedOutput::ArrayBuffer { overflow: true, .. })
}
}

impl Drop for BufferedOutput {
Expand Down Expand Up @@ -1931,7 +1943,7 @@ impl PipeReader {
// every PipeReader has signalled done. `cmd_mut` resolves through
// the node arena (see `CmdHandle`).
let cmd = unsafe { (*proc).cmd_parent.cmd_mut() };
let e: Option<SystemError> = {
let (e, overflow): (Option<SystemError>, bool) = {
// SAFETY: caller contract — `&mut *this` for the field rewrites;
// ends at the closing brace, *before* the `cmd` call below.
let me = unsafe { &mut *this };
Expand All @@ -1955,16 +1967,17 @@ impl PipeReader {
// `bun_sys::SystemError` isn't ref-counted nor `Clone`.
// Move it out (the only reader of
// `state.Err` after this point is `Drop`, which tolerates `None`).
if let PipeReaderState::Err(slot) = &mut me.state {
let err = if let PipeReaderState::Err(slot) = &mut me.state {
slot.take().map(|b| *b)
} else {
None
}
};
(err, me.buffered_output.overflowed())
};
// No `&`/`&mut PipeReader` is live here; `buffered_output_close`
// is free to deref the sibling `Arc<PipeReader>` in
// `Readable::Pipe` for `pipe.slice()` / `close_io`.
return cmd.buffered_output_close(out_type, e);
return cmd.buffered_output_close(out_type, e, overflow);
}
Yield::Suspended
}
Expand Down
111 changes: 111 additions & 0 deletions test/js/bun/shell/bunshell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { afterAll, beforeAll, describe, expect, it, test } from "bun:test";
import { chmodSync, mkdirSync } from "fs";
import { mkdir, rm, stat } from "fs/promises";
import { bunExe, isPosix, isWindows, runWithErrorPromise, tempDir, tempDirWithFiles, tmpdirSync } from "harness";
import os from "node:os";
import { join, sep } from "path";
import { createTestBuilder, sortedShellOutput } from "./util";
const TestBuilder = createTestBuilder(import.meta.path);
Expand Down Expand Up @@ -463,6 +464,116 @@ describe("bunshell", () => {
expect(new TextDecoder().decode(buffer.slice(0, sentinel))).toEqual(await thisFile.text());
});

describe("redirect into a too-small Buffer fails the command", () => {
// A `> ${buf}` target that cannot hold the full output must surface as a
// nonzero exit code (like `> /dev/full`), not exit 0 with the data
// silently cut short.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
test("builtin", async () => {
const buf = Buffer.alloc(4);
const r = await $`echo hello world > ${buf}`.quiet();
expect({
written: buf.toString("latin1"),
stderr: r.stderr.toString(),
exitCode: r.exitCode,
}).toEqual({
written: "hell",
stderr: "echo: No space left on device\n",
exitCode: 1,
});
});

test("builtin: `|| rhs` runs", async () => {
const buf = Buffer.alloc(4);
const r = await $`echo hello world > ${buf} || echo write_failed`.quiet();
expect({ stdout: r.stdout.toString(), exitCode: r.exitCode }).toEqual({
stdout: "write_failed\n",
exitCode: 0,
});
});

test("builtin: without .quiet() the diagnostic still reaches r.stderr", async () => {
// Default stderr is an fd teeing to the terminal + `r.stderr`; `done()`
// tees the message into the capture buffer synchronously.
const buf = Buffer.alloc(4);
const r = await $`echo hello world > ${buf}`;
expect({ stderr: r.stderr.toString(), exitCode: r.exitCode }).toEqual({
stderr: "echo: No space left on device\n",
exitCode: 1,
});
});

test("builtin: exact fit succeeds", async () => {
const buf = Buffer.alloc(12);
const r = await $`echo hello world > ${buf}`.quiet();
expect({
written: buf.toString("latin1"),
stderr: r.stderr.toString(),
exitCode: r.exitCode,
}).toEqual({ written: "hello world\n", stderr: "", exitCode: 0 });
});

test("builtin: Uint8Array view with nonzero offset", async () => {
const backing = new Uint8Array(24).fill(0x2e);
const view = new Uint8Array(backing.buffer, 8, 4);
const r = await $`echo hello world > ${view}`.quiet();
expect({
backing: Buffer.from(backing).toString("latin1"),
stderr: r.stderr.toString(),
exitCode: r.exitCode,
}).toEqual({
// Only the 4-byte view window is written; bytes on either side are
// untouched.
backing: "........hell............",
stderr: "echo: No space left on device\n",
exitCode: 1,
});
});

test("builtin: Uint8Array view with nonzero offset, exact fit", async () => {
const backing = new Uint8Array(24).fill(0x2e);
const view = new Uint8Array(backing.buffer, 8, 12);
const r = await $`echo hello world > ${view}`.quiet();
expect({
backing: Buffer.from(backing).toString("latin1"),
stderr: r.stderr.toString(),
exitCode: r.exitCode,
}).toEqual({ backing: "........hello world\n....", stderr: "", exitCode: 0 });
});

test("subprocess", async () => {
const buf = Buffer.alloc(4);
const r = await $`${BUN} -e ${'process.stdout.write("hello world")'} > ${buf}`.quiet();
expect({
written: buf.toString("latin1"),
stderr: r.stderr.toString(),
exitCode: r.exitCode,
}).toEqual({
written: "hell",
stderr: "",
exitCode: os.constants.errno.ENOSPC,
});
});
Comment thread
robobun marked this conversation as resolved.

test("subprocess: `|| rhs` runs", async () => {
const buf = Buffer.alloc(4);
const r = await $`${BUN} -e ${'process.stdout.write("hello world")'} > ${buf} || echo write_failed`.quiet();
expect({ written: buf.toString("latin1"), stdout: r.stdout.toString(), exitCode: r.exitCode }).toEqual({
written: "hell",
stdout: "write_failed\n",
exitCode: 0,
});
});

test("subprocess: own nonzero exit wins over overflow", async () => {
const buf = Buffer.alloc(4);
const r = await $`${BUN} -e ${'process.stdout.write("hello world"); process.exitCode = 5'} > ${buf}`.quiet();
expect({ written: buf.toString("latin1"), exitCode: r.exitCode }).toEqual({
written: "hell",
exitCode: 5,
});
});
});

test("redirect Buffer", async () => {
const buffer = Buffer.alloc(1 << 20);
const result = await $`cat ${import.meta.path} > ${buffer}`;
Expand Down
Loading
Loading