Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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 @@
/// 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 @@
// 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,
));

Check warning on line 410 in src/runtime/shell/Builtin.rs

View check run for this annotation

Claude / Claude Code Review

Stale doc comment on Builtin::write_no_io

The doc comment on `Builtin::write_no_io` still reads "Returns `Err(ENOSPC)` when an ArrayBuffer target is already full", but after this change `write_no_io_to` returns `Err(ENOSPC)` on *any* short write into an ArrayBuffer (after writing what fits), not just when it was full on entry. Suggest updating to something like "Returns `Err(ENOSPC)` when an ArrayBuffer target cannot hold all of `buf` (after writing what fits)."
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 @@
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 @@

/// 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
}

Check warning on line 885 in src/runtime/shell/Builtin.rs

View check run for this annotation

Claude / Claude Code Review

Error message dropped when stderr is a real fd (non-.quiet() path)

The `"<cmd>: No space left on device"` diagnostic is only emitted when `stderr.needs_io().is_none()`, so in the default (non-`.quiet()`) case — where stderr is `BuiltinIO::Fd` teeing to the terminal + captured buffer — the message is silently dropped and the command just exits 1 with nothing on stderr and nothing in `r.stderr`. All the added tests use `.quiet()`, so this path is uncovered. If skipping the async enqueue here is deliberate (to keep `done()` synchronous), it's worth a code comment;
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 @@
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 warning on line 2230 in src/runtime/shell/subproc.rs

View check run for this annotation

Claude / Claude Code Review

ENOSPC synthesis: clobbers subprocess's own nonzero exit + leaks message string

Two minor issues with this branch: (1) it clobbers the subprocess's own nonzero exit code — `buffered_output_close_stdout`/`_stderr` (Cmd.rs:988-990/1024-1026) unconditionally set `exit_code = errno`, so a subprocess that both overflows the buffer *and* exits nonzero has its real code replaced by 28 (asymmetric with the builtin path's `(code, _) => code` guard); and (2) `to_system_error()` heap-allocates a `WTFStringImpl` message that the sink never `.deref()`s, leaking it per overflow. Swapping
Comment thread
robobun marked this conversation as resolved.
Outdated
Comment thread
robobun marked this conversation as resolved.
Outdated
} else {
None
}
Expand Down
55 changes: 55 additions & 0 deletions test/js/bun/shell/bunshell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,61 @@ 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("subprocess", async () => {
const buf = Buffer.alloc(4);
const r = await $`${BUN} -e ${'process.stdout.write("hello world")'} > ${buf}`.quiet();
expect(buf.toString("latin1")).toBe("hell");
expect(r.exitCode).not.toBe(0);
});
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({ stdout: r.stdout.toString(), exitCode: r.exitCode }).toEqual({
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