fix: contain parser panics and stack overflows, and decide from the CST - #39
Merged
Merged
Conversation
…o not ship Dependency audit found nothing to upgrade. Every direct and transitive dependency, Elixir and Rust, is already at its own newest published version: `mix hex.outdated` reports all up to date, and `cargo update --dry-run` locks zero packages. That includes the biome crates, and the reason is worth writing down rather than rediscovering. biome_css_parser, biome_css_syntax and biome_rowan are all at 0.5.8, the newest published. We can be there only because this crate does not depend on biome_css_formatter, which is stuck at 0.5.7 and requires biome_css_syntax ^0.5.7 and biome_rowan ^0.5.7 -- adding it would pull the whole graph back a release. That is exactly why igniter_js, which does format CSS, pins its entire biome set to 0.5.7. Cargo.toml now says so, including the condition that keeps it true: our pretty-printer is written against the CST, not routed through biome_css_formatter. The lock also holds biome_diagnostics_categories, biome_diagnostics_macros and biome_markup at 0.5.7, which is their own latest, and biome_unicode_table at 0.5.9. Nothing is held back. Separately, sixteen doc comments across thirteen files cited "ROADMAP §..." for a document that is not in this repository -- it was a planning note, never committed. Every citation is now self-contained prose. A pointer to something a reader cannot open is worse than no pointer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sweep of every comment and doc-comment in the Elixir and Rust sources for references a reader cannot follow, and for history that belongs in the changelog rather than in the code. Removed the planning-document vocabulary. Sixteen comments cited "ROADMAP §..." for a file that was never committed; others carried its rule letters (rule A through rule E), its risk IDs (R1, R2, R4) and its "hard constraint #N" numbering. None of those resolve to anything in this repository. Each is now either stated plainly or replaced by a link to the module that actually holds the behaviour -- "see [`crate::trivia`]" instead of "rule D". Removed the migration commentary. The Parsers.Parser moduledoc explained what the Python/tinycss2 implementation did and how this differs; it now just describes what the module does. Comments in locate.rs and nif.rs justified decisions by reference to the old implementation, and one in locate.rs described a text scanner this branch had already deleted. The changelog is the right place for all of it, and it still says so there. Renamed tests/phase0_roundtrip.rs to tests/roundtrip.rs. The gate is not a phase, and its doc now says why it matters -- every byte-range edit depends on the parse reproducing its input -- rather than where it sat in a plan. No behaviour change. 339 Rust tests, 268 Elixir + 39 doctests, clippy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s in use Both declared floors sat well behind what the lock actually resolved: ex_doc at `~> 0.38` while 0.40.3 was installed, and igniter at `~> 0.5` while 0.8.3 was. `mix hex.outdated` does not catch this. It compares the resolved version against the newest published one, and both resolved fine, so it reported everything up to date -- the stale part was the requirement, not the dependency. No resolution changes; this only stops the declared floors drifting further from reality. Docs still build and the suite is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tack
Audit for anything in the Rust that could take the BEAM down.
Rustler wraps every NIF body in `catch_unwind` and turns a panic into an Erlang
exception in the calling process, so panics are survivable, and there are none
left outside tests anyway: no unwrap, expect, panic!, todo!, unreachable! or
indexing that is not bounds-checked first.
A stack overflow is different. It aborts the process, `catch_unwind` cannot
intercept it, and in a NIF that ends the VM rather than the call. Biome's CSS
parser is recursive descent and rowan's tree drop recurses too, so deeply nested
input overflows. Measured on a 2 MB stack, `@media print{` nesting and `:not(`
nesting both survive 1000 levels and abort at 2000 -- and a dirty scheduler
thread may have less headroom than that.
Every entry point that reaches a parse now measures nesting first, with a plain
byte scan that skips strings and comments and cannot itself recurse, and refuses
past 256 levels. That is an order of magnitude below the observed failure and
far above real CSS, which rarely exceeds ten.
The guard also applies to caller-supplied text, which turned up a second
problem: a selector nested past the limit did not crash, but `ensure_rule` would
splice it in and produce a file we would then refuse to read. Writing what we
will not read is its own bug, so `validate_snippet` enforces the same limit, and
`set_declaration` now validates the selector it may create a rule from.
tests/stack_safety.rs covers both nesting dimensions across every mutating,
read-only and transform op, and asserts ordinary CSS is nowhere near the limit.
Verified end to end through the real NIF: all ten paths refuse, the VM survives,
and ordinary CSS is unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… walk
Three findings from auditing what can take the VM down, and from testing on
Elixir 1.20.
Biome's CSS parser can panic. It asserts that it keeps making progress and
aborts with "The parser is no longer progressing" on some inputs; the fuzzer
found one by splicing comments into generated CSS at arbitrary offsets. Rustler
would turn that into an exception in the calling process, which is survivable
but breaks the promise that unpatchable input returns {:error, reason}.
ParseCtx::try_new now catches it and reports it as one, and every production
parse -- ops, transforms, validate, and the probe parses for a caller's selector
and value -- goes through it. No unguarded parse_css call remains outside ctx.
The property test asserting that spliced input always parses was asserting
something Biome cannot guarantee. It now asserts what this library actually
promises: either a context that round-trips, or a clean error, never a panic
that escapes.
Native CSS nesting was invisible to any query that descends. Biome models
`&:hover { }` as CSS_NESTED_QUALIFIED_RULE holding a CSS_RELATIVE_SELECTOR_LIST,
not CSS_QUALIFIED_RULE, so find_all_rules skipped nested rules entirely and
analyze under-reported: a file with three rules counted one. Both shapes are now
recognised. Top-level queries are unaffected -- nesting stays scoped, and
`.card`'s declarations remain its own.
analyze was quadratic in nesting depth. conditions_of rebuilt the list of every
at-rule in the file once per ancestor, for a node it already held, and
prelude_norm filtered the whole subtree when it only needed the tokens before
the block. Reading each ancestor directly and bounding the token scan with
take_while made depth 128 eleven times faster, 20.6ms to 1.9ms, and the growth
roughly linear.
352 Rust tests, 307 Elixir. Verified on Elixir 1.20.2/OTP 28: no warnings from
this project, credo, sobelow, dialyzer and both formatters clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… seed
The proptest regression file committed in the previous change is the seed store
proptest writes when a property fails. Its header recommends checking it in so
the failing case is replayed for everyone, which is worth having -- but it
carried no SPDX header, so REUSE would have failed on it. Adding the sidecar.
More usefully, it already held the minimal reproducer I had been searching for
by hand. proptest had shrunk the failure to a 24-byte edit, which reconstructs
to:
@layer/*x*/
color: var(--brand);
}
Raw biome_css_parser 0.5.8 panics on that; the guarded path returns Err.
That is now a deterministic test rather than something only a random fuzzer
finds: every entry point is asserted to return Unparseable, and one test asserts
the input still panics the underlying parser -- so if Biome fixes it upstream,
that test fails and tells us the guard may be removable rather than leaving it
in place forever with nobody knowing why.
Worth reporting upstream to Biome.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two project skills, written for an agent rather than a human reader: constraints first, exact commands, and the failure modes with their causes. `release` covers the tag-triggered flow. The point worth encoding is that a tag is the only manual step -- the checksum generation and Hex publish run in CI, and doing them by hand is a sign something is broken. It also lists the four failures seen in practice: the missing permissions block, OTP too old for hex.pm's certificate chain, a poisoned Actions cache, and the 404 you get from building locally before a release exists. `css-codemod` covers the parts an agent gets wrong here. Chiefly: never reprint the tree, and decide from the CST rather than by scanning text -- with a table of what Biome already models, because every text scanner in this codebase was eventually replaced by a node kind. It also records the safety rules that are not obvious, notably that rustler catches panics but a stack overflow aborts and takes the VM down, and the quadratic trap of calling find_all_* inside a per-node loop. Both carry SPDX sidecars so REUSE stays green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardening pass over the Rust side, plus a dependency audit. Nothing here changes the public API; the headline is that two ways of taking the BEAM down were found and closed, both of them in input handling rather than in the codemods themselves.
biome_css_parser0.5.8 asserts that it keeps making progress and panics on some input — the property tests found one, which shrinks to@layer/*x*/\n color: var(--brand);\n}\n. Rustler turns that into an exception in the calling process, which the VM survives but which breaks the promise that unpatchable input returns{:error, reason}. Every parse now goes throughParseCtx::try_new, which catches it and reports it asUnparseable. Pinned as a deterministic test, including one that asserts the input still panics the raw parser — so if Biome fixes it upstream, that test fails and says the guard may be removable.catch_unwindcannot intercept it, so in a NIF it ends the VM rather than the call. Measured on a 2 MB stack, both@media{nesting and:not(nesting survive 1000 levels and abort at 2000. Every entry point now measures nesting first — a byte scan that skips strings and comments and cannot itself recurse — and refuses past 256. Real CSS measures depth 7, including a 60 KB ex_doc stylesheet. The same limit applies to caller-supplied text, which turned up a second problem: a selector past the limit did not crash, butensure_rulewould splice it in and produce a file we would then refuse to read.&:hover { }asCSS_NESTED_QUALIFIED_RULEwith aCSS_RELATIVE_SELECTOR_LIST, notCSS_QUALIFIED_RULE, sofind_all_rulesskipped nested rules entirely andanalyzereported one rule for a file containing three. Top-level queries are unchanged — nesting stays scoped.analyzeis no longer quadratic in nesting depth.conditions_ofrebuilt the list of every at-rule in the file once per ancestor, for a node it already held, andprelude_normscanned a whole subtree when it needed only the tokens before the block. Depth 128 went from 20.6 ms to 1.9 ms, and growth is roughly linear.:nth-child(2n + 1)and:nth-child(2n+1)are one selector, and the scanner kept them distinct. Colour detection matched 148 names against raw text and needed a hack sourl(/red.png)was not read asred; it now matchesCSS_COLOR,CSS_FUNCTIONandCSS_IDENTIFIERnode kinds, and the hack is gone. At-rule preludes and caller-supplied declaration lists are likewise token-derived now.unwrap,expect,panic!,todo!,unreachable!, or unchecked indexing reachable from a NIF.ex_docandigniterdeclared floors that sat well behind what the lock resolved (~> 0.38against 0.40.3,~> 0.5against 0.8.3);mix hex.outdateddoes not catch this, because it compares resolved versions rather than requirements. Everything else, Elixir and Rust, is already at its newest published version.Cargo.tomlnow records why the biome pin is 0.5.8 and what would break it:biome_css_formatteris stuck at 0.5.7 and requiresbiome_css_syntax ^0.5.7, so depending on it would drag the whole graph back a release — which is exactly whyigniter_jspins its set to 0.5.7.ROADMAP §…that is not in this repository, and others carried its rule letters, risk IDs and constraint numbers; all are now self-contained or link to the module that holds the behaviour. Migration commentary about the previous implementation is gone from the code — the changelog is the right place for it..claude/skills: the release flow, and the codemod conventions with the failure modes behind them.Verified on Elixir 1.20.2 / OTP 28.0.2: 355 Rust tests, 307 Elixir,
clippy -D warningsclean, credo, sobelow, dialyzer and both formatters clean, and no warnings from this project. Target coverage was checked againstrustler_precompiled's defaults — we build nine of its ten plus FreeBSD; the missing one is 32-bit ARM, which ash-ci disables for all rustler projects because of a cranelift bug.