HTMLRewriter: fix quadratic selector-VM stack scans on hostile nesting - #35930
HTMLRewriter: fix quadratic selector-VM stack scans on hostile nesting#35930robobun wants to merge 2 commits into
Conversation
lol-html's selectors_vm open-element stack had two O(depth) per-token scans that go quadratic on crafted input: - pop_up_to() did a full rposition scan for every end tag; a stray end tag that matches nothing still walked the whole stack. A ~700KB doc of '<a></b>'.repeat(100k) pinned a core for ~25s. - Descendant-combinator matching walked every ancestor's hereditary jumps for every start tag, re-executing the same address range once per matching ancestor. 'div div' over '<div>'.repeat(40k) took ~30s. Fixes, shipped as a vendor patch until they land upstream: - Stack now keeps a per-name open count so pop_up_to rejects a non-present name in O(1). When the name is present the rposition scan length equals the drain length, so the hit path was already amortised linear. - Stack now keeps a flat, deduplicated list of active hereditary jump ranges tagged with the shallowest depth that introduced each. Descendant matching iterates that list (bounded by program size) instead of every ancestor, and pop_up_to prunes it by depth. Matching semantics are unchanged: re-running the same range against the same start tag is a no-op after payload dedup. All 147 lol_html lib tests and the integration selector-matching fixtures pass.
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 10:24 AM PT - Jul 26th, 2026
❌ @robobun, your commit e96793b has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35930That installs a local version of the PR into your bun-35930 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The open-element-stack presence map added in the previous commit keys on LocalName, whose PartialEq compares the Bytes variant case-insensitively while the derived Hash did not. That made <My-Widget>...</my-widget> miss the presence check and skip the pop, so onEndTag never fired for mixed-case custom elements (and any name that falls off the packed LocalNameHash path). Give LocalName a manual Hash that ASCII-lowercases the Bytes arm so Hash agrees with Eq. This also fixes the same latent contract violation in the existing TypedChildCounterMap used for :nth-of-type. Adds a guard test for the mixed-case end-tag match.
There was a problem hiding this comment.
The Hash/Eq issue from my earlier review is fixed in e96793b — LocalName's manual Hash now case-folds the Bytes arm, and cross-variant PartialEq is always false so hashing the discriminant is sound. I didn't find further issues, but this is a substantive algorithmic rewrite of lol-html's descendant-combinator matching applied as a vendor patch, so it's worth a human look.
What was reviewed:
active_hereditary_jumpsprune-by-depth: the stored depth is always the shallowest contributor andpop_up_todrains a suffix, soretain d < indexcannot drop a range that a surviving item still needs.- Bailout recovery:
HereditaryJumpPtr.instr_set_idxindexesactive_hereditary_jumps, which is stable between the try-without-attrs capture and the with-attrs recovery (no push/pop in between). open_name_counts:push_itemupdates only after the fallibleitems.push, so the map never desyncs onMemoryLimitExceededError;RawEntryMut/HashMapare already imported from hashbrown instack.rs.
Extended reasoning...
Overview
Vendor patch to cloudflare/lol-html fixing two O(depth²) hot loops in the CSS selector VM (selectors_vm/{mod,stack}.rs) plus a case-folding Hash impl for LocalName, wired via scripts/build/deps/lolhtml.ts (existing patches: mechanism, same pattern as lsquic/libuv/zlib). Three regression tests added to test/js/workerd/html-rewriter.test.js including one guarding the Hash/Eq fix from my earlier review.
Security risks
The motivation is itself a DoS-class fix (sub-MB hostile HTML → CPU pin). I checked that the new bookkeeping doesn't introduce a replacement DoS: active_hereditary_jumps length is bounded by distinct address ranges in the compiled selector program (attacker doesn't control selectors), and open_name_counts size is bounded by concurrently-open distinct names, itself bounded by the memory-limited items stack. No untrusted input reaches an allocation size or index computation.
Level of scrutiny
High. This replaces the per-ancestor hereditary-jump walk with a deduplicated flat set and relies on the argument that re-executing an address range against the same start tag is idempotent through add_execution_branch. I traced add_execution_branch at the pinned upstream commit: matched_payload is HashSet-deduped so match_handler fires once per payload regardless of execution count, and the additive pushes to ctx.stack_item.{jumps,hereditary_jumps} only feed future matching where the same idempotence applies. That argument holds for the shapes I walked (div div, div div div, div div > span), but it's subtle enough — and the patch will need carrying across upstream bumps — that someone familiar with lol-html's compiler output should confirm it for the full selector grammar.
Other factors
My previous finding (case-sensitive derived Hash vs. case-insensitive PartialEq on LocalName::Bytes) is resolved and covered by both a Rust unit test (bytes_variant_hash_matches_case_insensitive_eq) and a JS test (<My-Widget>…</MY-WIDGET> fires onEndTag). PR reports cargo test in the vendored crate (147 lib tests + integration selector fixtures) and the full 71-test HTMLRewriter suite pass under debug+ASAN. The new perf tests use Buffer.alloc(n, fill) per repo convention and run ~1.4s under ASAN.
|
CI is green except for
|
|
@robobun submit this as a PR to lol-html instead. |
|
Opened upstream as cloudflare/lol-html#331 (rebased onto their v3.0.0 main, 180 lib tests + integration fixtures + clippy all pass). Happy to close this vendor-patch PR if you'd rather wait for the upstream release and bump |
|
@robobun simplify your PR in cloudflare/lol-html#331. Remove comments that are noise. Reduce the non-test LOC in the PR. |
|
Simplified cloudflare/lol-html#331 in e1daebc: trimmed comments to one line each, collapsed the |
2 participants
What
Fixes two O(depth^2) paths in lol-html's
selectors_vmopen-element stack that let sub-MB hostile HTML pin a CPU core throughHTMLRewriter. Applied as a vendor patch tocloudflare/lol-html.Repro
Any
.on(selector, ...)registration (even one that never matches) enables the selector VM, so any surface that feeds untrusted HTML toHTMLRewriterwas exposed.Cause
selectors_vm/stack.rspop_up_todiditems.iter().rposition(|i| i.local_name == name)for every end tag. A stray end tag that matches nothing still scanned the entire open-element stack: N unclosed opens plus N stray closes is N full scans.selectors_vm/mod.rstry_exec_hereditary_jumps_without_attrswalked every ancestor for every start tag, re-executing the same hereditary-jump address range once per matching ancestor. Fordiv divover N nested<div>, every item carries the same range so the walk is O(depth) per tag.Fix
Stacknow maintains:open_name_counts: HashMap<LocalName, usize>sopop_up_torejects a name not on the stack in O(1). When the name is present therpositionscan length equals the subsequent drain length, so total scan work over a document is bounded by total pushes.active_hereditary_jumps: Vec<(AddressRange, usize)>, the distinct hereditary-jump ranges currently contributed by any open item, each paired with the shallowest depth that introduced it. Descendant matching iterates this flat list (bounded by program size, not depth);pop_up_toprunes it by depth. The per-itemhas_ancestor_with_hereditary_jumpsflag and theHereditaryJumpPtr.stack_offsetresume coordinate are gone.Re-executing the same address range against the same start tag was already a no-op after payload dedup in
add_execution_branch, so dedup-once execution is observationally equivalent.Verification
USE_SYSTEM_BUN=1 bun test test/js/workerd/html-rewriter.test.js -t "pathological"times out on both cases (>5s default).bun bd test test/js/workerd/html-rewriter.test.jspasses: 439ms and 1.4s under debug+ASAN; all 71 existing HTMLRewriter tests pass.cargo test --libinvendor/lolhtml: 147 passed.cargo test --features integration_test: 3 passed including the selector-matching fixture suite.Note: the fix lives in
patches/+scripts/, notsrc/, so agit stash -- src/ packages/fail-before check won't revert it. TheUSE_SYSTEM_BUN=1run above is the fail-before proof.[stamp-90s] gate passed · iteration 1 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file