css: bound raw-token expansion when compiling nesting for older targets - #32453
css: bound raw-token expansion when compiling nesting for older targets#32453robobun wants to merge 8 commits into
Conversation
When compiling CSS nesting away for browser targets that don't support it, each nested rule is duplicated once per enclosing selector combination, and every copy carries its own clone of the rule's declarations. The existing MAX_SELECTOR_EXPANSION cap bounds the number of such copies, but not the size of each copy: an unparsed property value (any value the property-specific parser couldn't read, stored as a raw TokenList) can hold thousands of TokenOrValue nodes, so a handful of two-selector nesting levels over a few-thousand-token value stays well under the 65,536-selector cap while deep-cloning gigabytes of TokenOrValue. Fuzzer signature: oom:css:_RINvXNtC15bun_collections7vec_extINtNtC5alloc3vec3Vec NtNtNtC7bun_css10properties6custom12TokenOrValueE... i.e. <Vec<TokenOrValue> as VecExt>::deep_clone_with. Add MAX_TOKEN_EXPANSION (1,048,576 tokens, ~100 MB of in-memory TokenOrValue) and charge multiplier * selectors * token_weight in charge_selector_expansion, where token_weight is the recursive TokenOrValue count across the rule's unparsed/custom declarations. Past the cap, minify fails with a new token_expansion_limit_exceeded error instead of allocating.
|
Updated 1:08 AM PT - Jun 17th, 2026
✅ @robobun, your commit 956d7927efb500af5e6c129f80b64d6fa49dabf4 passed in 🧪 To try this PR locally: bunx bun-pr 32453That installs a local version of the PR into your bun-32453 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new token-expansion budget is added to the CSS minifier to prevent OOM/hangs when compiling deeply nested CSS rules with large unparsed property values. ChangesCSS Token Expansion Budget
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/css/nested-selector-list-expansion.test.ts`:
- Around line 386-389: The test assertion in the
nested-selector-list-expansion.test.ts file is checking for the wrong error
constant in the expect assertion for stderr validation. Replace the reference
from LIMIT_ERROR to TOKEN_LIMIT_ERROR in the stringContaining check to ensure
the CLI regression test specifically validates the token-expansion failure path
rather than a generic selector-limit message. This ensures consistency with the
specificity used in other minify/prefix test assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09a10d3e-8278-47d6-a472-276c6c9298a4
📒 Files selected for processing (7)
src/css/css_parser.rssrc/css/declaration.rssrc/css/error.rssrc/css/properties/custom.rssrc/css/rules/mod.rssrc/css/rules/style.rstest/js/bun/css/nested-selector-list-expansion.test.ts
|
Not a duplicate of #32451: that PR bounds the serialized selector-prelude bytes on the printer side (trigger: Neither catches the other's reproduction: #32451 only meters preludes, and the selectors in this PR's repro are tiny; this PR only charges |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…n cap Two gaps in MAX_TOKEN_EXPANSION found in review, both reproducing the same Vec<TokenOrValue>/Vec<i32> deep_clone OOM under the selector cap: - CssRule::Unknown nested in a style rule stores its prelude and block as raw TokenLists and is deep-cloned by the same per-selector split, but charge_selector_expansion only weighed the carrying style rule's own declarations (which are empty when the payload is in the at-rule body). Charge prelude + block against the cap in CssRuleList::minify. - TokenList::token_weight's Env arm only counted the fallback TokenList, not the unbounded Vec<i32> indices list that every deep_clone also reallocates. Count each index as one unit (conservative). Factor the charge-and-check into MinifyContext::charge_token_expansion so both call sites share it.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/css/rules/style.rs (1)
392-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCharge token expansion when selector splitting clones declarations.
Line 392 skips the whole budget when
selector_expansion_multiplier == 1, but a top-level incompatible selector list is still split later anddc::decl_block_static(...)deep-clones the declaration block for each split rule at Lines 878-885. A large unparsed/custom value can still be cloned once per selector without hittingMAX_TOKEN_EXPANSION.Possible fix
- if context.selector_expansion_multiplier > 1 { - let copies = context - .selector_expansion_multiplier - .saturating_mul(self.selectors.v.len().max(1)); + let selector_count = self.selectors.v.len().max(1); + let selectors_incompatible = self.selectors.v.len() > 1 + && context.targets.should_compile_selectors() + && !self.is_compatible(context.targets); + let splits_selectors = selectors_incompatible + && !(context.targets.is_compatible(css::Feature::IsSelector) + && !self.selectors.any_has_pseudo_element() + && self.selectors.specifities_all_equal()); + let copies = context + .selector_expansion_multiplier + .saturating_mul(selector_count); + + if context.selector_expansion_multiplier > 1 { context.selector_expansion_total = context.selector_expansion_total.saturating_add(copies); if context.selector_expansion_total > super::MAX_SELECTOR_EXPANSION { context.err = Some(crate::error::MinifyError { kind: crate::error::MinifyErrorKind::selector_expansion_limit_exceeded, @@ }); return Err(MinifyErr::minify_err); } - // Same expansion multiplies this rule's unparsed/custom property - // token lists. A large raw value under the selector cap still - // deep-clones into gigabytes of `TokenOrValue`, so budget the - // token payload separately. - if context.charge_token_expansion(copies, self.declarations.token_weight()) { - context.err = Some(crate::error::MinifyError { - kind: crate::error::MinifyErrorKind::token_expansion_limit_exceeded, - loc: self.loc, - }); - return Err(MinifyErr::minify_err); - } + } + + // Same fan-out multiplies this rule's unparsed/custom property token + // lists. Include top-level selector splitting too: it deep-clones the + // declarations even when the nesting multiplier is still 1. + if (context.selector_expansion_multiplier > 1 || splits_selectors) + && context.charge_token_expansion(copies, self.declarations.token_weight()) + { + context.err = Some(crate::error::MinifyError { + kind: crate::error::MinifyErrorKind::token_expansion_limit_exceeded, + loc: self.loc, + }); + return Err(MinifyErr::minify_err); }Also applies to: 878-885
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/css/rules/style.rs` around lines 392 - 410, The token expansion budget check at line 392 is only performed when selector_expansion_multiplier is greater than 1, but the declarations get cloned for each selector even when the multiplier equals 1 (as shown by the dc::decl_block_static calls at lines 878-885). This allows large unparsed or custom property values to bypass the MAX_TOKEN_EXPANSION limit. Move the token expansion charging logic (the call to context.charge_token_expansion with copies and self.declarations.token_weight()) outside of the condition that checks if selector_expansion_multiplier > 1, so that token weight is always budgeted when selector splitting will occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/css/rules/style.rs`:
- Around line 392-410: The token expansion budget check at line 392 is only
performed when selector_expansion_multiplier is greater than 1, but the
declarations get cloned for each selector even when the multiplier equals 1 (as
shown by the dc::decl_block_static calls at lines 878-885). This allows large
unparsed or custom property values to bypass the MAX_TOKEN_EXPANSION limit. Move
the token expansion charging logic (the call to context.charge_token_expansion
with copies and self.declarations.token_weight()) outside of the condition that
checks if selector_expansion_multiplier > 1, so that token weight is always
budgeted when selector splitting will occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5d3e408-c841-47bb-a2fc-80ea80277d38
📒 Files selected for processing (4)
src/css/properties/custom.rssrc/css/rules/mod.rssrc/css/rules/style.rstest/js/bun/css/nested-selector-list-expansion.test.ts
|
Re the coderabbit outside-diff note on |
| let weight = self.declarations.token_weight(); | ||
| if weight > 0 { | ||
| context.token_expansion_total = context | ||
| .token_expansion_total |
There was a problem hiding this comment.
🟡 Beyond CssRule::Unknown (above), self.declarations.token_weight() also misses two more TokenList carriers in the same sty.rules.deep_clone() subtree that the Unknown-specific fix wouldn't cover: (1) @container style(--foo: x x …) — StyleQuery::Feature(Box<Property::Custom>) is deep-cloned via ContainerRule::deep_clone but the Container arm of CssRuleList::minify only recurses into cont.rules and never charges the condition (likewise MediaFeatureValue::Env fallbacks in @media/@container size queries); (2) nested rules' selectors — PseudoElement::CustomFunction { arguments: TokenList } / PseudoClass::CustomFunction (e.g. .inner::-foo(x x …)), cloned via self.selectors.deep_clone(). Both reproduce the same depth-12 ~0.9 GB OOM with token_expansion_total == 0. Rather than patching each carrier, a recursive StyleRule::token_weight() (declarations + selector CustomFunction args + nested at-rule preludes/blocks/conditions) charged once at the split level would close all of these — including the Unknown gap — together.
Extended reasoning...
What this adds to the comment above
The 🔴 comment above establishes that charge_selector_expansion only weighs self.declarations.token_weight() and so misses TokenLists carried elsewhere in the cloned subtree, citing CssRule::Unknown as the example. Both fixes that comment proposes are scoped to CssRule::Unknown specifically (charge prelude + block in the _ => {} arm of CssRuleList::minify, or walk self.rules for Unknown children). This comment names two further carriers that an author implementing either of those fixes literally would still leave open — same multiplier, same deep_clone sink, same crash class, but different containers in different code paths — so the fuzzer would surface them next under the same signature.
Carrier 1: @container style() conditions
@container is in allowed_in_style_rule() (css_parser.rs:1041), so it nests inside style rules. @container style(--foo: x x x …) parses via ContainerCondition::parse (with ALLOW_STYLE, container.rs:322) → parse_style_query → StyleQuery::parse_feature (container.rs:147–156) → Property::parse on PropertyId::Custom(--foo) → CustomProperty::parse → TokenList::parse, producing a StyleQuery::Feature(Box<Property::Custom>) whose value is an arbitrarily large TokenList. ContainerRule::deep_clone (container.rs:364–374) deep-clones condition → StyleQuery::deep_clone (container.rs:180–194) → dc::property → CustomProperty::deep_clone → TokenList::deep_clone. The Container arm of CssRuleList::minify (rules/mod.rs) only does cont.rules.minify(...) and never charges the condition; every enclosing style rule has empty declarations (only a nested @container), so token_expansion_total stays at 0. The same applies to MediaFeatureValue::Env(EnvironmentVariable) fallbacks reachable via @media (width: env(--x, <large fallback>)) and @container (width: env(--x, …)) size queries — QueryFeature::deep_clone deep-clones the fallback TokenList and nothing charges it.
This is not covered by the Unknown fix: @container has its own CssRuleList::minify arm (Container(cont)), not the catch-all _ => {}, and "walk self.rules for Unknown children" doesn't match a Container child.
Carrier 2: selector CustomFunction arguments
PseudoElement::CustomFunction { arguments: TokenList } and PseudoClass::CustomFunction { arguments: TokenList } (parser.rs:1040–1045, 3150–3155) are parsed via TokenList::parse_raw (parser.rs:1312–1317, 1406–1410), so .inner::-foo(x x …) produces a multi-thousand-entry Vec<TokenOrValue> on the inner rule's selector. The partition path's sty.rules.deep_clone(context.arena) reaches StyleRule::deep_clone → self.selectors.deep_clone() (style.rs) → per-component deep_clone() → PseudoElement::deep_clone() = self.clone() (parser.rs:3177–3179) → TokenList::clone() → Vec<TokenOrValue>::clone(), allocating a fresh vec per clone. The leaf rule's declarations (color: red) parse as a typed Property::Color, so self.declarations.token_weight() == 0 and token_expansion_total again stays at 0.
This is not covered by either Unknown fix or any "walk self.rules for at-rule TokenLists" fix: the carrier lives in the selector tree (StyleRule.selectors), not in StyleRule.rules.
Step-by-step proof (concrete numbers, both carriers)
Same skeleton as the PR's repro — only the payload location changes:
const payload = "x ".repeat(3000); // ~6,000 raw tokens
// Carrier 1: @container style() condition
const css1 = "x::part(a), y::part(b) {\n".repeat(12)
+ "@container style(--foo: " + payload + ") { .inner { color: red } }";
// Carrier 2: selector CustomFunction
const css2 = "x::part(a), y::part(b) {\n".repeat(12)
+ ".inner::-foo(" + payload + ") { color: red }";
cssInternals._test(css1 /* or css2 */, "", { chrome: 80 << 16 });- Selector cap. At depth 12 with two-selector lists,
selector_expansion_total≈ 8K–12K, far under 65,536. - Token cap. Every style rule on the path has
declarations.token_weight() == 0(empty blocks for the 12 wrapper levels; parsedcolor: redfor.inner), sotoken_expansion_totalstays at 0. - Split-clone. Levels 2..12 carry implicit
Component::Nesting→ incompatible with chrome 80;::part()is a pseudo-element so no:is()collapse.minify_style_armpartitions both selectors at every level, executingsty.rules.deep_clone(context.arena)per partition, doubling the leaf subtree at each level → ~2^11 ≈ 2,048 clones bottom-up. - Allocation. 2,048 × ~6,000 tokens × 72 B/
TokenOrValue≈ 0.88 GB ofVec<TokenOrValue>allocations before either cap fires — same magnitude and crash class this PR targets. (Carrier 2 goes throughVec::clone()rather thandeep_clone_with(), but the allocation is identical.)
Suggested fix
Rather than patching three more arms individually (the catch-all for Unknown, the Container/Media arms for conditions, and a separate selector walk), replace the per-rule declaration-only weight with a recursive StyleRule::token_weight() that sums self.declarations.token_weight() + Σ arguments.token_weight() over each Component::PseudoElement(CustomFunction{..}) / Component::NonTsPseudoClass(CustomFunction{..}) in self.selectors + a walk over self.rules collecting TokenList weights from Unknown.{prelude,block}, Container.condition, Media.query env fallbacks, and recursing into nested style rules. Charged once at the outermost split level (where copies is computed), this closes all four carriers under the new cap's documented contract ("raw TokenOrValue nodes that compiling nested rules … may clone") in one place.
There was a problem hiding this comment.
Verified all three carriers (@container style(--foo: ...), ::-foo(...), :-foo(...)) reproduce at D=12. These, along with @media env fallbacks and selector-list arguments, are exactly what #31913's clone_weight module walks: it recursively weighs Container.condition via style_query -> property -> token_list, selector Component::PseudoElement(CustomFunction) / PseudoClass(CustomFunction) arguments, media/supports conditions, and the full nested rule_list, charging the total against MAX_SELECTOR_SPLIT_CLONE_WEIGHT in the while incompatible.len() > 0 loop before the clone. Reimplementing that walk here at the charge_selector_expansion layer would duplicate ~200 lines of #31913 and conflict on merge.
This PR's scope is the nesting-multiplier vector the fuzzer reported (Vec<TokenOrValue>::deep_clone_with under declarations + the immediately adjacent Unknown and env() carriers surfaced in this review round); #31913 is listed under Related as the complementary cap that bounds every split-clone payload carrier. Leaving this open for a maintainer to weigh in on scope; happy to fold a comprehensive walk in if preferred over landing #31913.
charge_selector_expansion previously only charged tokens when the enclosing nesting multiplier was > 1, so a flat top-level rule whose N > 1 incompatible selectors minify_style_arm partitions into N cloned declaration blocks was never charged. Gate on copies > 1 instead (where copies = multiplier * selectors): a flat single-selector rule still pays nothing, and a flat N-selector rule charges N * W as it should.
The previous wording claimed other property kinds are fixed-size, which is not true for list-typed parsed values (font-family, background-image, etc.). Those carry no raw TokenOrValue nodes so MAX_TOKEN_EXPANSION is not the right budget for them; state that accurately rather than claiming they are bounded.
The copies > 1 gate added in 29405e1 charged every flat top-level rule with N > 1 selectors, including when minify_style_arm never partitions it (no targets configured, or all selectors compatible). Those cases clone nothing, so charging N * W is a false positive on input that compiled fine before this PR. Gate additionally on should_compile_selectors(): the cheapest necessary condition for any partition. With no selector compilation configured the partition never runs and nothing is charged. Pin the no-targets flat case so it stays that way.
1 participant
Fixes a fuzzer-found OOM in the CSS minifier.
Fuzzer signature:
oom:css:_RINvXNtC15bun_collections7vec_extINtNtC5alloc3vec3VecNtNtNtC7bun_css10properties6custom12TokenOrValueEINtB3_6Ve, i.e.<Vec<TokenOrValue> as VecExt>::deep_clone_with, on1.4.0-canary.1+55f6c899f.Repro
6.3 KB of CSS; chrome 80 is the default bundler target. On the unfixed build, D=10 allocates ~360 MB, D=12 passes ~1.5 GB, D=14 passes 6 GB; none hit the existing 65,536-selector cap (which only fires at D=16). The same shape reproduces with
--foo:,z-index:, or any property whose value the property-specific parser can't read; nothingcolor-specific. Reaches the same path throughbun build --minify.Cause
When the targets require compiling CSS nesting away, each nested rule is duplicated once per enclosing selector combination (
selector_expansion_multiplierinMinifyContext), and every copy carries its own clone of the rule's declarations. A property value that the property-specific parser couldn't read is stored asProperty::Unparsed/Property::Customholding a rawTokenList(Vec<TokenOrValue>), which is deep-cloned in full for every copy.MAX_SELECTOR_EXPANSION(src/css/rules/mod.rs) bounds the number of copies at 65,536 but not their size, so a multi-thousand-token value under the cap still clones into gigabytes ofTokenOrValue. In the repro,sizeof(TokenOrValue)is 72, so 4,096 copies of a 6,001-token value is 4,096 x 6,001 x 72 ≈ 1.7 GB of allocations; the failed allocation is exactly 6,001 x 72 = 432,072 bytes.Fix
Add
MAX_TOKEN_EXPANSION = 1 << 20(~1M tokens, on the order of 100 MB of in-memoryTokenOrValue) next toMAX_SELECTOR_EXPANSION.StyleRule::charge_selector_expansion(src/css/rules/style.rs) already chargesmultiplier x selectorsagainst the selector cap; it now also chargesmultiplier x selectors x token_weightagainst the new cap, wheretoken_weightis the recursiveTokenOrValuecount across the rule's unparsed/custom declarations (newTokenList::token_weightin src/css/properties/custom.rs andDeclarationBlock::token_weightin src/css/declaration.rs). Past the cap, minify fails with a newMinifyErrorKind::token_expansion_limit_exceeded(src/css/error.rs) instead of allocating. Rules with only parsed properties havetoken_weight == 0and pay nothing; the walk is O(tokens) per charged rule, bounded by input size.Related
Complements #31913 (bounds the split-clone payload, but only fires when a selector is actually target-incompatible;
::part()is compatible with chrome 80 so nothing splits here) and #31916 (bounds selector-chain bytes, but the selectors here are tiny; the amplification is the declaration value). Different crash signatures; each covers a distinct amplification vector under the same expansion multiplier.Verification
_testandprefixTestinstead of OOMing;minifyTestwith no targets still completes (nothing expands).test/js/bun/css/nested-selector-list-expansion.test.ts(the existing home of the selector-expansion cap tests) fail on the unfixed build (5 failures) and pass with the fix (33/33).test/js/bun/css/css.test.ts(1093) andtest/bundler/css/(167) pass;test/js/bun/css/passes except the pre-existing debug-build fuzz-test timeouts (css-fuzz.test.ts,color.test.tsfuzz ansi256), identical on the unfixed build in the same container.