diff --git a/src/runtime/shell/Builtin.rs b/src/runtime/shell/Builtin.rs index d6e36a49c81c..0616accf22ae 100644 --- a/src/runtime/shell/Builtin.rs +++ b/src/runtime/shell/Builtin.rs @@ -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, /// Scratch for `fmt_error_arena`. One outstanding error string at a time. pub err_buf: Vec, pub impl_: Impl, @@ -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, )); } - 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()), @@ -522,6 +528,7 @@ impl Builtin { stdin, stdout, stderr, + write_err: None, err_buf: Vec::new(), impl_: Self::make_impl(kind), })); @@ -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) @@ -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, @@ -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. diff --git a/src/runtime/shell/states/Cmd.rs b/src/runtime/shell/states/Cmd.rs index 2d9f5fc1ac6b..1328a130c79e 100644 --- a/src/runtime/shell/states/Cmd.rs +++ b/src/runtime/shell/states/Cmd.rs @@ -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 @@ -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); } } @@ -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, })); @@ -934,7 +946,13 @@ impl Cmd { &mut self, kind: OutKind, err: Option, + 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), @@ -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(); } let redirect = self.ast_node().redirect; let Exec::Subproc(sub) = &mut self.exec else { @@ -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 { diff --git a/src/runtime/shell/subproc.rs b/src/runtime/shell/subproc.rs index 499bc889b055..0153ddfd8034 100644 --- a/src/runtime/shell/subproc.rs +++ b/src/runtime/shell/subproc.rs @@ -1213,6 +1213,7 @@ impl Readable { .buffered_output = BufferedOutput::ArrayBuffer { buf: core::mem::take(array_buffer), i: 0, + overflow: false, }; Readable::Pipe(pipe) } @@ -1258,6 +1259,7 @@ impl Readable { .buffered_output = BufferedOutput::ArrayBuffer { buf: core::mem::take(array_buffer), i: 0, + overflow: false, }; Readable::Pipe(pipe) } @@ -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, }, } @@ -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 { @@ -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 = { + let (e, overflow): (Option, 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 }; @@ -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` 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 } diff --git a/test/js/bun/shell/bunshell.test.ts b/test/js/bun/shell/bunshell.test.ts index 24c152de773a..72ea276cbdc9 100644 --- a/test/js/bun/shell/bunshell.test.ts +++ b/test/js/bun/shell/bunshell.test.ts @@ -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); @@ -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. + 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, + }); + }); + + 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}`; diff --git a/test/js/bun/shell/leak.test.ts b/test/js/bun/shell/leak.test.ts index 14991a5bba53..f299a5056d49 100644 --- a/test/js/bun/shell/leak.test.ts +++ b/test/js/bun/shell/leak.test.ts @@ -170,9 +170,20 @@ describe.concurrent("fd leak", () => { memLeakTest(...args); }); - // Use text of this file so its big enough to cause a leak - memLeakTest("ArrayBuffer", () => TestBuilder.command`cat ${import.meta.filename} > ${new ArrayBuffer(128)}`, 100); - memLeakTest("Buffer", () => TestBuilder.command`cat ${import.meta.filename} > ${Buffer.alloc(128)}`, 100); + // Use text of this file so its big enough to cause a leak. Overflowing the + // redirect target now fails the command (ENOSPC), so ignore exit/stderr: + // this test is about the buffer's Strong ref being released, not the result. + memLeakTest( + "ArrayBuffer", + () => + TestBuilder.command`cat ${import.meta.filename} > ${new ArrayBuffer(128)}`.exitCode(() => {}).stderr(() => {}), + 100, + ); + memLeakTest( + "Buffer", + () => TestBuilder.command`cat ${import.meta.filename} > ${Buffer.alloc(128)}`.exitCode(() => {}).stderr(() => {}), + 100, + ); memLeakTest( "Blob_something", () =>