Skip to content

🐛 fix(css): keep merge property scan in the body - #972

Merged
gaborbernat merged 1 commit into
tox-dev:mainfrom
gaborbernat:fix/css-minify-unbounded-reads
Oct 2, 2026
Merged

gaborbernat merged 1 commit into
tox-dev:mainfrom
gaborbernat:fix/css-minify-unbounded-reads

Conversation

@gaborbernat

Copy link
Copy Markdown
Member

Before the CSS minifier merges a rule into an earlier one, it checks that no rule in between sets a conflicting property, reading the names back out of each rendered declaration body. css_body_next_prop searched for a declaration's : with no length bound. When the last ;-separated piece of a body held no :, as in [:;a, c:d[;e] or c:d\;e, the search ran past the body into adjacent heap memory until some later : turned up. clean.minify_css and <style> minification under Minify(minify_css=CSSMinify()) then crash.

The scan now reads one declaration at a time and stops at the body's end. A ; at paren depth 0 ends a declaration, its name runs to the first :, and a piece with no : counts as one name. The parser keeps ; inside [...] blocks and escapes, so the scan can split one declaration into several pieces; an extra name can only add a conflict and block a merge. A stray ) now leaves the depth at 0, as css_read_until does.

The old scan also crossed a ; while looking for the next :, which hid properties. In .t{color:blue}.u{c:d[;e];color:red}.t{color:green} it read e];color as one name, missed that .u sets color, and folded the last .t into the first, so red beat green on an element matching both. A stray ) in .u{c:d);color:red} did the same. Both now keep the three rules apart.

The other minifiers take property names from their parser instead of rescanning output. tdewolff/parse ends a declaration on ; or } at nesting level 0 or at EOF and reports one without a : as an error grammar. rust-cssparser calls expect_colon inside parse_until_after(Semicolon), which skips nested blocks and stops at EOF.

css-tree, which csso uses, checks eof and requires the colon with eat(Colon). lightningcss merges only adjacent rules and compares parsed Property values. clean-css records the name at the : inside a loop bounded by the source length.

cbuf_put_run also handed memcpy a NULL source for an empty run. A nested rule with no selector (a{{b:c}}) and a declaration with no value (a{b:}) each render a buffer that never allocated. Its length is 0, because a css_buf grows its length only after a successful reserve, so nothing was read; glibc still declares memcpy's source non-null, which makes the call undefined. cbuf_put_run now returns early on an empty run, as sbuf_put_run already does.

This fixes the CSS minifier heap over-read on a declaration piece without a colon, tracked privately in GHSA-mg53-v965-qg5r.

@codspeed

codspeed Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 581 untouched benchmarks
⏩ 32 skipped benchmarks1


Comparing gaborbernat:fix/css-minify-unbounded-reads (0244ff7) with main (c795023)

Open in CodSpeed

Footnotes

  1. 32 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@gaborbernat
gaborbernat force-pushed the fix/css-minify-unbounded-reads branch 2 times, most recently from 2f7cb78 to 92db80f Compare October 1, 2026 23:49
@gaborbernat
gaborbernat enabled auto-merge (squash) October 2, 2026 00:08
@gaborbernat
gaborbernat force-pushed the fix/css-minify-unbounded-reads branch from 92db80f to b7b5796 Compare October 2, 2026 00:21
Before merging a rule into an earlier one, the minifier reads property
names back out of each rendered body to find conflicts. The scan searched
for a declaration's ':' with no length bound, and the parser keeps a ';'
inside [...] blocks and escapes, so a piece such as the tail of c:d[;e]
has no ':' and sent the search past the heap buffer.

The scan now reads one declaration at a time inside the body and counts a
colon-less piece as a name, which can only block a merge. Ending each
piece at its own ';' and clamping a stray ')' at depth 0 also stops a
later property from hiding in the previous name, which had let a merge
move a rule past one that overrides it.

cbuf_put_run now skips an empty run, whose source is a NULL buffer that
glibc's memcpy declares non-null, as sbuf_put_run already does.
@gaborbernat
gaborbernat force-pushed the fix/css-minify-unbounded-reads branch from b7b5796 to 0244ff7 Compare October 2, 2026 00:24
@gaborbernat
gaborbernat merged commit b1f2895 into tox-dev:main Oct 2, 2026
52 checks passed
@gaborbernat
gaborbernat deleted the fix/css-minify-unbounded-reads branch October 2, 2026 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant