css: budget selector expansion and nesting substitution by bytes - #31916
css: budget selector expansion and nesting substitution by bytes#31916robobun wants to merge 3 commits into
Conversation
The selector-expansion limits bound how many selectors compiling nesting away produces (MAX_SELECTOR_EXPANSION) and how many & substitutions the printer performs (MAX_NESTING_EXPANSIONS), but both count units, not bytes. Per-selector size is input-controlled (long identifiers, multi-argument :lang(), ::part(), raw custom pseudo-class arguments), so a ~30 KB stylesheet could stay under both count limits while expanding to hundreds of MB of cloned rules and output. Charge the minify-side expansion by an estimated serialized byte weight (MAX_SELECTOR_EXPANSION_BYTES, 64 MB) in addition to the count, and meter the bytes emitted by & parent-selector substitutions against a stylesheet-wide budget (MAX_NESTING_EXPANSION_BYTES, 64 MB) at print time. Past either budget, a catchable error is reported instead of materializing the blowup. Stylesheets with ordinary-sized selectors keep hitting the existing count limits first.
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. Warning Review limit reached
More reviews will be available in 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (3)
WalkthroughThis PR extends the CSS minifier to enforce byte-based budgets on selector expansion alongside existing selector-count limits. Two independent layers meter output: nesting serialization meters ChangesCSS Selector Expansion Byte-Based Budgeting
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 `@src/css/selectors/selector.rs`:
- Around line 242-404: The estimator must stop truncating lengths to u32 and
must charge numeric token payloads: change all weight-related types and local
vars in selector_list_weight, selector_weight, component_weight,
ident_or_ref_weight, pseudo_class_weight, pseudo_element_weight,
token_list_weight and token_weight to use u64 (including BASE/NAME constants),
replace all len() casts to u64 and use saturating_add on u64, and in
token_weight include the numeric portion for Token::Dimension (charge dim.value
serialized size or its decimal string length) and similarly charge numeric-like
tokens (e.g., Number/Percentage/Dimension) instead of flat-rates so numeric
payloads are counted; ensure var/env fallbacks and function/name lengths are
also u64-consistent and return the new u64 weight type throughout the call
chain.
🪄 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: da6a57c5-8ce3-4fa9-be6f-57e2e2e4394d
📒 Files selected for processing (7)
src/css/css_parser.rssrc/css/error.rssrc/css/printer.rssrc/css/rules/mod.rssrc/css/rules/style.rssrc/css/selectors/selector.rstest/js/bun/css/selector-expansion-bytes.test.ts
Selector weight estimation truncated byte lengths to u32, so payloads past 4 GB could wrap to a small weight instead of charging the budget. Keep the whole weight chain in u64 (usize to u64 is lossless on every supported target). Charge numeric tokens with a constant bound derived from dtoa's shortest round-trip form instead of the generic flat rate.
|
CI status: the diff-relevant lanes are green. The new regression tests in The remaining failures in build 60959 are unrelated to this change:
Ready for review; nothing here depends on those lanes. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found here, but this adds a fair amount of new estimation logic (the selector/token weight walk and the chain-bytes accounting heuristic) on top of the existing expansion caps, so it's worth a human pass before merging — especially given the expected conflicts with #31913.
Extended reasoning...
Overview
This PR adds byte-based budgets alongside the existing count-based caps for CSS selector expansion when compiling nesting away for older targets. It touches src/css/{css_parser,error,printer,rules/mod,rules/style,selectors/selector}.rs plus a new test file. The bulk of the new code is a ~170-line selector-weight estimator (selector_list_weight → token_weight) that walks the Component/PseudoClass/PseudoElement/TokenList AST, two new MinifyContext fields with chain-bytes accounting in charge_selector_expansion/minify_nested_rules, and a refactored serialize_nesting that meters emitted bytes via a depth counter on Printer.
Security risks
None introduced. This is a hardening change against adversarial-input OOM (fuzzing crash); it adds defensive limits rather than removing them. No auth, crypto, filesystem, or network surface is touched. The 64 MB thresholds are generous enough that legitimate stylesheets should never trip them, and the existing count caps are checked first so ordinary cases keep their existing error.
Level of scrutiny
Moderate. The change follows the established pattern of #31276/#31277/#31482/#31642 and is well-tested (7 new tests covering both blowup → error and below-budget → unchanged-output cases, plus byte-exact regression locks on the minimized fuzz input). However, the weight estimator is a new heuristic that touches many AST node types, the chain-bytes contribution uses an average-weight-per-level approximation, and the serialize_nesting refactor introduces depth tracking to avoid double-counting — these are the kind of subtleties where a second pair of eyes on the accounting model is valuable.
Other factors
- CodeRabbit raised one concern (u32 truncation + uncharged numeric tokens) which was addressed in 94751a7 and confirmed resolved.
- The PR description notes expected textual conflicts with #31913 in
error.rs/rules/mod.rs/css_parser.rs; whichever lands second needs a manual merge. - The musl build failures in CI appear to be an unrelated LTO/toolchain issue (data-layout mismatch in
regular-lto-flag-stub.bc), not caused by these source changes. - The meter-depth decrement in
serialize_nestingis correctly placed beforeresult?, so the depth is restored on error paths.
1 participant
What does this PR do?
Fixes a CSS minifier OOM family found by fuzzing (signature
oom:css:_RNvXs1_NtNtC8bun_core4util2ioINtNtC5alloc3vec3VechENtB5_5Write9write_allC11bun_css_jsc|_RNvMNtC7bun_css10css_pa…, i.e.<Vec<u8> as Write>::write_allgrowing the output buffer under acss_parserframe, on 1.4.0-canary.1+d2a6506df).The minimized 45-byte input from the report (
a:lang(en, fr) { color: 0red; }with a chrome 80 target) does not reproduce on its own: its downlevel output is 83 bytes and a fixed point under re-minification, verified at the exact reported revision under ASAN. Like the sibling report fixed in #31277, the published repro lost the context that makes the family blow up. The real trigger is the same:lang()/::part()downlevel path inside nested rules with fat selectors.Root cause
The selector-expansion limits added for earlier fuzz reports bound expansion by count, not bytes:
MAX_SELECTOR_EXPANSION(65,536) counts how many selectors compiling nesting away produces (css: bound selector-list expansion when compiling nesting for older targets #31277, css: bound selector expansion through nesting-holding at-rules #31482).MAX_NESTING_EXPANSIONS(65,536) counts how many&parent-selector substitutions the printer performs per rule prelude (css: cap&parent-selector expansion when compiling nesting for older targets #31276).Each counted unit's serialized size is input-controlled: long identifiers, multi-argument
:lang()(which downlevels to:is(:lang(a), :lang(b))/:-webkit-any(...)for old targets),::part(), raw custom pseudo-class arguments. Count x size stays under every cap while the output explodes. On current main (all existing caps intact, release build):The same shapes reach the same paths through
bun build --minify(default browser targets compile nesting away). In a fuzzing session with a memory cap, allocation fails while the printer grows its outputVec, which is exactly the reported crash stack.Fix
Budget both expansions by estimated serialized bytes, alongside the existing count caps (checked second, so ordinary-sized selectors keep reporting the existing count errors):
charge_selector_expansionnow also chargesmultiplier x (own list weight + ancestor chain weight)againstMAX_SELECTOR_EXPANSION_BYTES(64 MB, matchingMAX_PREFIX_EXPANSION_BYTESfrom css: bound vendor-prefix fan-out when serializing nested rules #31642). The chain weight is tracked next to the multiplier inMinifyContext. Selector weights are estimated byselector_list_weight(identifier/payload byte lengths, recursing into:is()/:not()/:nth-child(of)/custom pseudo-class token lists). Reported as a newselector_expansion_bytes_limit_exceededminify error.serialize_nestingmeters the byte span each outermost&substitution writes into a stylesheet-widePrinter.nesting_expansion_bytes, bounded byMAX_NESTING_EXPANSION_BYTES(64 MB) with the existingmaximum_nesting_expansionprinter error.The weight walk only runs for rules inside compiled nesting (
selector_expansion_multiplier > 1or a bumping level), so flat stylesheets and modern targets pay nothing.Relationship to #31913 (same fuzzing campaign, different signature
hang:css:…PropertyIdTag…): that PR bounds the payload (declarations, nested subtrees, token lists) deep-cloned when splitting target-incompatible selector lists. It does not bound the printer's&substitution path (the second repro above stays exponential under it: no splits happen, the multiplier stays 1, and no clones are made), and its declaration-payload shapes stay under this PR's selector-weight budgets, so the two are complementary. Textual conflicts inerror.rs/rules/mod.rs/css_parser.rsare expected with whichever lands second.Verification
test/js/bun/css/selector-expansion-bytes.test.ts. On an unfixed build the three blowup tests fail (the 265 MB / 120 MB / 107 MB outputs are returned instead of errors); on this branch all 7 pass.minifyTest,_test, andprefixTestwith the chrome 80 target.:lang()and:dir()downlevel outputs are byte-identical, and the thin-selector deep-nesting case still reports the existing 65,536-selector count error.test/js/bun/css/(2135 pass; the 5css-fuzz.test.ts/fuzz ansi256debug-build timeouts are pre-existing, reproduced on an unfixed debug build) andtest/bundler/css/(167 pass).