You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(xml): one lexical pass before the parser - #381
Every entry point takes at most one lexical pass before the backend parse. For xmloxide on a document without a DOCTYPE there is none: the parser enforces the node, depth and namespace-binding limits as it reads. For documents with a DOCTYPE and for roxmltree, the strict scan that records source positions checks the limits on each event.
SAML 16384 verify user CPU: 24.1 -> 21.8 ms.
Changes
LimitCounter holds the limit semantics (the root is a node, a run of character data is one node, whitespace outside the document element is none; an element applies its declarations, then counts, then nests). The parser, the scan and the standalone pass all count through it.
The scan takes a ScanObserver. LimitPass observes it, expands internal entities in content and attribute values, and applies DTD attribute defaults. The DOCTYPE is walked once, for entities, defaults and the attribute types the positions need.
The standalone pass remains only for the XSLT input parser, which reads no positions.
Whether a document requires its DTD comes from the one parse; the second parse with DTDs disallowed is gone.
Each parse charges a parse-work allowance of twice the source (the pass and the parser).
Typed refusals are unchanged. In malformed XML, a syntax error that precedes the limit point in document order is now reported before the limit refusal, because the checks run in document order.
Testing
Tests, lints, formatting and the per-feature profiles pass on macOS; CI runs the full platform and feature matrix. New tests sweep every limit from zero, for the parser path, for DOCTYPE documents and for roxmltree. Each case must refuse where the standalone pass refuses, from the parser or scan itself, or accept with the same peak bindings. Policy entry points are checked at exact and one-over boundaries.
Reviewing files that changed from the base of the PR and between 7427fb4 and 78068cf.
Files selected for processing: 13
scripts/patches/xmloxide-0.5.0.patch
src/document.rs
src/document/parser_limit_tests.rs
src/document/scan_limit_tests.rs
src/parser_xmloxide/parser/mod.rs
src/parser_xmloxide/parser/xml.rs
src/xml/dom/limits.rs
src/xml/dom/mod.rs
src/xml/dom/preflight.rs
src/xml/dom/tree.rs
src/xml/dom/xmloxide.rs
src/xmlenc/parse.rs
tests/owned_document_pipeline.rs
No actionable comments were generated in the recent review.
Walkthrough
XML document parsing now enforces resource limits through parser-side accounting or an observed lexical scan. The checked path returns namespace-binding metrics and DTD provenance. Tests cover limit boundaries, refusal behavior, and parse-work accounting.
The scanner reports XML events to observers. LimitPass accounts for those events, entity expansions, DTD declarations and defaults, and namespace bindings.
The xmloxide parser applies limits while parsing. The backend distinguishes limit refusals from other parse failures and returns peak namespace-binding counts. The parser patch also updates storage-aware parsing and related parser behavior.
Document parsing selects parser-side enforcement or an observed scan. The shared path returns parsing metrics and DTD provenance from the semantic parse.
Limit boundaries and parse-work tests src/document/*, src/document.rs, src/xmlenc/parse.rs, tests/owned_document_pipeline.rs
Tests compare scan and parser outcomes, check limit boundaries and refusal precedence, verify DTD provenance, and calculate parse-work allowances from selected parser passes.
sequenceDiagram
participant XmlDocument
participant parse_checked
participant LimitPass
participant XmloxideBackend
participant Document
XmlDocument->>parse_checked: input and configured limits
alt parser-side enforcement is selected
parse_checked->>XmloxideBackend: parse with limits
XmloxideBackend->>Document: projected document and peak bindings
else observed scan is selected
parse_checked->>LimitPass: observe lexical events
LimitPass->>parse_checked: accounting result or refusal
parse_checked->>Document: parse with selected backend
end
Loading
Possibly related PRs
structured-world/xml-sec#289: Changes xmloxide parsing, lexical preflight, DOM projection, and parser storage used by this change.
structured-world/xml-sec#378: Builds XSLT node-set input with parser-equivalent limits and parse-work accounting shared with this change.
structured-world/xml-sec#312: Changes entity-expansion accounting in parser and lexical limit paths touched by this change.
Merge Risk: Minimal · previous review 7427f
No actionable merge-blocking issue remains after normal checks.
Pre-merge checks: 2 · 2 · 1
Failed checks: 2 warnings · 1 inconclusive
Check name
Status
Explanation
Resolution
Linked Issues check
Active direct target: #347 (open). No other active target applies. No parent context was supplied.
Objective group A [#347]: one lexical pass before the backend parse; limits enforced before DOM cons…
1. Restore the prior accept or refuse result for DTD-capable documents under xml-backend-xmloxide. Restore the prior parse-work allowance and the refusal assertion in tests/owned_document_pipeline.rs. Alternatively, record an XML-specificat…
Docstring Coverage
Docstring coverage is 50.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Out of Scope Changes check
The changed source areas support the limit merge of #347: src/xml/dom/limits.rs, src/xml/dom/preflight.rs, src/parser_xmloxide/parser/xml.rs and mod.rs, src/xml/dom/xmloxide.rs, src/xml/dom/tree.rs, s…
Provide the reason for the public module removals, the public signature changes, and the XML 1.0-only version rejection in scripts/patches/xmloxide-0.5.0.patch. Identify the hunks that make these changes. Show that they are required by the …
Passed checks: 2 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "perf(xml): one lexical pass before the parser" matches the main change. The PR centralizes parser limit checks into one lexical scan before backend parsing, removes the second DTD-provenanc…
Full details: Title check
Explanation
The title "perf(xml): one lexical pass before the parser" matches the main change. The PR centralizes parser limit checks into one lexical scan before backend parsing, removes the second DTD-provenance parse, and sets parse-work allowance to twice the source size. The title names the performance goal and the scope (XML), which fits the objectives. It omits the limit-enforcement detail, but that detail is secondary to the headline change.
Full details: Linked Issues check
Explanation
Active direct target: #347 (open). No other active target applies. No parent context was supplied.
Objective group A [#347]: one lexical pass before the backend parse; limits enforced before DOM construction; exact-boundary and one-over tests through public entry points.
Met, per the PR description and change summary. Limit tests are in src/document/parser_limit_tests.rs and src/document/scan_limit_tests.rs. The tests were not run. Assessment rests on the description and summary.
Objective group B [#347]: accepted or refused documents and typed errors unchanged, unless a difference is justified by the XML specification and recorded.
Not met (acceptance outcome). Verified by diff of tests/owned_document_pipeline.rs, merge base to head. Before this PR, under xml-backend-xmloxide, a DTD-capable document was refused at a parse-work allowance of 2 * xml.len(); actual work was 3 * xml.len(). At head, the same document is accepted at 2 * xml.len() and refused at 2 * xml.len() - 1. The second parse was removed and the allowance was set to 2 *. This changes the accept or refuse result. The PR and #347 give no XML-specification justification.
Not met (typed error). The PR states that, in malformed XML, a syntax error preceding the limit point is now reported before the limit refusal. No XML-specification justification is given.
Objective group C [#347]: existing rejection, donor, differential and adversarial suites pass unchanged.
Not met as reported. Existing expectations were edited: the refusal assertion in tests/owned_document_pipeline.rs was replaced; the parser pass-count tests were changed; src/xmlenc/parse.rs replaced the fixed multiplier 3 with selected_parser_passes(). The PR does not report the rejection, donor, differential or adversarial suites as passing unchanged.
Objective group D [#347]: parse time and allocations on the SAML shape measured before and after on a quiet host.
Partly met. The PR reports SAML 16384 verify user CPU, 24.1 to 21.8 ms, on macOS. No allocation measurement and no quiet-host statement are given. This is a measurement task. It is noted but is not the basis of the FAIL.
Evidence used: PR description, change summary, base-to-head diff of tests/owned_document_pipeline.rs, and previous assessment. The full diff was not read. CI results were not inspected.
Resolution
Restore the prior accept or refuse result for DTD-capable documents under xml-backend-xmloxide. Restore the prior parse-work allowance and the refusal assertion in tests/owned_document_pipeline.rs. Alternatively, record an XML-specification justification for the new boundary in #347 and in the PR, and obtain approval from the issue owner.
Restore the prior error precedence for malformed XML that also exceeds a limit. Alternatively, record an XML-specification justification for the new precedence in #347 and in the PR.
Run the rejection, donor, differential and adversarial suites with no expectation edits. Report the results. For each remaining edited expectation (tests/owned_document_pipeline.rs, parser pass-count tests, src/xmlenc/parse.rs), show that it changes no accept, refuse or error outcome.
Report allocation counts and parse time before and after for the SAML 16384 shape on a quiet host.
Full details: Docstring Coverage
Explanation
Docstring coverage is 50.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 61 functions across 12 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Out of Scope Changes check
Explanation
The changed source areas support the limit merge of #347: src/xml/dom/limits.rs, src/xml/dom/preflight.rs, src/parser_xmloxide/parser/xml.rs and mod.rs, src/xml/dom/xmloxide.rs, src/xml/dom/tree.rs, src/xml/dom/mod.rs, and src/document.rs, with their tests. The behavior of src/xmlenc/parse.rs and tests/owned_document_pipeline.rs is assessed under the linked-issue result.
The vendored patch scripts/patches/xmloxide-0.5.0.patch is not confirmed. The visible hunks contain limit hooks and storage-generic refactoring, which relate to #347. The change summary also lists changes that limit enforcement does not obviously require:
Removal of public modules from xmloxide: async_xml, catalog, css, ffi, html, html5, reader, sax, serde_xml, util, xinclude, xpath, relaxng, schematron, xsd, c14n.
Changed public signatures on ParseError, NamespaceResolver, ParserInput and XmlParser.
Rejection of XML versions other than 1.0.
The truncated patch output did not show the hunks for these changes, such as the lib.rs and module export hunks. The summary gives no reason for them. Their connection to the limit merge cannot be established. The previous PASS did not cover the vendored patch, so it is not carried forward.
Resolution
Provide the reason for the public module removals, the public signature changes, and the XML 1.0-only version rejection in scripts/patches/xmloxide-0.5.0.patch. Identify the hunks that make these changes. Show that they are required by the storage-generic refactoring or by the limit merge. If they are not required, remove them or move them to a separate change.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 2d9a6f5 and 7427fb4.
Files selected for processing: 1
scripts/patches/xmloxide-0.5.0.patch
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…e it
A document without a DOCTYPE parsed with xmloxide no longer takes the
lexical limit pass before the parser. The parser counts semantic nodes,
element nesting and in-scope namespace bindings as it reads, with the
pass's semantics (the root is a node, a run of text, references and CDATA
is one node, whitespace outside the document element is none; an element
applies its declarations, then counts, then nests), and refuses at the
first limit it passes before allocating the node. The refusal keeps the
document error and the peak namespace bindings the pass reported.
The absolute nesting ceilings are checked after the policy limits, so a
policy depth equal to them still reports its own refusal. Parse work no
longer charges a pass that does not run. Documents with a DOCTYPE, which
need entity expansion bounded first, and the roxmltree backend keep the
limit pass.
Tests: every limit swept from zero over documents covering every counted
construct must refuse where the pass refused, from the parser itself, and
accept with the same peak; precedence when one element passes several
limits; exact and one-over boundaries through the policy entry point.
SAML 16384 verify: user CPU 24.0 -> 21.8 ms, minimum 28.3 -> 26.1 ms.
Part of #347
A document the parser does not read positions from by itself (one with
a DOCTYPE, or any document for the roxmltree backend) took two lexical
passes before the parser: one enforcing the policy limits, one recording
source positions and DTD attribute types. The strict scan now takes an
observer that checks the limits on each event before recording it, so
one pass serves both, on the native and the accounted entry points and
on the public DOM parse.
- The limit pass becomes LimitPass: its counts go through the same
LimitCounter the parser uses, and it expands internal entities in
content and attribute values as before. It also runs by itself over a
whole document for the XSLT input parser, which reads no positions.
- The DOCTYPE is visited once: entities, attribute defaults and the
attribute types the positions need come from the same walk.
- Whether a document requires its DTD comes from that one parse instead
of parsing it again with DTDs disallowed.
- Every parse now charges its allowance, the pass and the parser, of
twice the source; the separate pass and the provenance parse are gone.
Tests: for DOCTYPE documents (entities expanding markup and namespace
declarations, attribute defaults, references in attribute values) and
for roxmltree, every limit swept from zero must refuse where the
separate pass refuses, from the scan itself, and accept with the same
peak bindings; DTD provenance from the one parse; exact parse-work
allowances.
Part of #347
A start tag's omitted attributes take their first declared default. The
scan found a default an earlier one or the tag already supplied by
comparing against every preceding default, which is quadratic in the
defaults of one element and charged nothing to the work allowance. It
now looks names up in an index the storage meters, as the separate
limit pass does.
Regression test: the_scan_charges_the_entity_work_the_limit_pass_charges
compares the entity expansion work of the scan and of the separate pass,
over a DTD that declares one attribute's default twice.
Part of #347
The module gate names xmlenc as well as xmldsig, which reads as if an
xmlenc-only build could compile it without the verification policy.
xmlenc enables xmldsig through xmldsig-sign, so no such build exists.
Part of #347
The maintained xmloxide source gained the parse that enforces limits as
it reads, but the recorded delta against xmloxide 0.5.0 still described
the previous source, so the kernel verification failed. The delta is
regenerated from the maintained files.
Part of #347
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
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
verifyuser CPU: 24.1 -> 21.8 ms.Changes
LimitCounterholds the limit semantics (the root is a node, a run of character data is one node, whitespace outside the document element is none; an element applies its declarations, then counts, then nests). The parser, the scan and the standalone pass all count through it.ScanObserver.LimitPassobserves it, expands internal entities in content and attribute values, and applies DTD attribute defaults. The DOCTYPE is walked once, for entities, defaults and the attribute types the positions need.Testing
Tests, lints, formatting and the per-feature profiles pass on macOS; CI runs the full platform and feature matrix. New tests sweep every limit from zero, for the parser path, for DOCTYPE documents and for roxmltree. Each case must refuse where the standalone pass refuses, from the parser or scan itself, or accept with the same peak bindings. Policy entry points are checked at exact and one-over boundaries.
Closes #347
Summary by CodeRabbit