css: bound the payload cloned when splitting target-incompatible selectors#31913
css: bound the payload cloned when splitting target-incompatible selectors#31913robobun wants to merge 6 commits into
Conversation
…ctors Splitting a selector list that is incompatible with the browser targets deep-clones the rule's declarations and entire nested-rule subtree once per split-off selector. Under CSS nesting the subtree at each level already contains the clones made at deeper levels, so the cloned payload compounds exponentially with depth. The existing selector-count cap bounds how many rules this produces but not their size, so a few hundred KB of input could clone and print gigabytes while staying under it. Charge a weight estimate (rules, declarations, selector components, raw tokens and their text) against a 64 MB budget before each split and report a minify error past it, mirroring the selector-count cap. Also widen the printer's indent counter from u8 to u32: pretty-printing a valid stylesheet with 128+ nested levels overflowed it, a panic in debug builds.
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. WalkthroughThe PR implements a clone-weight budget to prevent CSS minifier pathological expansion when splitting incompatible nested selectors. It adds weight estimation for cloned AST payloads, accumulates weights during minification, enforces an upper limit, and widens the printer indent type to support deeper nesting without overflow. ChangesClone-weight budget mechanism
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Heads up: #31916 fixes the OOM half of this fuzz family (signature |
The weight walk only counted plain ident-like token text, so a dimension token's unit, a dashed ident, or a var()/env()/function name carrying the same input-sized text was charged the flat token constant and bypassed the budget. Count every token variant that stores its text inline. url() stores only an import-record index; its text is not reachable during minify, so like parsed leaf values it stays on the selector-count bound (noted in the module doc).
Custom-property names (both the Custom and Unknown spellings) and selector component text (classes, ids, element and attribute names, attribute values, namespace urls, ::part names, unknown pseudo-class and pseudo-element names, :lang lists) carry input-sized borrowed text that is re-emitted per split clone, but were charged only the flat constants. Moving a large payload from a property value into the property name or a nested rule's selector bypassed the budget. Charge their text lengths. CSS-modules local references print a symbol-table name rather than inline text and keep the flat charge.
…ctors Sweep the remaining inline-text carriers reachable inside a style rule's subtree: view-transition part names, ::cue()/::cue-region() and :local()/:global() wrapped selectors, unknown at-rule names, and the names, preludes, and conditions of every at-rule that can nest inside a style rule (@media queries, @supports conditions, @container names and conditions including style() queries, @layer names, @scope selector lists). Each of these amplified to 33+ MB from ~30 KB of input while charging only the flat constants.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 336-349: The test spawns a Bun process into variable proc with
stdout set to "pipe" but never consumes proc.stdout, risking a deadlock; update
the await that collects outputs to concurrently drain stdout as well as stderr
and exit by replacing the Promise.all([...]) that currently only awaits
proc.stderr.text() and proc.exited with one that also awaits proc.stdout.text()
(e.g., Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited()])),
ensuring you still check proc.signalCode and use the captured stdout/stderr as
needed.
- Around line 401-409: The test currently masks all unexpected stderr by using
stderr.includes("panic") ? stderr : "" which hides other failures; update the
expectation to assert a concrete stderr contract instead: replace the
conditional stderr expression (the use of stderr.includes("panic")) so the
expected object checks for the exact stderr value you expect (e.g., an empty
string) and keep the indentedLength and exitCode checks (refer to
indentedLength, parseInt(stdout, 10), exitCode, and stderr.includes in the
existing snippet) so the test fails on any unexpected stderr output rather than
only on "panic".
🪄 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: 8e8dfc79-605a-43d2-85ff-e11cd2289cd7
📒 Files selected for processing (5)
src/css/css_parser.rssrc/css/error.rssrc/css/printer.rssrc/css/rules/mod.rstest/js/bun/css/nested-selector-list-expansion.test.ts
A namespaced attribute selector ([ns|attr]) stores its prefix and url on the boxed attribute struct, separate from the type-selector namespace components that were already charged; a large prefix bypassed the budget. Charge both fields. Also drain stdout in the bun build clone-limit test.
Same pattern as the clone-limit test fixed in the previous commit.
There was a problem hiding this comment.
All four rounds of clone-weight coverage gaps I flagged are now addressed and the bug hunter found nothing new, but ~400 lines of new heuristic weight-estimation logic in the production CSS minifier path (plus the noted overlap with #31916) is worth a human pass before merging.
Extended reasoning...
Overview
This PR adds a per-stylesheet clone-weight budget (MAX_SELECTOR_SPLIT_CLONE_WEIGHT = 64 MB) to bound the exponential payload duplication that occurs when minify_style_arm splits target-incompatible selector lists under deep CSS nesting. It introduces a ~330-line clone_weight module in src/css/rules/mod.rs that walks rules, declarations, selector components, pseudo-classes/elements, at-rule preludes, and raw token lists to estimate cloned-AST weight; threads a split_clone_weight_total accumulator through MinifyContext; adds a new MinifyErrorKind variant; widens Printer::indent_amt from u8 to u32 to fix a debug-build overflow panic at 128+ nesting levels; and adds ~190 lines of regression tests including a 19-case test.each matrix and an embedded fuzzer input.
Security risks
None introduced. The change is purely defensive — it converts an adversarial-input OOM/hang (449 KB → 1 GB output, 2.1 GB RSS) into a catchable error. Output below the budget is asserted byte-for-byte unchanged. No new parsing, no new external input handling, no auth/crypto/permissions surface.
Level of scrutiny
This warrants human review. The clone_weight module is a heuristic that must enumerate every AST variant carrying input-sized borrowed text; missing one reopens the OOM via that vector. Over four review rounds I found and the author confirmed/fixed gaps for Token::Dimension units, DashedIdent/var()/env() names, custom-property names, selector component text (class/id/type/attribute), view-transition part names, ::cue()/:local()/:global() wrapped selectors, unknown-at-rule names, nestable at-rule preludes, and finally attribute-selector namespace prefixes — the round-3 "full sweep" claim was followed by a round-4 miss. The author has since re-walked struct fields, the test matrix covers each fixed vector, and the bug hunter found nothing this pass, so I believe coverage has converged. But the iteration history itself is the signal that a human should sanity-check the final variant coverage rather than rely solely on bot review.
Other factors
- The minifier runs on every
bun buildwith CSS, so correctness of the "output unchanged below budget" guarantee matters; it is asserted at depth 12 (8,595,441 bytes) plus the existing 1300+ CSS tests pass per the description. - The
u8→u32indent widening and error-variant addition are mechanical and low-risk on their own. - robobun noted PR #31916 touches adjacent lines in
error.rs,rules/mod.rs, andcss_parser.rs; whichever lands second needs a rebase, which a human should coordinate. - No CODEOWNERS entry for
src/css. - All inline comments (mine and CodeRabbit's) are resolved; the one declined CodeRabbit suggestion (strict
stderr === "") was declined with a sound rationale matching established repo conventions.
|
Status summary for reviewers:
|
1 participant
What does this PR do?
Fixes a CSS minifier hang/OOM family found by fuzzing (signature:
hang:css:...PropertyIdTag::name, an 11.6 KB input that hung or ate 6+ GB on the canary it was found on), and a related debug-build panic.Root cause
When a style rule's selector list is incompatible with the configured browser targets (and cannot be collapsed into
:is()),minify_style_arm(src/css/rules/mod.rs) splits it: each split-off selector gets a deep clone of the rule's declarations and its entire nested-rule subtree:Minify recurses bottom-up, so at each nesting level the subtree already contains the clones split off at deeper levels. With two-selector lists the cloned payload doubles per level: exponential time and memory in nesting depth. Caught live under gdb on the fuzzer input: the stack is
CssRuleList::deep_clonerecursing intoTokenListclones fromminify_style_arm.The existing caps don't bound this.
MAX_SELECTOR_EXPANSION(65,536) counts selectors, not bytes; each cloned rule can carry arbitrarily large token lists. On current main (all caps intact, release build):The same shape reaches the same path through
bun build --minify(default browser targets compile nesting away), so the bundler amplifies identically.Fix
Charge a weight estimate of each split's clones against a per-stylesheet budget,
MAX_SELECTOR_SPLIT_CLONE_WEIGHT = 64 MB, before cloning. The weight walk counts every structure whose count or size scales with user input: rules, declarations, raw token lists with their text (including dimension units and dashed idents), custom-property and var()/env()/function/pseudo names, selector text (classes, ids, element, attribute and namespace names, attribute values, wrapped::cue()/:local()/view-transition selectors), and the names, preludes, and conditions of every at-rule that can nest inside a style rule (@media, @supports, @container, @layer, @scope, unknown at-rules). Past the budget, minification reports a catchable error ("Splitting nested CSS rules with selectors unsupported by the configured browser targets duplicates too much CSS."), mirroring the existingMAX_SELECTOR_EXPANSIONandMAX_PREFIX_EXPANSION_BYTES(#31642) bounds. The weight walk only runs for rules that actually split, so stylesheets that never split pay nothing, and its cost is proportional to what it charges.Also fixes a bug in the same input family: the printer's indent counter was a
u8incremented by 2 per nesting level, so pretty-printing a valid stylesheet with 128+ nested levels overflowed it; that is a panic ("attempt to add with overflow") in debug builds. Widened tou32.Not changed: the
>-combinator token floods from the fuzzer input parse and minify in linear time already (consecutive combinators collapse during selector parsing; raw token lists in unknown declarations round-trip linearly); tests now pin that down.Verification
css.test.ts(1093),nested-selector-list-expansion.test.ts,nested-selector-expansion.test.ts,nested-vendor-prefix-duplication.test.ts,doesnt_crash.test.ts,test/bundler/css/(164),test/bundler/esbuild/css.test.ts(53) all pass.test/js/bun/css/nested-selector-list-expansion.test.ts, including a 19-casetest.eachmatrix moving the payload into every chargeable text carrier. On an unfixed build: the clone-budget tests fail (33 MB / 69 MB outputs are returned instead of the error,bun buildwrites the file), and the indent test fails on unfixed debug builds with the overflow panic.(
css-fuzz.test.tsdebug-build timeouts are pre-existing on main and the file is skipped on CI.)