Repository navigation
shell: widen the brace expansion output slot counter - #37921
Conversation
|
Status: reproduced on a debug build of main ( Latest push (4aac840) only removes a comment flagged by review; the code change is still the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 2 remain after this review. WalkthroughChangesBrace expansion output keys and counters now use Brace expansion
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:29 PM PT - Aug 16th, 2026
✅ @robobun, your commit 4aac840dd2fa34b7e96dc7d83be82428ab5a87f6 passed in 🧪 To try this PR locally: bunx bun-pr 37921That installs a local version of the PR into your bun-37921 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical u16 → usize widening for the brace-expansion output slot counter.
What was reviewed:
- Confirmed all
out_key/out_key_countersites inexpand,expand_flat, and both arms ofexpand_nestedare updated consistently; the removedusize::from()wrappers were the only conversions. - Checked that
outis caller-preallocated fromcalculate_expanded_amount(au32capped at 65536), sousizeis the natural index type and the counter's final valueout.len()always fits. - Tests cover both code paths (flat via shell, nested via
$.braces()) at exactly N=65536, run in subprocesses so the pre-fix abort is observable, and assert the last slot's content.
Extended reasoning...
Overview
This PR widens out_key and out_key_counter from u16 to usize in the shell brace-expansion expander (src/shell_parser/braces.rs: expand, expand_flat, expand_nested). The counter starts at 1 and is bumped once per output slot claimed, so after N slots it holds N. Callers cap N at MAX_BRACE_EXPANSIONS = 65536, which is exactly one more than a u16 holds — so a word with exactly 65536 variants panicked on the final += 1 in overflow-checked builds. The Rust change is a pure type widening plus removal of the now-redundant usize::from(out_key) at each indexing site. Two new subprocess tests in test/js/bun/shell/brace.test.ts exercise the 2^16 flat case and the 4^8 nested case.
Security risks
None. This is an internal counter type change on a debug-only panic path; release builds already wrapped harmlessly (nothing reads the counter after the last bump). No user-controlled data is interpreted differently, and the 65536 expansion cap enforced by callers is unchanged.
Level of scrutiny
Low-to-medium. The diff is a mechanical type substitution with no control-flow change. I traced every touched line: each was either usize::from(out_key) → out_key (identity now that out_key: usize) or the type annotation on the parameter/local. The counter is only ever compared implicitly via split_at_mut bounds and read as an index into out: &mut [Vec<u8>], so usize is the correct type. Both recursive functions and both match arms of expand_nested are updated — no sibling site was missed.
Other factors
The tests follow repo conventions well: subprocess isolation (the pre-fix behavior aborts), Buffer.alloc(n, fill) instead of .repeat(), test.concurrent for independent spawns, concurrent drain of stdout/stderr/exited, and assertions on the count plus first/last variant (the last variant lives in the slot whose claim previously overflowed, so it's load-bearing). The 16-group and 8×3-group inputs stay well under MAX_BRACE_GROUPS = 256. The PR description states both tests fail on unpatched debug builds and pass with the fix, and that the existing brace/bunshell suites and cargo test -p bun_shell_parser pass. No prior review comments on the timeline.
The counter that hands out output slots is bumped once per slot, so after the last of N slots it holds N. The expansion cap admits N = 65536, which does not fit the u16 it was stored in, so a word expanding to exactly 65536 variants hit the overflow check in debug builds (release builds wrapped harmlessly because nothing reads the counter after the last bump). Store the slot index and the counter as usize.
04ae697 to
ea7a685
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM — mechanical u16 → usize widening on the output-slot counter, which was already used only as a slice index.
What was reviewed:
- Confirmed both callers (
Expansion.rs,BunObject.rs) cap atcount > 65536, so N = 65536 is admitted and the counter's terminal value of N fits inusize=out.len(). - Both sibling paths (
expand_flat,expand_nested) get the same widening;expand's public signature is unchanged. - New tests cover both paths at the exact boundary in subprocesses, drain pipes concurrently, and assert on the combined
{stdout, stderr, exitCode}object; the comment-cop feedback was addressed in 4aac840.
Extended reasoning...
Overview
This PR widens out_key and out_key_counter from u16 to usize in src/shell_parser/braces.rs (expand, expand_flat, expand_nested). The counter starts at 1 and is bumped once per output slot claimed, so after the last of N slots it holds N. Both callers cap expansion at count > 65536, admitting N = 65536, which is one past u16::MAX and panics on the final bump in overflow-checked builds. Release builds wrap harmlessly (nothing reads the counter after the last bump). The values are only ever used to index out: &mut [Vec<u8>], so usize is the natural type; all usize::from(...) conversions become direct uses. Two subprocess regression tests are added covering the flat and nested expanders at the exact 65536 boundary.
Security risks
None. This is a pure integer-type widening on an internal counter; input caps (MAX_BRACE_EXPANSIONS, MAX_BRACE_GROUPS) are unchanged and still enforced before allocation.
Level of scrutiny
Low. The Rust diff is a mechanical type change with no control-flow or logic changes — every touched line either changes a parameter/local type or drops a now-redundant usize::from(). The public expand signature is unchanged. The fix is applied to all three sites that share the counter (whole bug class). The PR description demonstrates the mechanism precisely and the evidence block shows the tests fail on main (debug) and pass with the fix on both debug and release.
Other factors
The tests follow repo conventions: subprocess isolation for a process-aborting panic, Buffer.alloc(n, fill).toString() instead of .repeat(), test.concurrent for independent spawns, concurrent pipe draining via Promise.all, and a single toEqual on {stdout, stderr, exitCode}. The one prior review comment (comment-cop on a long explanatory comment) was resolved in 4aac840. No outstanding reviewer feedback remains.
2 participants
Problem
$.braces()input) that expands to exactly 65536 variants, e.g.{a,b}repeated 16 times, aborts debug/assert builds withpanic: attempt to add with overflowatbun_shell_parser::braces::expand_flat(src/shell_parser/braces.rs:754,*out_key_counter += 1).expand_nestedhas the same arithmetic.out_key_counter(au16, src/shell_parser/braces.rs:543) is the index of the next unclaimed output slot. It starts at 1 and is bumped once per slot claimed, so after the last of N slots it holds N. The callers' cap (MAX_BRACE_EXPANSIONS = 65536in src/runtime/shell/states/Expansion.rs and src/runtime/api/BunObject.rs) admits N = 65536, which is one more than au16holds.Fix
out_keyandout_key_counterinexpand,expand_flatandexpand_nestedare nowusize, the type they are used as (they indexout). The counter's final value isout.len(), which always fits. The 65536 cap and release output are unchanged.expand_flat, the reported repro) and one with 8 nested 4-way groups through$.braces()(expand_nested). Each runs in a subprocess and checks the count plus the first and last variant; the last variant is written into the slot whose claim used to overflow.cargo test -p bun_shell_parserpasses.Background
out, oneVec<u8>per result, sized bycalculate_expanded_amount(au32, capped by the callers). The expander then walks the word: the first variant of a group keeps writing into the current slot, and every other variant claims a fresh slot from the counter and copies the prefix built so far into it before continuing. Every slot after the first is claimed exactly once, which is why the counter ends atout.len()rather than at the largest slot index.[review] gate passed · iteration 6 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 6
evidence per changed file
root cause · written by the author bot
The brace expansion output slot counter in braces.rs was a u16 that starts at 1 and is incremented once per output slot, so it reaches N after the last slot, while the callers' cap admits exactly 65,536 expansions, one more than u16::MAX, causing the final increment to overflow (a panic in overflow-checked builds, a harmless wrap in release since nothing reads it afterwards). The fix widens the out_key and out_key_counter values in expand, expand_flat, and expand_nested to usize, which is the natural type since they are only used to index the output slice, leaving the expansion limit and ou…