Repository navigation
Conversation
A NUL byte in the output of `$(...)` stayed in the word. An argument, a builtin operand, an exported value and a redirect target end at the first NUL, so `rm $(printf 'rmf\0zz')` removed `rmf`, not `rmfzz`. `Expansion::child_done` now moves the capture out of the env of the child script and does not clone it. It drops the NUL bytes before the trim and the field split, as bash and dash do. The drop comes from #40204, and the move from #40228. Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
Status: ready for review. Reproduced with (release build of main, bash 5.2 beside it): mkdir t && cd t && touch rmf rmfzz t1
bun exec "rm \$(printf 'rmf\0zz'); ls"
# main: rmfzz t1 (it removed rmf). bash and this PR: rmf t1
bun exec "[[ -f \$(printf 't1\0zzz') ]] && echo yes || echo no"
# main: yes, and a debug build aborts. bash and this PR: noPR: #44752 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughCommand-substitution processing now takes ownership of captured stdout and removes NUL bytes before further processing. Quoted substitutions that contain only NUL bytes are recorded as empty quoted words. New tests cover substitution output across shell contexts and distinguish it from shell-captured output. ChangesShell command substitution
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Command substitution now drops NUL bytes from its output, matching bash and dash. The change is narrow and comes with tests covering the new behavior. No merge-blocking risk is evident. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the new unreachable! on the Bufio::Borrowed arm (src/runtime/shell/states/Expansion.rs:622): the command-substitution child is always duped with OutKind::Pipe and ShellExecEnvKind::CmdSubst (Expansion.rs:205-213), which dupe_for_subshell maps to Bufio::Owned, so the arm is not reachable. The mem::take leaves the child env's buffer empty until deinit_node(child) a few lines later with no reader in between, and the retain closure is not a form flagged by the byte-search source lint.
Extended reasoning...
The change replaces an unsafe clone of the command-substitution capture with a mem::take on the owned buffer, strips NUL bytes before trimming and field splitting, and removes the now-unused unsafe fn buffered_stdout_mut; it touches no auth, crypto or injection surface, though it does feed shell words into C-string consumers. The invariant behind the new unreachable! is provable from the dup site, and the ownership transfer has no later reader. The posted findings are a test-strength nit, a pre-existing trim bug and a bash-divergence nit around has_quoted_empty, so a human should weigh the deliberate behaviour deviation rather than this being approved outright.
| .ensureTempDir() | ||
| .stdout("no\n") | ||
| .fileEquals("fzz", "hi\n") | ||
| .doesNotExist("f") |
There was a problem hiding this comment.
🟡 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.
| 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); |
There was a problem hiding this comment.
🟣 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.
There was a problem hiding this comment.
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; | ||
| } |
There was a problem hiding this comment.
🟣 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.
This branch has not been deployed
2 participants
Problem
$(...)output stays in the word, and each C-string consumer ends the word there.rm $(printf 'rmf\0zz')removesrmf, exit 0. bash removesrmfzz.[[ -f $(printf 't\0z') ]]:panic: ZStr::as_cstr: interior NUL would truncate the C view.Expansion::child_done(src/runtime/shell/states/Expansion.rs:616-649) copies the captured stdout into the word unchecked.Fix
child_donedrops the NUL bytes before the trim and the field split, as bash and dash do.unsafe fn buffered_stdout_muthas no caller and is removed.test/js/bun/shell/bunshell.test.ts(12 new cells) andexec.test.ts(1). All fail on main. Also all oftest/js/bun/shell/.Background
Script. Its stdout goes to a buffer in its ownShellExecEnv, andchild_doneis the only reader.[[ == ]]and field splitting stay wrong.Downsides
X=$(printf '\0'); cmd "$X"now passes no argument (main and bash: an empty one), because an empty quoted variable is no word in Bun..env(),process.env,.cwd(), a.envfile,.shtext or Workerargvstill ends the word. That needs a reject-or-cut decision..text+512 bytes.Notes
Credit. The drop is the
child_donehunk of #40204 (4b7c7b9, b9ae552). Thecore::mem::takeis in #40228. This PR carries that part alone, with tests. After it, #40204 has 66 conflict regions against main (65 before), and #40228 has 126 (124 before).What changes for a word. Before, a word from output with a NUL was cut at the first NUL by each C-string consumer, and
[[ ]]string tests and assignments kept the NUL. Now the word is all the bytes that are not NUL. The drop comes first, so"$(printf 'a\n\0')"isa, andx $(printf '\0') yis two arguments.bash cells.
rm,mkdir,touch,cd,[[ ]], redirect, assignment,export, glob and brace cells, each beside bash 5.2. 28-cell set: 1 differs (main 24). 8-cell set: 0 differ (main 4). The one left iscommand not found, exit 1 here and 127 in bash. These sets have no unquoted[[ ]]operand with a blank (see below).Cells that move away from bash. The NUL hid four bugs that main has without a NUL. Their NUL twins now match their NUL-free twins:
"x$(printf 'a \0')"xaxaxa"$(printf 'a ')"(twin)aaa$(printf 'a \0')baba,b$(printf 'a ')b(twin)ababa,b[[ $(echo -e 'a\0 b') == ab ]][[ $(echo a b) == ab ]](twin)X=$(printf '\0'); cmd "$X"X=; cmd "$X"(twin)~"$(printf '\0')"~$HOME~~""(twin)$HOME$HOME~With
X=$(printf '\0'),cd "$X"now goes to$HOME, ascd "$EMPTY"does on main."$(printf '\0')"used directly stays one empty argument: a quoted substitution whose output held a NUL setshas_quoted_empty. Atodocell inbunshell.test.tsrecords the variable case with the result of bash. The four causes (quoted trim of blanks, blank next to text,[[ ]]field split, empty quoted expansion) are separate work.NUL channels that stay open. Each was checked on a release build with this change. With the value
rmf\0zz,rmremovesrmf, and.cwd()runs in the directory before the NUL:$Xfrom.env(),$.env()orprocess.env$Xfrom a.envfile underbun exec.cwd()with a NUL.shfileargvread as$NJS template text and
${...}values are already refused (#25698). For these caller values the choice is reject or cut, and it is not made here.Measured on release builds of main
bbdc5a519ewith and without this change (same toolchain, built back to back). The comments changed after the measurement, the code did not.$(...)statement (mi_mallochits, gdb, (2000 - 1000) / 1000 lines, main vs PR):X=$(..)26 -> 24,"$(..)"30 -> 28, unquoted 38 -> 37.echo hicontrol 10 -> 10.mi_free: 32 -> 30, 36 -> 34, 46 -> 45, control 13 -> 13.$(...)whose output holds a NUL (mi_mallochits, gdb): no-split 26 (NUL-free 26), split 34 (NUL-free 34).$(...):valgrindandperfare not available here (no hardware counters), so these are gdb single-steps from the capture read untilInterpreter::child_donereturns, 5 steady-state hits, all equal. Arep movsbcounts once.X=$(cat F): 17 B 1492 -> 1312, 4 KiB 1676 -> 1687, 64 KiB 1830 -> 7447.echo $(cat F): 17 B 1802 -> 1782, 4 KiB 68110 -> 68359. With eachrep movsbbyte as one step:X=$(cat F)4 KiB 9866 -> 1687, 64 KiB 132900 -> 7447.ru_maxrss, 5 runs, min-max): main 779.7-780.0 MiB, PR 545.5-571.6 MiB..text: +512 bytes (size, 88695343 vs 88695855).Interpreter::child_done8436 -> 8651 bytes (nm -S).$(...)statement: 2 -> 2 (gdbcatch syscall, (2000 - 1000) / 1000 lines).Not done here.
push_current_outstill copies each finished word intoout.buf(Vec::append), so one copy of the capture remains on this path. bash prints a warning for a dropped NUL. AnExpansionhas no stderr route, and dash prints none.Tests. The cells make the NUL with the
echo -ebuiltin and with a subprocess. The redirect target and[[ -f ]]cells run throughbun exec, because a debug build without the fix aborts the process on them. One cell checks that the output the shell itself captures keeps its NUL bytes and loses nothing to a substitution.Suites. Debug build:
bunshell.test.ts,exec.test.ts,yield.test.ts,assignments-in-pipeline.test.ts,pipeline_stack.test.ts,shell-cmdsub-crash.test.ts,test/internal/source-lints/byte-search.test.ts. Inbunshell.test.tsthe sevenstdin redirect from a zero-length buffercells take about 5 s each on this debug build and time out at the 5 s limit with and without this change. Release build: all oftest/js/bun/shell/, withleak.test.tsandshell-leak-args.test.ts. The twolspermission tests fail as root on main too.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts