Repository navigation
glob: fix brace depth overflow when skipping a deeply nested branch - #32733
Conversation
…ranch
skip_branch scans forward over every '{' in a skipped brace branch and
increments State.brace_depth for each one. That field was a u8, so a
pattern with >255 nested braces inside a skipped branch wraps the
counter in release builds and skip_branch exits at the wrong '}',
causing the matcher to resume mid-group and return the wrong result.
Debug builds panic on the overflow.
Widen State.brace_depth and Wildcard.brace_depth to u32 (glob_index is
already u32, so the pattern cannot contain more braces than a u32 can
count). Also widen the i16 scan counter in match_brace to i32 for the
same reason.
|
Warning Review limit reached
More reviews will be available in 16 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 6:08 PM PT - Jun 26th, 2026
❌ @robobun, your commit a6e74ac has some failures in 🧪 To try this PR locally: bunx bun-pr 32733That installs a local version of the PR into your bun-32733 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward integer widening to fix the overflow, with regression tests.
Extended reasoning...
Overview
This PR widens brace_depth from u8 to u32 in the glob matcher's State and Wildcard structs, widens a local i16 scan counter in match_brace to i32, and updates the one assignment site (u8::try_from(brace_stack.len()).expect(...) → as u32) to match. A regression test in test/js/bun/glob/match.test.ts covers deeply nested braces in skipped branches, including under wildcard backtracking.
Security risks
None. This is purely an integer-width fix in a pattern matcher. The previous behavior was a silent wrap in release (incorrect match result) or a debug panic — neither is exploitable beyond an incorrect boolean. Widening to u32 is bounded by glob_index: u32, so the new counter cannot itself overflow on valid input.
Level of scrutiny
Low. The change is mechanical type widening with no algorithmic changes. All read/write sites of brace_depth remain semantically identical (increment, decrement, compare, copy). Struct sizes are unchanged after padding (Wildcard stays 12 bytes, State stays 36 bytes), so there's no perf regression on the hot *state copies. The brace_stack.len() as u32 cast is safe since the stack is bounded at 10 entries.
Other factors
The bug-hunting system found no issues. The PR includes targeted regression tests that exercise the exact overflow path (>255 nested braces in a skipped branch, both before and after the matching alternative, and under backtracking). The files are not covered by CODEOWNERS. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — straightforward type widening (u8→u32, i16→i32) with regression tests.
Extended reasoning...
Overview
This PR widens brace_depth from u8 to u32 in the State and Wildcard structs of src/glob/matcher.rs, widens the local brace_depth scan counter in match_brace from i16 to i32, and replaces a u8::try_from(brace_stack.len()).expect(...) with a direct as u32 cast. A regression test in test/js/bun/glob/match.test.ts exercises >255 nested braces inside a skipped brace branch, which previously wrapped the u8 counter (wrong result in release, panic in debug).
Security risks
None. This is a pure integer-width fix in a self-contained pattern matcher. It removes a debug-build panic and a release-build silent wraparound; it does not introduce any new code paths, allocations, or external inputs. The as u32 cast on brace_stack.len() is safe because BraceStack is a BoundedArray<Brace, 10> whose len() is at most 10.
Level of scrutiny
Low. The change is mechanical: four type annotations widened, no control-flow or arithmetic changes. Since glob_index is already u32, the number of { characters in a pattern cannot exceed u32::MAX, so the new width is provably sufficient. Struct sizes are unchanged after alignment padding (Wildcard was already 12 bytes due to two u32 fields; State was already 36).
Other factors
No CODEOWNERS cover the touched paths. The bug-hunting system found no issues. There are no outstanding human review comments on the PR. The added test covers the skipped-branch case before, after, and under wildcard backtracking, and the PR description confirms existing match.test.ts tests still pass.
|
Diff is ready. Both CI runs failed only on unrelated flaky lanes; neither touched Build #64835 (284 pass / 2 fail):
Build #64863 (285 pass / 1 fail):
|
|
Isn't the correct fix to not append skipped braces? |
skip_branch was using state.brace_depth as its scan cursor, incrementing
it for every '{' it passed while looking for the closing brace of the
current group. state.brace_depth only means anything as the count of
groups actually entered via match_brace_branch, which is bounded by the
BraceStack cap; braces in a branch being skipped over are not, so enough
of them wrapped the u8 (wrong match result in release, panic in debug).
Track the skipped region's relative nesting in a local counter and
decrement state.brace_depth once at the real closing brace, which is all
the old code ever did to it net. State.brace_depth and
Wildcard.brace_depth go back to u8.
match_brace's forward pre-scan has the same class of bug in its already
local i16 counter, which overflows at 32768 consecutive '{'. Widen it to
i32.
robobun
changed the title
|
Yes, good call. Reworked in 249a8c2.
One widening stayed: |
… that was failing every CI lane)
|
CI on this branch was blocked by an unrelated, fleet-wide failure, not by anything in this diff: That was fixed on main in 034f693 (#32789) after this branch's base, so I merged main in to pick it up. The glob diff itself is unchanged: |
There was a problem hiding this comment.
LGTM — the local-counter rework matches Jarred's suggestion and is behaviorally equivalent to the old skip_branch minus the overflow; description is now in sync.
Extended reasoning...
Overview
This PR fixes a u8 overflow in skip_branch (src/glob/matcher.rs) when the glob matcher skips past a brace branch containing ≥255 nested {. The fix stops using state.brace_depth as scratch storage during the skip scan and instead tracks relative nesting in a local u32, decrementing state.brace_depth exactly once at the group's real closing } — which is all the old code ever did to it net. It also widens match_brace's local pre-scan counter from i16 to i32 for the same overflow class at ≥32768 {. A regression test in test/js/bun/glob/match.test.ts covers both, including the wildcard-backtrack path.
Security risks
None. Glob pattern matching is not auth/crypto/permission-sensitive. The bug was a wrong-result (release) / debug-panic on pathological input, and the fix only changes which integer holds a scan cursor — no new allocation, recursion, or input-derived sizing.
Level of scrutiny
Low-to-moderate. The skip_branch change is ~10 lines and provably equivalent to the prior logic for the non-overflow case: old code computed end = depth - 1, then ran depth up and down through the skipped region and exited when it returned to end; new code runs a local nested from 0 and exits at the first } with nested == 0, then does the single depth -= 1. Same exit point, same net state mutation. The i16 → i32 widening is trivially safe. The u32 local cannot itself overflow because glob_index is u32, bounding the number of { in the pattern.
Other factors
- Jarred-Sumner suggested exactly this approach ("Isn't the correct fix to not append skipped braces?"); the rework in 249a8c2 implements it directly.
- My previous review flagged a stale PR description; the author confirmed it had already been updated (race with my snapshot), and the current description/title accurately reflect the shipped diff. That thread is resolved.
- The only commit since my last review is a merge from main to pick up an unrelated fleet-wide CI fix (OCSP test); the glob diff is unchanged.
- No CODEOWNER covers
src/glob/or*.rs. - The bug-hunting pass found nothing.
- Regression tests are thorough (skipped-deep-nest before/after the matching branch, under wildcard backtrack, and the 40k-
{case formatch_brace).
|
Build 65191 update: merging main fixed the OCSP blocker, and nothing in this build fails on glob. The remaining real failure is again upstream, not this diff: #32774 landed a sourcemap test on main that asserts a forward-slash The glob diff is unchanged and ready; this PR cannot go green on Windows until that is fixed on main. |
2 participants
Reproduction
Cause
skip_branchinsrc/glob/matcher.rsusedstate.brace_depthas its scan cursor while looking for the}that closes the current brace group, incrementing it for every{it passed and decrementing it for every}.state.brace_depthis meant to count the groups the matcher has actually entered viamatch_brace_branch. Every other writer is bounded by theBraceStackcap of 10, which is why it is au8. Braces inside a branch that is only being skipped over were never validated against that cap, so 255 or more consecutive{inside a skipped branch wrap theu8.skip_branchthen satisfies its exit condition at the wrong}and the matcher resumes mid-group, returning the wrong result. Debug builds panic on the checked overflow instead.Fix
Per @Jarred-Sumner's suggestion,
skip_branchno longer writes the braces it merely scans over intostate.brace_depth. It tracks the relative nesting of the skipped region in a local counter and decrementsstate.brace_depthexactly once, at the group's real closing}. That is all the old code ever did to it net (it always exited atoriginal - 1); the intermediate inflated values were pure scratch and are what overflowed.State.brace_depthandWildcard.brace_depthstayu8.match_brace's forward pre-scan has the same class of bug in its (already local)i16counter, which overflows with 32768 or more consecutive{. Widened it toi32.Verification
Added a regression test in
test/js/bun/glob/match.test.tscovering deeply nested braces in a skipped branch (before and after the matching branch, and under wildcard backtracking), plus a 40000-brace pattern for thematch_bracecounter. The first assertion returnsfalseon the released build andtruewith this change. All 26 existingmatch.test.tstests continue to pass.