[matt-dz]

Summary

Closes VULN-4760, VULN-5664, VULN-5630, VULN-1675 (rshell threat pack, triage group 8). One memory-accounting change in interp/ fixes the remaining checks that ran only after memory was already allocated:

• Shared variable budget (VULN-1675). Variable storage is now one run-wide budget shared by every subshell, command substitution and pipeline stage that is alive at the same time. Each overlay charges only what it adds on top of what it inherited, and returns it when it finishes (releaseVarStorage at the four subshell() call sites). A shared lock makes concurrent stages check and update the budget atomically. Previously each concurrent stage started with its own copy of the counter.
• Entry cap (VULN-5630). Variable names plus a fixed 64-byte per-variable overhead now count against a new MaxTotalVarEntryBytes (1 MiB, about 16k variables), so empty-valued variables are no longer free. MaxTotalVarsBytes keeps its value-bytes meaning, so a single MaxVarBytes value still fits and the existing error text is unchanged when the value cap is the one exceeded.
• Field bound before splitting (VULN-5664). The expander builds all of a word's fields before yielding any, so the per-command field cap is now enforced on an upper bound before the split happens. The bound counts unsplit parts plus non-IFS runs in unquoted parameter values. Unquoted command substitution output is charged against the same bound before the expander splits it. Brace alternatives are bounded one at a time, matching how FieldsSeq expands them lazily.
• VULN-4760 is already fixed on main (by fix(interp): bound expansion before command authorization #625, which made brace expansion lazy and capped). This PR adds a regression test only, so that ticket can be closed citing fix(interp): bound expansion before command authorization #625 and this test.

Docs: README and SHELL_FEATURES describe the shared budget, the entry cap, and the pre-split field bound.

Tests

interp/memory_accounting_test.go has one regression test per finding. Each one fails on main and passes here, except 4760, which passes on both:

Finding	Test	On main
VULN-4760	TestVULN4760BraceExpansionBoundedBeforeCommandGate	passes (already fixed)
VULN-5664	TestVULN5664UnquotedSplitBoundedBeforeExpansion, TestVULN5664UnquotedCommandSubstitutionSplitBounded	fail: about 900 MB and 208 MB allocated, against a 64 MiB bound
VULN-5630	TestVULN5630EmptyVariablesChargedAgainstStorageCap, TestVULN5630EmptyAssignmentsChargedAgainstStorageCap	fail: no cap error
VULN-1675	TestVULN1675ConcurrentPipelineStagesShareStorageCap	fails: no cap error

Normal workloads (TestMemoryAccountingNormalWorkloads, which pass on both main and here):

• Storage is released after 40 sequential pipelines, command substitutions and subshells.
• Every stage can see parent variables.
• 5,000 variables work.
• Splitting into 4,000 fields works, as do custom IFS, unquoted command substitution, and exactly 16,384 arguments plus an empty unquoted word.

interp/var_budget_test.go tests the budget directly, including sibling sharing, release, and nested subshells. It also checks that after a run the budget returns exactly to the root's own share.

• go test -race ./...: everything passes except TestCheckRemoteNoHostIO/stat-canary, the known arm64 failure fixed by fix(tests): block ARM64 stat calls in remote I/O canary #684.
• RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash: passes.
• No new symbols were added to the allowlist.

Overlap with parallel fixes

• Group 7 (bounded replacement in xargs -I / find -exec / sed) touches builtins and SHELL_FEATURES.md. The only shared file is that doc, so at most a trivial merge conflict.
• Group 6 (nesting and pipeline depth) touches interp/api.go, so a small textual conflict is possible. Each pipeline stage still copies the parent's variable entries; with the entry cap here that copy is bounded per stage, and group 6's stage limit bounds how many stages there are.

Behavior changes to note

• Scripts can now hold about 16k variables at once. Beyond that, an assignment aborts with variable storage limit exceeded (... bytes of names and entries total).
• Concurrent pipeline stages share the 1 MiB value budget instead of each getting their own.
• A word that is certain to split past 16,384 arguments now fails with the same maximum field count error, just before it is built rather than after.

🤖 Generated with Claude Code

[julesmcrt]

Review

Correct

• The shared counter updates are mutex-protected (interp/vars.go:213–223).
• The new tests verify empty-variable limits, ordinary field splitting, sibling budgets, and sequential subshell cleanup. All targeted existing tests pass.

P1 — clearing inherited variables bypasses the shared storage budget

Location: interp/vars.go:203–205, 215–223

The child's charge is computed as max(newTotal − seed, 0), so clearing an inherited variable creates credit for new child-owned values even though the parent still retains the original allocation.

Reproduced: a parent holding a 600 KiB A; each child clears A and builds a distinct 600 KiB B. The internal reproduction reported 614,400 bytes charged while 2,457,600 bytes of distinct live values existed across parent and children. An end-to-end script (grow A in the parent, clear A and grow B in a subshell) also completes successfully with no diagnostic, where the shared 1 MiB cap should reject it.

Smallest fix: track storage owned by each overlay rather than its net size relative to the inherited environment; hiding or replacing an inherited value must not refund storage an ancestor still retains. Add both a replacement and a clear-then-create regression.

This is an incomplete fix for the shared-cap claim, not a new bypass introduced by the PR.

P1 — temporary inline-assignment overlays leak their shared-budget charge

Location: interp/vars.go:124–125 (introducing shared accounting); affected caller interp/runner_exec.go:241–243

The overlay created while expanding inline assignments is restored without ever releasing its charge; installing the assignments into the original environment then charges them again.

Reproduced: with a configured 600 KiB PAYLOAD environment value, X=$PAYLOAD true fails with X: variable storage limit exceeded (1228800 bytes total) — a single valid 600 KiB script-assigned value, charged twice. Smaller repeated assignments accumulate stale charges the same way.

Smallest fix: release the temporary overlay's charge on all exits (including error paths) before restoring/installing into the previous environment. Add an inline-assignment cleanup test; the existing release tests cover subshells and assignment-only pipeline stages, not this overlay.

Introduced by this PR's sharing of the formerly independent temporary overlay accounting.

P2 — a completed pipeline producer keeps its budget until the consumer finishes

Location: interp/runner_exec.go:385–393

The left stage closes its pipe in its goroutine cleanup, but its variable storage is released only after the right stage finishes. Reproduced: { X=$PAYLOAD; echo marker; } | { cat >/dev/null; Y=$PAYLOAD; echo OK; } fails with Y: variable storage limit exceeded (1228800 bytes total) even though the producer completed before cat observed EOF and the two stages never need both budgets simultaneously.

Smallest fix: release the left stage's storage in its goroutine cleanup, before closing the pipe, and remove the delayed release. Add an EOF-ordered regression test.

P2 — the field estimate rejects commands at the documented argument limit

Location: interp/runner_expand.go:355–360; counting logic at :420–435

The estimator counts adjacent literal and parameter parts independently although they concatenate into one field, then treats an upper bound exceeding the remaining budget as proof that the actual count exceeds it.

Reproduced: x=b then true {1..16383} a$x — exactly 16,384 arguments (16,383 numbers plus ab) — fails with expansion exceeds maximum field count (16385). The existing boundary test uses plain brace fields and misses concatenated parts.

Smallest fix: account for field continuation across adjacent parts, or run a bounded exact counting pass when the conservative estimate exceeds remaining capacity, preserving the early allocation guard. Add a concatenated-parts boundary test.

Validation

All targeted existing tests pass on macOS arm64 (Test(VULN(4760|5664|5630|1675)|MemoryAccountingNormalWorkloads|VarBudget)). A temporary diagnostic test confirmed all four findings (removed afterward; worktree clean at the reviewed head).

Requesting changes for the two P1s.

[matt-dz]

@julesmcrt thanks for the review. I've addressed all four findings in af9b2c6:

P1: clearing inherited variables refunds storage. Each overlay is now charged only for the variables it assigned itself (overlayEnviron.owned). It is no longer charged for its net size relative to what it inherited. Hiding, clearing or replacing an inherited variable leaves the parent's copy charged until the parent is released. This means a subshell override is now charged on top of the parent's copy. I updated TestNonBackgroundSubshellVarOverrideTracking to match: 3 × 340 KiB fits, and double-charging any one of them would not. New regressions:

• TestVarBudgetInheritedStorageNotRefunded: replacement and clear-then-create, for both subshells and pipeline stages.
• The subshell clear then create, subshell replacement and pipeline stage clear then create cases in TestVULN1675StorageChargedOnceAndReleasedPromptly.

P1: the inline-assignment overlay leaks its charge. The temporary overlay is now released in the same deferred cleanup that restores the previous environment, so it is released on every exit, including error paths (runner_exec.go). Regressions: inline assignment charged once (X=$PAYLOAD true with a 600 KiB env value), repeated inline assignments release their overlay, and an inline assignment added to TestVarBudgetReturnsToRootAfterRun.

P2: the pipeline producer holds its budget until the consumer finishes. The left stage now releases its storage in its goroutine cleanup, before pw.Close(). The delayed release after wg.Wait() is removed. Regression: finished producer releases before consumer EOF, which is your exact script.

P2: the field estimate rejects at the argument limit with concatenated parts. The estimate now tracks whether a field is open across adjacent parts:

• a literal or quoted part that continues an open field adds no source;
• a split value's first run joins the open field unless the value starts with IFS;
• trailing IFS closes the field;
• brace alternatives propagate the state;
• an unquoted command substitution conservatively closes it.

Regressions in TestMemoryAccountingNormalWorkloads: x=b; echo {1..16383} a$x (16,384 args), and a mixed split case a$x"q"$y$x with x='b ', y=' c' at exactly 16,384 args.

All the new cases fail on the previous head and pass now. go test -race -p 4 ./... passes everywhere except the known arm64 TestCheckRemoteNoHostIO/stat-canary failure (fixed by #684).

[julesmcrt]

Reviewed af9b2c6958f30c255eba1a6f3eb7b7cdf10990fd.

Targeted normal/race tests, the full interpreter package, and non-Docker scenarios pass, but the following cases are not covered.

P1 — command-substitution normalization can bypass the pre-split field guard

interp/runner_expand.go:484–500 charges the raw command-substitution bytes. The downstream mvdan expander removes NUL bytes and trims trailing newlines before splitting. Removing a NUL embedded between UTF-8 bytes can create an IFS delimiter that the raw-byte counter did not see.

Confirmed with a tiny diagnostic: a two-field collector, IFS=é, and only 12 substitution-output bytes — three repetitions of a\xc3\x00\xa9. The raw output counts as one source; after NUL removal it counts as three. A following substitution in the same word runs before the collector finally rejects the fields:

ok=false fields=["a" "a"]
stderr="REACHED_AFTER_SPLIT\nexpansion exceeds maximum field count (2)\n"
rawSources=1 normalizedSources=3

The eventual field-count rejection still works. The defect is that the new before-splitting protection is skipped: expansion can build the excess fields before the outer collector rejects them. This is an incomplete fix of the allocation-before-check issue, not a claim that an oversized command executes.

Fix: Count the exact normalized substitution text that mvdan will split, with the same IFS semantics. Add the small UTF-8/NUL regression, asserting rejection before the later substitution executes.

P2 — restoring an inherited temporary assignment double-charges its value

interp/vars.go:279–291, called by inline-assignment restoration in interp/runner_exec.go.

With a configured 600 KiB PAYLOAD:

A=$PAYLOAD; ( A=x true; B=y; echo OK ); echo DONE

The child fails on B=y with variable storage limit exceeded (1228801 bytes total). The root-level equivalent succeeds. Restoration stores and charges the inherited A in the child overlay even though the parent already owns the same backing string. This differs from a deliberate script-owned override: the command has finished and the previous inherited binding should be restored.

Fix: Preserve the original overlay ownership state across temporary assignments. Restoring an inherited binding should remove the temporary child override and its charge rather than create a newly owned copy. Add a regression that performs another assignment after restoration.

P2 — restored undeclared variables retain uncharged map entries

interp/vars.go:279–291, together with varStorageCost returning zero for undeclared variables.

17,000 distinct vN= true commands leave 17,000 undeclared entries in the root overlay. Their name/entry cost is 1,178,890 bytes, above the new 1,048,576-byte entry cap, but both root and shared accounting report zero entries. This leaves a reachable gap in the VULN-5630 entry-cap fix. Script-size and execution-time limits still bound the attack; this is not unlimited growth.

Fix: When restoring a previously absent binding, delete the temporary map entry instead of storing an undeclared zero-cost value. Preserve masking semantics where an inherited binding exists. Add temporary-assignment coverage beside the direct-empty-assignment cap tests.

P2 — substitution concatenation and trimmed output cause false argument-limit errors

interp/runner_expand.go:415–421,484–500.

Each of these has exactly 16,384 arguments but fails with expansion exceeds maximum field count (16385):

true {1..16383} a$(echo b); echo OK
true {1..16383} $(echo b)a; echo OK
IFS=,; true {1..16384} $(echo); echo OK

The first two double-count a concatenated field; the last counts a trailing newline that normalization removes entirely. The literal ab control passes.

Fix: Normalize before counting and account for field continuation across command-substitution boundaries, without weakening the early allocation guard. Add these boundary cases.

Requesting changes, particularly for the pre-split undercount and the incomplete entry accounting. All diagnostics were temporary (removed afterward); worktree clean at the reviewed head. Validation ran on macOS arm64; Docker/bash comparison and Linux/Windows runtime checks were not run.

[matt-dz]

@julesmcrt thanks for the second pass. All four findings are addressed in 53a663b.

P1: substitution normalization bypassed the pre-split field guard. chargeSplitSubstOutput now counts exactly the text mvdan splits: NUL bytes removed and trailing newlines trimmed (normalizeSubstOutput in interp/runner_expand.go), with the same IFS rune test. Your IFS=é / a\xc3\x00\xa9 case is now rejected at the first substitution, before the following one runs. Regression: TestVULN5664SubstitutionCountedAfterNormalization asserts the later $(echo REACHED >&2) never runs. It fails on af9b2c6.

P2: restoring an inherited temporary assignment double-charged. FOO=val cmd now snapshots the overlay's own binding (entry and owned storage) when installing the assignment, and puts back exactly that (snapshotBinding/restoreBinding in interp/vars.go). When the overlay had no entry, the restore deletes the temporary override instead of storing a charged copy of the inherited value. A=$PAYLOAD; ( A=x true; B=y; echo OK ); echo DONE now succeeds (new case in TestVULN1675StorageChargedOnceAndReleasedPromptly).

P2: restored undeclared variables left uncharged entries. The same restore deletes the entry for a name that didn't exist before, so vN= true leaves nothing in the root overlay. An entry the overlay did have (including a masking one) is restored as it was. TestVarBudgetTemporaryAssignmentsRestoreExactly checks that no entry or owned storage remains for previously absent names, that an inherited binding isn't copied into the child, and that charged == budget.live.

P2: false argument-limit errors around substitutions. The estimator now applies field continuation across substitution boundaries: a substitution's first run continues a field left open before it (using the previous substitution's actual output when two are adjacent), and the part after a substitution is no longer counted statically; it is charged with that substitution's output only if the output doesn't leave a field open. Since the substitution runs before the expander reaches that part, the early guard still holds. Substitutions inside brace alternatives keep the conservative full count. All three of your cases plus $(echo b)$(echo c) and a$(echo)b now pass at exactly 16,384 arguments (new TestMemoryAccountingNormalWorkloads cases). Over-limit variants (a$(echo "b c"), $(echo "b ")a, a$(echo " ")b, {1..16384} $(echo)a) are still rejected.

Tests: targeted interp tests pass. All new cases fail on af9b2c6. go test -race ./... passes except the known arm64 TestCheckRemoteNoHostIO/stat-canary, which #684 fixes on main. The normalization uses a byte loop instead of strings.ReplaceAll/TrimRight, so the analysis/ allowlist is unchanged.

[julesmcrt]

Reviewed 53a663ba.

All four findings from the previous round are fixed, and the round-1 accounting fixes did not regress. No new issues found.

P1 (normalization bypass): chargeSplitSubstOutput now counts normalizeSubstOutput(out) — NULs removed, trailing newlines trimmed — with the same IFS rune test the expander splits with, so the IFS=é / a\xc3\x00\xa9 shape is charged as the three fields it really produces. Confirmed with the adapted repro: the word is rejected at the first substitution's charge; the following $(echo REACHED >&2) never executes. TestVULN5664SubstitutionCountedAfterNormalization pins this.

P2 (inherited temporary assignment double-charge): snapshotBinding/restoreBinding restore the overlay's own binding exactly and delete the override when the overlay had no entry. A=$PAYLOAD; ( A=x true; B=y; echo OK ); echo DONE now succeeds under budget, at both subshell and root level.

P2 (restored absent names left uncharged entries): the same restore deletes the entry for a previously absent name and refunds by the exact delta, so vN= true leaves nothing behind. TestVarBudgetTemporaryAssignmentsRestoreExactly asserts no entry, no owned storage, and charged == budget.live.

P2 (false rejections at 16,384 args): the estimator now applies field continuation across substitution boundaries — using the previous substitution's actual output for adjacency — and defers the post-substitution part's source to the substitution's own charge. All the boundary cases pass at exactly 16,384 (a$x, a$x"q"$y$x, $(echo b)$(echo c), a$(echo)b, $(echo) with IFS=,), and the over-limit variants (a$(echo "b c"), $(echo "b ")a, a$(echo " ")b, {1..16384} $(echo)a) are still rejected before the excess fields materialize.

I also hunted for new gaps in this fix and found none worth flagging:

• normalizeSubstOutput is equivalent to mvdan's own ReplaceAll(NUL) + TrimRight("\n") under both operation orders; empty, NUL-only, and IFS-only multi-byte outputs all behave consistently.
• Continuation accounting matches the expander's per-part splitting in both directions, including outputs whose bytes join into a multi-byte IFS rune only after concatenation (they are not delimiters, and the estimator agrees), and brace alternatives keep the conservative full count.
• Snapshot/restore of masked entries, duplicate assignment names, and cap-rejected mid-command assignments all keep Σ overlay.charged == budget.live exact; DeclClause builtins are validator-blocked, so nothing can reassign between snapshot and restore.

go test -race ./interp/... passes (except the known arm64 TestCheckRemoteNoHostIO/stat-canary, fixed by #684), and go vet on the touched packages is clean. All diagnostics were temporary (removed afterward); worktree clean at the reviewed head. Validation ran on macOS arm64; Docker/bash comparison and Linux/Windows runtime checks were not run.