Skip to content

shell: take the command-substitution capture and drop its NUL bytes #44752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −30
Open

shell: take the command-substitution capture and drop its NUL bytes #44752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 3 additions & 16 deletions src/runtime/shell/interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1869,7 +1869,7 @@ impl ShellExecEnv {
pub(crate) fn buffered_stdout(&mut self) -> *mut Vec<u8> {
// Return the raw `*mut` directly — no `&mut Vec<u8>` is materialised,
// so the `Bufio::Borrowed` aliasing concern (which forces
// [`buffered_stdout_mut`] to be `unsafe fn`) does not apply here. The
// [`buffered_stderr_mut`] to be `unsafe fn`) does not apply here. The
// dereference obligation is on whoever later writes through it.
match &mut self._buffered_stdout {
Bufio::Owned(o) => std::ptr::from_mut(o),
Expand All @@ -1884,7 +1884,7 @@ impl ShellExecEnv {
}
}

/// Mutably borrow the captured-stdout buffer (owned, or the parent env's
/// Mutably borrow the captured-stderr buffer (owned, or the parent env's
/// buffer for subshell/pipeline children — see `Bufio`).
///
/// # Safety
Expand All @@ -1896,23 +1896,10 @@ impl ShellExecEnv {
/// `unsafe fn`. The parent env strictly outlives this child (parents
/// `deinit` after children), so the pointer is never dangling.
#[inline]
pub(crate) unsafe fn buffered_stdout_mut(&mut self) -> &mut Vec<u8> {
match &mut self._buffered_stdout {
Bufio::Owned(o) => o,
// SAFETY: caller contract.
Bufio::Borrowed(b) => unsafe { &mut **b },
}
}

/// See [`buffered_stdout_mut`].
///
/// # Safety
/// See [`buffered_stdout_mut`].
#[inline]
pub(crate) unsafe fn buffered_stderr_mut(&mut self) -> &mut Vec<u8> {
match &mut self._buffered_stderr {
Bufio::Owned(o) => o,
// SAFETY: caller contract; see `buffered_stdout_mut`.
// SAFETY: caller contract.
Bufio::Borrowed(b) => unsafe { &mut **b },
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/shell/states/Cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ impl Cmd {
if out.bounds.is_empty() {
// An empty
// expansion that did *not* see a `""` literal pushes
// no arg at all — `$unset` vanishes, only `""` yields
// no arg at all — `$unset` vanishes, `""` (`has_quoted_empty`) yields
// an empty argv word.
if !out.buf.is_empty() || out.has_quoted_empty {
me.args.push(out.buf);
Expand Down
42 changes: 29 additions & 13 deletions src/runtime/shell/states/Expansion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

use crate::shell::ast;
use crate::shell::interpreter::{
EventLoopHandle, Interpreter, Node, NodeId, ShellExecEnv, ShellExecEnvKind, StateKind, log,
Bufio, EventLoopHandle, Interpreter, Node, NodeId, ShellExecEnv, ShellExecEnvKind, StateKind,
log,
};
use crate::shell::io::{IO, OutKind};
use crate::shell::states::base::Base;
Expand Down Expand Up @@ -43,7 +44,7 @@ pub(crate) struct Expansion {
/// The atom is an assignment value (`A=v`, `export A=v`), so
/// command-substitution output is not field split (POSIX 2.9.1).
pub(crate) assign_ctx: bool,
/// Set when a `""`/`''` literal
/// Set when a `""`/`''` literal (or a quoted `$(...)` whose output was only NUL bytes)
/// was seen so an *empty* expansion is still pushed as an argv word.
/// Without this, `$unset` and `""` are indistinguishable in
/// [`ExpansionOut`] (both → `buf=[], bounds=[]`) and Cmd would push an
Expand Down Expand Up @@ -598,7 +599,7 @@ impl Expansion {
exit_code: ExitCode,
) -> Yield {
// Child is a Script (command substitution). Its captured stdout lives
// in the duped `ShellExecEnv` it owns; read it before deinit.
// in the duped `ShellExecEnv` it owns; take it before deinit.
debug_assert!(matches!(interp.node(child).kind(), StateKind::Script));
if interp.failed() {
// The script failed: the rest of the word is not expanded.
Expand All @@ -611,16 +612,22 @@ impl Expansion {
let parent = interp.as_expansion(this).base.parent;
return interp.child_done(parent, this, 1);
}
// SAFETY: single trampoline frame; the child script's env (and its
// parent buffer in the `Borrowed` case) has no other live borrow.
let stdout = unsafe {
interp
.as_script_mut(child)
.base
.shell_mut()
.buffered_stdout_mut()
let mut stdout = match &mut interp
.as_script_mut(child)
.base
.shell_mut()
._buffered_stdout
{
Bufio::Owned(buf) => core::mem::take(buf),
Bufio::Borrowed(_) => unreachable!(
"dupe_for_subshell gives a command substitution an owned stdout buffer"
),
};
// NUL bytes are dropped as in bash and dash, before the trim and the field split.
let had_nul = bun_core::strings::contains_char(&stdout, 0);
if had_nul {
stdout.retain(|&b| b != 0);
}
.clone();

// Propagate the exit code if the *whole* atom was a single `$(...)`
// (so `$(false)` as argv0 fails the command).
Expand All @@ -643,7 +650,16 @@ impl Expansion {
while hi > 0 && matches!(stdout[hi - 1], b' ' | b'\n' | b'\r' | b'\t') {
hi -= 1;
}
me.current_out.extend_from_slice(&stdout[..hi]);
stdout.truncate(hi);
Comment on lines 650 to +653

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: pre-existing: Users of a quoted or assigned command substitution lose trailing spaces, tabs and CRs that bash keeps; X=$(printf 'a ') or "$(printf 'a ')" yields a, bash yields a . The trim loop at src/runtime/shell/states/Expansion.rs:650 strips b' ' | b'\n' | b'\r' | b'\t' in the no_split branch, but POSIX 2.6.3 removes only trailing newlines from $(...) output. Fix: in the no_split branch (quoted and assign_ctx) trim only trailing \n, leaving space, tab and CR in place, while keeping the newline trim the existing nested echo cmd subst tests rely on. The PR text lists this as separate work; the diff rewrites this hunk (truncate(hi)) and its NUL table shows "x$(printf 'a \0')" now giving xa where bash gives xa .
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

A script runs X=$(printf 'a ') or echo "[$(printf 'a\t')]" through Bun.$ or bun exec; the capture reaches Expansion::child_done at src/runtime/shell/states/Expansion.rs:615 with no_split true (src/runtime/shell/states/Expansion.rs:639-642). The loop at src/runtime/shell/states/Expansion.rs:650-653 drops every trailing byte in b' ' | b'\n' | b'\r' | b'\t' and truncate(hi) commits it, so the word is a. bash and dash remove only trailing newlines in a quoted or assignment command substitution, so they yield a / [a\t]. The base branch has the same loop (it used extend_from_slice(&stdout[..hi])), so this is pre-existing; the diff touches the hunk and, with the NUL drop, now routes "x$(printf 'a \0')" to xa where the base happened to keep xa because the NUL protected the blank. No guard elsewhere restores the blanks: Cmd and Assign consume out.buf as produced.

Verification: pre-existing. Any quoted or assignment-context command substitution whose output ends in a space, tab or CR. The no_split branch strips b' ' | b'\n' | b'\r' | b'\t' (src/runtime/shell/states/Expansion.rs:650-653). POSIX 2.6.3 removes only trailing newlines, so X=$(printf 'a ') yields a versus a in bash. The base has the same predicate at line 643.

@robobun robobun Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The follow-up for this trim is ready on the branch robobun/6d873e57/shell-cmdsubst-keep-trailing-whitespace. It is one commit on top of this PR: robobun/53f4200c/shell-cmdsubst-drop-nul...robobun/6d873e57/shell-cmdsubst-keep-trailing-whitespace

It removes only the trailing line ends (each LF, and a CR directly before it) and keeps trailing spaces and tabs. It rewrites the same hunk as this PR, so it opens as its own PR after this PR merges.

// A quoted substitution that printed only NUL bytes is one empty word, as in bash.
if had_nul && me.cmd_subst_quoted {
me.has_quoted_empty = true;
}
Comment on lines +654 to +657

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: nit: Users get a quoted command substitution whose argv result depends on whether the discarded bytes happened to be NUL. cmd "$(printf '\0')" passes one empty argument while cmd "$(true)" and cmd "$(printf '\n')" pass none; bash passes one empty argument in all three. The rule at src/runtime/shell/states/Expansion.rs:655 keys has_quoted_empty on had_nul, a property of bytes the word no longer contains. Fix: set has_quoted_empty for every quoted $(...) (the me.cmd_subst_quoted branch), not only when a NUL was seen, so the empty-quoted-word rule matches "" at Expansion.rs:481; or leave the flag out and treat the quoted-empty case as one fix. The doc at Expansion.rs:47 also says "only NUL bytes" but the flag is set whenever any NUL was present.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

A user runs cmd "$(printf '\0')" and cmd "$(true)" through Bun.$ or bun exec. At src/runtime/shell/states/Expansion.rs:655-657 the first sets me.has_quoted_empty = true because had_nul is true, so Cmd.rs:390 pushes one empty argv word; the second leaves the flag false and pushes nothing. On the base branch the first case also yields one empty argument (the NUL stayed in the word and C-string consumers truncated it), and the second yields none, so this is not a regression, but the new code now encodes that outcome as an explicit content-dependent heuristic rather than the structural rule bash uses (a quoted substitution is always a word). The doc comment at Expansion.rs:47 describes the flag as set for output that 'was only NUL bytes', but the condition at line 655 does not check that the remaining buffer is empty; it is harmless only because the flag is read solely when the word is empty (Cmd.rs:390, Expansion.rs:238).

Verification: Not a regression: no input changes result versus base. src/runtime/shell/states/Expansion.rs:655-657 sets has_quoted_empty only if had_nul && me.cmd_subst_quoted, and nothing else sets it for a quoted $(...). Cmd.rs:390 then pushes the empty word for cmd "$(printf '\0')" but not for cmd "$(true)"; bash passes one empty argument in both.

if me.current_out.is_empty() {
me.current_out = stdout;
} else {
me.current_out.extend_from_slice(&stdout);
}
} else {
Self::post_subshell_expansion(me, stdout);
}
Expand Down
64 changes: 64 additions & 0 deletions test/js/bun/shell/bunshell.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,70 @@ describe("deno_task", () => {
TestBuilder.command`echo $(echo 1)`.stdout("1\n").runAsTest("nested echo cmd subst");
TestBuilder.command`echo $(echo 1 && echo 2)`.stdout("1 2\n").runAsTest("nested echo cmd subst with conditional");
// TODO Sleep tests

// bash and dash drop NUL bytes from the output of a command substitution.
// A NUL left in the word ends an argument, a builtin operand, an exported
// value and a redirect target there.
describe("drops NUL bytes from the output", () => {
const printArgs = /* ts */ `console.log(JSON.stringify(process.argv.slice(2)))`;

TestBuilder.command`echo "$(echo -e 'a\0b\0c')"`.stdout("abc\n").runAsTest("quoted");
TestBuilder.command`echo $(echo -e 'a\0b c\0d')`.stdout("ab cd\n").runAsTest("unquoted");
TestBuilder.command`echo x$(${BUN} -e ${"process.stdout.write('a\\0b')"})y`
.stdout("xaby\n")
.runAsTest("written by a subprocess");
TestBuilder.command`echo "$( (echo -e 'a\0b'; echo -e 'c\0d') | cat)"`
.stdout("ab\ncd\n")
.runAsTest("written by a subshell and a pipeline");
TestBuilder.command`echo "[$(echo -e 'a\n\0')]"`
.stdout("[a]\n")
.runAsTest("before the trailing newlines are trimmed");
TestBuilder.command`echo $(echo -e 'a\0b'){1,2}`.stdout("ab1 ab2\n").runAsTest("before brace expansion");

TestBuilder.command`${BUN} run ./code.ts x $(echo -e '\0') y $(echo -e 'a \0 b') "$(echo -e '\0')" z`
.ensureTempDir()
.file("code.ts", printArgs)
.stdout(out => expect(JSON.parse(out)).toEqual(["x", "y", "a", "b", "", "z"]))
.runAsTest("a NUL is not a field, and a quoted NUL is one empty argument");

// bash passes ["a", "", "b"]. Here an empty quoted variable is no word
// at all, and the NUL that hid this is gone.
TestBuilder.command`X=$(echo -e '\0'); ${BUN} run ./code.ts a "$X" b`
.ensureTempDir()
.file("code.ts", printArgs)
.stdout(out => expect(JSON.parse(out)).toEqual(["a", "", "b"]))
.todo("an empty quoted variable yields no word")
.runAsTest("a quoted variable assigned only NUL bytes is one empty argument");

TestBuilder.command`X=$(echo -e 'a\0b'); echo "$X"; export Y=$(echo -e 'c\0d'); ${BUN} -e ${"console.log(process.env.Y)"}`
.stdout("ab\ncd\n")
.runAsTest("assignment");

TestBuilder.command`[[ "$(echo -e 'a\0b')" == "ab" ]] && echo yes || echo no; [[ -z "$(echo -e '\0')" ]] && echo yes || echo no`
.stdout("yes\nyes\n")
.runAsTest("[[ ]] operands");

TestBuilder.command`touch rmf rmfzz; rm $(echo -e 'rmf\0zz'); ls`
.ensureTempDir()
.stdout("rmf\n")
.runAsTest("rm removes the file that the output names");

// A debug build aborted on these two words ("interior NUL would truncate
// the C view"). `bun exec` runs them in a child process.
TestBuilder.command`${BUN} exec ${`echo hi > "$(echo -e 'f\\0zz')"; touch t1; [[ -f $(echo -e 't1\\0zzz') ]] && echo yes || echo no`}`
.ensureTempDir()
.stdout("no\n")
.fileEquals("fzz", "hi\n")
.doesNotExist("f")
Comment on lines +1551 to +1554

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): the [[ -f ]] half of this cell only proves the operand names a file that does not exist, not that the NUL was dropped. At test/js/bun/shell/bunshell.test.ts:1551 the expected "no\n" is also what any mangled or truncated-to-garbage operand yields, so a regression that keeps the word wrong in a new way still passes. Fix: create the file the NUL-free word names (touch t1zzz) and assert yes, so only the dropped-NUL path can satisfy it, keeping the existing t1 precondition so the base-branch C-string truncation still produces a different answer.

Why this was flagged

The cell at test/js/bun/shell/bunshell.test.ts:1545-1554 runs touch t1; [[ -f $(echo -e 't1\\0zzz') ]] && echo yes || echo no through bun exec and asserts stdout "no\n". On the base branch the operand is t1\0zzz, which the C-string stat sees as t1, so it prints yes (debug builds abort), and the assertion fails. After the change the word is t1zzz, which does not exist, so no is printed. But no is also printed if a future change leaves the operand as any other nonexistent string (for example dropping the byte after the NUL, or replacing the NUL with another byte), so the assertion cannot distinguish the NUL-dropped word from a wrongly-built word. The redirect half of the same cell is strong (fileEquals("fzz", "hi\n") and doesNotExist("f")); only the -f half is weak. REVIEW.md asks every assertion to assert the strongest invariant.

Verification: nit. Triggering condition: any future regression that leaves the [[ -f ]] operand as some nonexistent string other than t1 passes this cell unchanged. Mechanism verified at test/js/bun/shell/bunshell.test.ts:1550-1555: the cell asserts .stdout("no\n"). The only precondition created is t1; t1zzz is never created, so no is what every nonexistent operand produces.

.runAsTest("redirect target and file test, through bun exec");

// Only the word loses the NUL bytes. The output that the shell itself
// captures keeps them, and a substitution takes nothing out of it.
TestBuilder.command`echo before; echo "$(echo -e 'm\0id')"; echo -e 'a\0b' | cat; echo -e 'c\0d'; echo after`
.quiet()
.stdout("before\nmid\na\0b\nc\0d\nafter\n")
.runAsTest("the captured output of the shell keeps its NUL bytes");
});
});

describe("shell variables", async () => {
Expand Down
5 changes: 5 additions & 0 deletions test/js/bun/shell/exec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ describe("bun exec", () => {
)
.runAsTest("write a lot of data");

TestBuilder.command`${BUN} exec ${`echo "$(echo -e 'a\\0b')"`}`
.env(bunEnv)
.stdout("ab\n")
.runAsTest("command substitution drops NUL bytes from the output");

describe("--help works", () => {
// prettier-ignore
const programs = [
Expand Down
Loading