Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
40 changes: 34 additions & 6 deletions src/runtime/shell/Builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ pub struct Builtin {
/// Set by `done()` and stashed by `write_failing_error` so the async
/// `on_io_writer_chunk` path can recover the intended exit code.
pub exit_code: Option<ExitCode>,
/// 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 @@ -393,16 +397,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 @@ -535,6 +541,7 @@ impl Builtin {
stdout,
stderr,
exit_code: None,
write_err: None,
err_buf: Vec::new(),
impl_: Self::make_impl(kind),
}));
Expand Down Expand Up @@ -863,6 +870,21 @@ 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)) => {
if Self::of(interp, cmd).stderr.needs_io().is_none() {
let msg =
Self::task_error_to_string(interp, cmd, Self::kind_of(interp, cmd), &e)
.to_vec();
let _ = Self::write_no_io(interp, cmd, IoKind::Stderr, &msg);
}
1
}
Comment thread
robobun marked this conversation as resolved.
Outdated
(code, _) => code,
};
Self::of_mut(interp, cmd).exit_code = Some(exit_code);
// Output is written through immediately in `write_no_io`, so there
// is nothing to flush here.
Expand Down Expand Up @@ -931,7 +953,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
31 changes: 24 additions & 7 deletions src/runtime/shell/subproc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1324,6 +1324,7 @@
.buffered_output = BufferedOutput::ArrayBuffer {
buf: core::mem::take(array_buffer),
i: 0,
overflow: false,
};
Readable::Pipe(pipe)
}
Expand Down Expand Up @@ -1369,6 +1370,7 @@
.buffered_output = BufferedOutput::ArrayBuffer {
buf: core::mem::take(array_buffer),
i: 0,
overflow: false,
};
Readable::Pipe(pipe)
}
Expand Down Expand Up @@ -1648,6 +1650,10 @@
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 @@ -1678,19 +1684,25 @@
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 @@ -2211,6 +2223,11 @@
// `state.Err` after this point is `Drop`, which tolerates `None`).
if let PipeReaderState::Err(slot) = &mut me.state {
slot.take().map(|b| *b)
} else if me.buffered_output.overflowed() {
Some(
bun_sys::Error::from_code(bun_sys::E::ENOSPC, bun_sys::Tag::write)
.to_system_error(),
)

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

View check run for this annotation

Claude / Claude Code Review

PR breaks leak.test.ts: existing > ${buf} overflow tests now fail exit-code assertion

This behavior change breaks `test/js/bun/shell/leak.test.ts`, which redirects `cat ${import.meta.filename}` / `echo ${import.meta.filename}` into 64/128-byte ArrayBuffer/Buffer targets via `TestBuilder.command` and asserts the default `expected_exit_code = 0` — CI (build #75749) confirms it failing on 🪟 2019 x64-baseline, 🪟 2019 x64, 🪟 11 aarch64, and 🐧 13 x64-asan for 69bdefac. This is separate from the `to_system_error()` leak on line 2230: 3 of the 4 red lanes are Windows, which takes the bui
Comment thread
robobun marked this conversation as resolved.
Outdated
Comment thread
robobun marked this conversation as resolved.
Outdated
} else {
None
}
Expand Down
91 changes: 91 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,96 @@ 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: 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("redirect Buffer", async () => {
const buffer = Buffer.alloc(1 << 20);
const result = await $`cat ${import.meta.path} > ${buffer}`;
Expand Down
Loading