Skip to content

shell: fail the command when a > ${buf} redirect overflows the target Buffer - #34698

Open
robobun wants to merge 6 commits into
mainfrom
claude/farm/ada0ba20/shell-arraybuf-enospc
Open

shell: fail the command when a > ${buf} redirect overflows the target Buffer#34698
robobun wants to merge 6 commits into
mainfrom
claude/farm/ada0ba20/shell-arraybuf-enospc

shell: fail the command when a `> ${buf}` redirect overflows

2a0c3c8
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 19, 2026 in 25m 42s

Code review found 3 potential issues

Found 5 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 3
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/runtime/shell/Builtin.rs:878-885 Error message dropped when stderr is a real fd (non-.quiet() path)
🟡 Nit src/runtime/shell/subproc.rs:2226-2230 ENOSPC synthesis: clobbers subprocess's own nonzero exit + leaks message string
🟡 Nit src/runtime/shell/Builtin.rs:406-410 Stale doc comment on Builtin::write_no_io

Annotations

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

See this annotation in the file changed.

@claude 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;

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

See this annotation in the file changed.

@claude 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

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

See this annotation in the file changed.

@claude 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)."