From f141a3b71766fedbcd2c5a9470f60c451915f152 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 3 Nov 2026 16:03:45 +0200 Subject: [PATCH 1/5] perf(xml): enforce input limits in the parser instead of a pass before 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 --- src/document.rs | 175 ++++++++++++++++++------- src/document/parser_limit_tests.rs | 202 +++++++++++++++++++++++++++++ src/parser_xmloxide/parser/mod.rs | 10 ++ src/parser_xmloxide/parser/xml.rs | 107 ++++++++++++--- src/xml/dom/limits.rs | 196 ++++++++++++++++++++++++++++ src/xml/dom/mod.rs | 20 ++- src/xml/dom/tree.rs | 27 ++++ src/xml/dom/xmloxide.rs | 83 +++++++++++- 8 files changed, 756 insertions(+), 64 deletions(-) create mode 100644 src/document/parser_limit_tests.rs create mode 100644 src/xml/dom/limits.rs diff --git a/src/document.rs b/src/document.rs index 656e79df..25f3d502 100644 --- a/src/document.rs +++ b/src/document.rs @@ -950,7 +950,8 @@ impl XmlDocument { } let ((peak_namespace_bindings, requires_internal_dtd), preflight_work) = metered(budget, |budget| { - let peak = preflight_document_limits(&xml, settings, budget)?; + let peak = + limit_pass_unless_parser_enforces(&xml, settings, budget, NativeXmlStorage)?; #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] let requires_internal_dtd = document_requires_internal_dtd(&xml, settings, budget)?; #[cfg(not(any(feature = "xmldsig", feature = "xmlenc")))] @@ -2451,7 +2452,7 @@ fn build_cell( budget: Option<&XmlParseWorkBudget>, ) -> Result { let (peak_namespace_bindings, preflight_work) = metered(budget, |budget| { - preflight_document_limits(&xml, settings, budget) + limit_pass_unless_parser_enforces(&xml, settings, budget, NativeXmlStorage) })?; build_cell_after_preflight( xml, @@ -2462,12 +2463,13 @@ fn build_cell( ) } -/// Builds a generation whose preflight already charged `preflight_work`. +/// Builds a generation whose limit pass, if one ran, already charged +/// `preflight_work` and found `peak_namespace_bindings`. fn build_cell_after_preflight( xml: String, settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, - peak_namespace_bindings: usize, + peak_namespace_bindings: Option, preflight_work: usize, ) -> Result { let ((), semantic_work) = metered(budget, |budget| { @@ -2485,16 +2487,17 @@ fn build_cell_after_preflight( fn build_semantic_cell( xml: String, settings: DocumentParseSettings, - peak_namespace_bindings: usize, + preflight_peak: Option, parse_work: usize, ) -> Result { - #[cfg(not(any(feature = "xmldsig", feature = "xmlenc")))] - let _ = peak_namespace_bindings; // Only XMLDSig entry points take over a caller's parse and charge its work. #[cfg(not(feature = "xmldsig"))] let _ = parse_work; DocumentCell::try_new(xml, |source| { - let (document, metrics) = parse_semantic_document(source, settings)?; + let (document, metrics, peak_namespace_bindings) = + parse_semantic_document(source, settings, preflight_peak)?; + #[cfg(not(any(feature = "xmldsig", feature = "xmlenc")))] + let _ = peak_namespace_bindings; debug_assert!( document .descendants() @@ -2520,9 +2523,10 @@ pub(crate) fn parse_borrowed_with_settings_and_budget<'a>( settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, ) -> Result, XmlDocumentError> { - preflight_document_limits(xml, settings, budget)?; + let preflight_peak = + limit_pass_unless_parser_enforces(xml, settings, budget, NativeXmlStorage)?; charge_semantic_parser_work(budget, xml.len())?; - let (document, _) = parse_semantic_document(xml, settings)?; + let (document, _, _) = parse_semantic_document(xml, settings, preflight_peak)?; Ok(document) } @@ -2533,17 +2537,28 @@ pub(crate) fn parse_borrowed_in<'input, 'w, W: structured_asn1::memory::Work>( budget: Option<&XmlParseWorkBudget>, storage: crate::xml::dom::storage::Accounted<'w, W>, ) -> Result>, XmlDocumentError> { - preflight_document_limits_in(xml, settings, budget, storage)?; + let preflight_peak = limit_pass_unless_parser_enforces(xml, settings, budget, storage)?; charge_semantic_parser_work(budget, xml.len())?; - let document = Document::parse_after_limit_preflight_in( - xml, - ParsingOptions { - allow_dtd: settings.allow_dtd, - nodes_limit: settings.nodes_limit, - }, - settings.backend, - storage, - )?; + let options = ParsingOptions { + allow_dtd: settings.allow_dtd, + nodes_limit: settings.nodes_limit, + }; + let document = match preflight_peak { + Some(_) => { + Document::parse_after_limit_preflight_in(xml, options, settings.backend, storage)? + } + #[cfg(feature = "xml-backend-xmloxide")] + None => { + Document::parse_enforcing_limits_in(xml, options, parse_limits(settings), storage) + .map_err(|refused| match refused { + crate::xml::dom::Refused::Limit(refusal) => limit_refusal(refusal), + crate::xml::dom::Refused::Parse(error) => error.into(), + })? + .0 + } + #[cfg(not(feature = "xml-backend-xmloxide"))] + None => unreachable!("only xmloxide enforces limits while it parses"), + }; validate_document_metrics_in(&document, settings.depth_limit, storage)?; Ok(document) } @@ -2556,6 +2571,57 @@ fn preflight_document_limits( preflight_document_limits_in(xml, settings, budget, NativeXmlStorage) } +/// The limit pass before the parser, returning the peak namespace bindings, +/// or `None` for a document whose parser enforces the limits as it reads it, +/// which leaves only the size check before the parse. +fn limit_pass_unless_parser_enforces( + xml: &str, + settings: DocumentParseSettings, + budget: Option<&XmlParseWorkBudget>, + storage: S, +) -> Result, XmlDocumentError> { + if settings.backend.records_positions(xml) { + check_document_size(xml, settings)?; + return Ok(None); + } + preflight_document_limits_in(xml, settings, budget, storage).map(Some) +} + +fn check_document_size(xml: &str, settings: DocumentParseSettings) -> Result<(), XmlDocumentError> { + if xml.len() > settings.max_bytes { + return Err(XmlDocumentError::DocumentTooLarge { + maximum: settings.max_bytes, + actual: xml.len(), + }); + } + Ok(()) +} + +/// The limits a parser enforces as it reads a document under `settings`. +#[cfg(feature = "xml-backend-xmloxide")] +fn parse_limits(settings: DocumentParseSettings) -> crate::xml::dom::ParseLimits { + crate::xml::dom::ParseLimits { + nodes: settings.nodes_limit, + depth: settings.depth_limit, + namespace_bindings: settings.namespace_bindings_limit, + } +} + +/// The refusal for the limit a parse stopped at, as the limit pass reports it. +#[cfg(feature = "xml-backend-xmloxide")] +fn limit_refusal(refusal: crate::xml::dom::LimitRefusal) -> XmlDocumentError { + use crate::xml::dom::LimitRefusal; + match refusal { + LimitRefusal::Nodes => XmlDocumentError::Parse(ParseError::NodesLimitReached), + LimitRefusal::Depth { maximum, actual } => { + XmlDocumentError::DocumentTooDeep { maximum, actual } + } + LimitRefusal::NamespaceBindings { maximum, actual } => { + XmlDocumentError::Parse(ParseError::NamespaceBindingLimitReached { maximum, actual }) + } + } +} + /// Enforce every policy limit on `xml` in one charged lexical pass, for a /// parser outside this crate's DOM that does not enforce them itself. #[cfg(feature = "xslt")] @@ -2574,14 +2640,9 @@ fn preflight_document_limits_in( budget: Option<&XmlParseWorkBudget>, storage: S, ) -> Result { - // This bounded lexical pass is the first parser stage for every entry - // point, before either backend is allowed to construct a DOM. - if xml.len() > settings.max_bytes { - return Err(XmlDocumentError::DocumentTooLarge { - maximum: settings.max_bytes, - actual: xml.len(), - }); - } + // This bounded lexical pass precedes any DOM construction wherever the + // parser does not enforce the limits itself. + check_document_size(xml, settings)?; charge_parse_work(budget, xml.len())?; let mut dtd = InternalDtd::new(storage); let mut expansion_stack = storage.names(); @@ -3948,21 +4009,36 @@ fn find_unquoted_byte(bytes: &[u8], needle: u8, start: usize) -> Option { None } +/// Parses `source` after the limit pass that found `preflight_peak` namespace +/// bindings, or, for `None`, with a parser that enforces the limits as it +/// reads; returns the document's peak namespace bindings either way. fn parse_semantic_document( source: &str, settings: DocumentParseSettings, -) -> Result<(Document<'_>, DocumentMetrics), XmlDocumentError> { - let document = Document::parse_after_limit_preflight_with_backend( - source, - ParsingOptions { - allow_dtd: settings.allow_dtd, - nodes_limit: settings.nodes_limit, - }, - settings.backend, - ) - .map_err(XmlDocumentError::Parse)?; + preflight_peak: Option, +) -> Result<(Document<'_>, DocumentMetrics, usize), XmlDocumentError> { + let options = ParsingOptions { + allow_dtd: settings.allow_dtd, + nodes_limit: settings.nodes_limit, + }; + let (document, peak) = match preflight_peak { + Some(peak) => ( + Document::parse_after_limit_preflight_with_backend(source, options, settings.backend) + .map_err(XmlDocumentError::Parse)?, + peak, + ), + #[cfg(feature = "xml-backend-xmloxide")] + None => Document::parse_enforcing_limits(source, options, parse_limits(settings)).map_err( + |refused| match refused { + crate::xml::dom::Refused::Limit(refusal) => limit_refusal(refusal), + crate::xml::dom::Refused::Parse(error) => XmlDocumentError::Parse(error), + }, + )?, + #[cfg(not(feature = "xml-backend-xmloxide"))] + None => unreachable!("only xmloxide enforces limits while it parses"), + }; let metrics = validate_document_metrics(&document, settings.depth_limit)?; - Ok((document, metrics)) + Ok((document, metrics, peak)) } fn validate_document_metrics( @@ -4347,6 +4423,9 @@ mod entity_ceiling_tests; #[cfg(test)] mod replacement_tests; +#[cfg(all(test, feature = "xml-backend-xmloxide"))] +mod parser_limit_tests; + #[cfg(test)] mod tests { use super::*; @@ -4744,13 +4823,15 @@ mod tests { #[cfg(feature = "xmldsig")] #[test] fn byte_decoding_and_parsing_share_one_work_allowance() { - // UTF-16 decoding consumes its source bytes; preflight and the two + // UTF-16 decoding consumes its source bytes; the limit pass, where + // the parser does not enforce the limits as it reads, and the two // parser/projection passes then consume normalized UTF-8 bytes. let xml = ""; let mut bytes = vec![0xff, 0xfe]; bytes.extend(xml.encode_utf16().flat_map(u16::to_le_bytes)); - let required = bytes.len() + 3 * xml.len(); for backend in XmlBackend::available() { + let passes = if backend.records_positions(xml) { 2 } else { 3 }; + let required = bytes.len() + passes * xml.len(); let mut policy = crate::policy::VerificationPolicy::default(); policy.resources.max_xml_parse_work_bytes = required - 1; assert!(matches!( @@ -4775,7 +4856,7 @@ mod tests { let settings = DocumentParseSettings::default(); let xml = r#""#; - let retained = build_semantic_cell(xml.to_owned(), settings, 1, 0) + let retained = build_semantic_cell(xml.to_owned(), settings, Some(1), 0) .expect("selected backend semantic projection must parse"); retained.with_dependent(|_, parsed| { let item = parsed @@ -5415,11 +5496,11 @@ mod tests { fn selected_backend_enforces_exact_depth_boundary() { let settings = DocumentParseSettings::new_with_depth(false, 128, 2, 4_096); let accepted = nested_document(2); - build_semantic_cell(accepted, settings, 0, 0).expect("exact depth must parse"); + build_semantic_cell(accepted, settings, Some(0), 0).expect("exact depth must parse"); let rejected = nested_document(3); assert!(matches!( - build_semantic_cell(rejected, settings, 0, 0), + build_semantic_cell(rejected, settings, Some(0), 0), Err(XmlDocumentError::DocumentTooDeep { maximum: 2, actual: 3, @@ -6049,7 +6130,11 @@ mod tests { .into_iter() .filter(|backend| backend.is_available()) { - let expected = xml.len() * 3; + // The limit pass, where the parser does not enforce the limits + // as it reads, and the parse and its projection each charge the + // source length. + let passes = if backend.records_positions(xml) { 2 } else { 3 }; + let expected = xml.len() * passes; let budget = XmlParseWorkBudget::with_limit(expected); parse_borrowed_with_settings_and_budget( xml, diff --git a/src/document/parser_limit_tests.rs b/src/document/parser_limit_tests.rs new file mode 100644 index 00000000..bd158b7b --- /dev/null +++ b/src/document/parser_limit_tests.rs @@ -0,0 +1,202 @@ +use super::*; + +/// Documents without a DOCTYPE that exercise every construct the limits count: +/// prolog and epilog markup, runs of text, references and CDATA, whitespace +/// inside and outside the document element, empty elements, and namespace +/// declarations that bind, rebind, undeclare and go out of scope. +const DOCUMENTS: &[&str] = &[ + "", + concat!( + "\n\n", + "a&bAd g\n", + " \n" + ), + concat!( + r#""#, + r#""# + ), + concat!( + "t", + "" + ), + "\n \n text < \n", + "t ", +]; + +fn settings(nodes: u32, depth: usize, bindings: usize) -> DocumentParseSettings { + DocumentParseSettings { + backend: XmlBackend::Xmloxide, + allow_dtd: false, + nodes_limit: nodes, + depth_limit: depth, + namespace_bindings_limit: bindings, + max_bytes: 1 << 20, + } +} + +/// The node, depth and namespace-binding limits of `settings`. +fn limits(settings: DocumentParseSettings) -> (u32, usize, usize) { + ( + settings.nodes_limit, + settings.depth_limit, + settings.namespace_bindings_limit, + ) +} + +/// The refusal the parser itself makes for `xml`, before any projection could +/// refuse it again. +fn parser_refusal(xml: &str, settings: DocumentParseSettings) -> Option { + let options = crate::parser_xmloxide::parser::ParseOptions::default(); + let (parsed, counter) = crate::parser_xmloxide::parser::parse_str_enforcing_limits( + xml, + &options, + parse_limits(settings), + ) + .expect("the counter sets up"); + let refusal = counter + .refusal() + .map(|refusal| format!("{:?}", limit_refusal(refusal))); + assert_eq!( + parsed.is_err(), + refusal.is_some(), + "{xml}: {:?}", + limits(settings) + ); + refusal +} + +/// Parse `xml` with the limits in the parser, returning the peak namespace +/// bindings or the refusal. +fn parsed(xml: &str, settings: DocumentParseSettings) -> Result { + assert!(settings.backend.records_positions(xml)); + XmlDocument::parse_with_settings(xml.to_owned(), settings) + .map(|document| document.with_view(|view| view.parsed.peak_namespace_bindings)) + .map_err(|error| format!("{error:?}")) +} + +#[test] +fn the_parser_refuses_each_limit_where_the_limit_pass_does() { + // The limit pass before the parser is gone where the parser enforces the + // limits as it reads. For every document and every value of each limit, + // from none past what the document needs, the parse must refuse with the + // error the limit pass reported, or accept with the same peak bindings. + let mut refusals = 0; + for xml in DOCUMENTS { + let cases = (0..48) + .map(|nodes| settings(nodes, 64, 64)) + .chain((0..8).map(|depth| settings(u32::MAX, depth, 64))) + .chain((0..6).map(|bindings| settings(u32::MAX, 64, bindings))); + for case in cases { + let expected = + preflight_document_limits(xml, case, None).map_err(|error| format!("{error:?}")); + let actual = parsed(xml, case); + assert_eq!(actual, expected, "{xml}: {:?}", limits(case)); + // The parser refuses before it allocates, not a later stage. + assert_eq!( + parser_refusal(xml, case), + actual.as_ref().err().cloned(), + "{xml}: {:?}", + limits(case) + ); + refusals += usize::from(actual.is_err()); + } + } + // Both outcomes must occur for the comparison to mean anything. + assert!(refusals > 50, "{refusals} refusals"); +} + +#[test] +fn an_element_past_two_limits_is_refused_for_the_first_the_pass_checked() { + // An element applies its declarations, then counts as a node, then as a + // level of nesting; the first limit it passes in that order is reported. + let xml = ""; + for (case, expected) in [ + ( + settings(2, 1, 0), + XmlDocumentError::Parse(ParseError::NamespaceBindingLimitReached { + maximum: 0, + actual: 1, + }), + ), + ( + settings(2, 1, 1), + XmlDocumentError::Parse(ParseError::NodesLimitReached), + ), + ( + settings(3, 1, 1), + XmlDocumentError::DocumentTooDeep { + maximum: 1, + actual: 2, + }, + ), + ] { + assert_eq!( + parsed(xml, case), + Err(format!("{expected:?}")), + "{:?}", + limits(case) + ); + } + // A policy depth equal to the parser's own nesting ceiling is still the + // policy refusal, not the ceiling's. + let ceiling = crate::hard_limits::XML_DOCUMENT_DEPTH_CEILING; + let deep = format!( + "{}{}", + "".repeat(ceiling + 1), + "".repeat(ceiling + 1) + ); + let expected = XmlDocumentError::DocumentTooDeep { + maximum: ceiling, + actual: ceiling + 1, + }; + assert_eq!( + parsed(&deep, settings(u32::MAX, ceiling, 64)), + Err(format!("{expected:?}")) + ); +} + +#[test] +fn policy_limits_refuse_at_one_over_through_the_public_entry() { + // Exact boundaries through the policy entry point: a document needing N + // of a resource is accepted at N and refused at N - 1. + let xml = "text"; + let mut policy = crate::policy::VerificationPolicy::default(); + let parse = |policy: &crate::policy::VerificationPolicy| { + XmlDocument::parse_with_policy_and_backend(xml.to_owned(), policy, XmlBackend::Xmloxide) + .map(drop) + .map_err(|error| format!("{error:?}")) + }; + let refused = |error: XmlDocumentError| Err(format!("{error:?}")); + // Root, r, c, d, text and comment. + policy.resources.max_xml_nodes = 6; + assert_eq!(parse(&policy), Ok(())); + policy.resources.max_xml_nodes = 5; + assert_eq!( + parse(&policy), + refused(XmlDocumentError::Parse(ParseError::NodesLimitReached)) + ); + policy = crate::policy::VerificationPolicy::default(); + policy.resources.max_xml_depth = 3; + assert_eq!(parse(&policy), Ok(())); + policy.resources.max_xml_depth = 2; + assert_eq!( + parse(&policy), + refused(XmlDocumentError::DocumentTooDeep { + maximum: 2, + actual: 3, + }) + ); + policy = crate::policy::VerificationPolicy::default(); + policy.resources.max_xml_namespace_bindings = 1; + assert_eq!(parse(&policy), Ok(())); + policy.resources.max_xml_namespace_bindings = 0; + assert_eq!( + parse(&policy), + refused(XmlDocumentError::Parse( + ParseError::NamespaceBindingLimitReached { + maximum: 0, + actual: 1, + } + )) + ); +} diff --git a/src/parser_xmloxide/parser/mod.rs b/src/parser_xmloxide/parser/mod.rs index cbc90f8e..9a11a302 100644 --- a/src/parser_xmloxide/parser/mod.rs +++ b/src/parser_xmloxide/parser/mod.rs @@ -250,3 +250,13 @@ pub(crate) fn parse_str_recording_positions( ) -> Result<(Document, alloc::vec::Vec), ParseError> { xml::XmlParser::new(input, options)?.parse_recording_positions() } + +/// Parses a document without a DOCTYPE as [`parse_str_recording_positions`] +/// does, enforcing `limits` as it reads. +pub(crate) fn parse_str_enforcing_limits( + input: &str, + options: &ParseOptions, + limits: crate::xml::dom::ParseLimits, +) -> Result, ParseError> { + xml::XmlParser::new(input, options)?.parse_enforcing_limits(limits) +} diff --git a/src/parser_xmloxide/parser/xml.rs b/src/parser_xmloxide/parser/xml.rs index a72d6ee2..2a27491e 100644 --- a/src/parser_xmloxide/parser/xml.rs +++ b/src/parser_xmloxide/parser/xml.rs @@ -5,7 +5,7 @@ use crate::xml::dom::storage::names::{NameStorage, Names}; use crate::xml::dom::storage::{Native, Storage, Values}; -use crate::xml::dom::{PositionRecorder, SourceKind, SourceNode}; +use crate::xml::dom::{LimitCounter, ParseLimits, PositionRecorder, SourceKind, SourceNode}; use crate::parser_xmloxide::error::{ErrorSeverity, ParseDiagnostic}; use crate::parser_xmloxide::tree::{Attribute, Document, NodeId, NodeKind}; @@ -72,6 +72,11 @@ const MAX_ENTITY_DEPTH: u32 = 32; /// A parsed document with the source ranges of its nodes. pub(crate) type Located = (Document, ::Values); type Finished = (Document, Option>); +/// A parse, with the limit counter that outlives a failed parse. +type Counted = (Result, ParseError>, Option>); +/// A parse that enforced its limits, with the counter that names a limit it +/// stopped at. +pub(crate) type Limited = (Result, ParseError>, LimitCounter); /// The core XML parser. pub(crate) struct XmlParser<'a, S: NameStorage = Native> { @@ -98,6 +103,8 @@ pub(crate) struct XmlParser<'a, S: NameStorage = Native> { entity_depth: u32, /// Source ranges of the nodes, when the caller has no lexical sidecar. positions: Option>, + /// The policy limits, when no pass before the parser enforced them. + limits: Option>, } impl<'a> XmlParser<'a> { @@ -136,24 +143,72 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { expansion_size: 0, entity_depth: 0, positions: None, + limits: None, }) } /// Main parse entry point. Parses the entire document. pub fn parse(self) -> Result, ParseError> { - self.parse_tree().map(|(document, _)| document) + self.parse_tree().0.map(|(document, _)| document) } /// Parses a document without a DOCTYPE and returns its nodes' source ranges, /// in the order the projector visits them. pub(crate) fn parse_recording_positions(mut self) -> Result, ParseError> { self.positions = Some(PositionRecorder::new(self.storage)?); - let (document, positions) = self.parse_tree()?; + let (document, positions) = self.parse_tree().0?; let positions = positions.expect("recording was enabled above"); Ok((document, positions.into_nodes())) } - fn parse_tree(mut self) -> Result, ParseError> { + /// Parses a document without a DOCTYPE as + /// [`parse_recording_positions`](Self::parse_recording_positions) does, + /// refusing it at the first of `limits` it passes before allocating the + /// node that passes it. + /// + /// # Errors + /// Returns a resource refusal when the counter cannot be set up; the + /// parse's own outcome is the first member of the result. + pub(crate) fn parse_enforcing_limits( + mut self, + limits: ParseLimits, + ) -> Result, ParseError> { + self.positions = Some(PositionRecorder::new(self.storage)?); + self.limits = Some(LimitCounter::new(limits, self.storage)?); + let (parsed, counter) = self.parse_tree(); + let located = parsed.map(|(document, positions)| { + let positions = positions.expect("recording was enabled above"); + (document, positions.into_nodes()) + }); + Ok((located, counter.expect("limits were enabled above"))) + } + + /// Report a construct to the limit counter, if the parse enforces limits. + fn limit( + &mut self, + report: impl FnOnce(&mut LimitCounter) -> Result<(), crate::xml::dom::ParseError>, + ) -> Result<(), ParseError> { + if let Some(limits) = &mut self.limits { + report(limits)?; + } + Ok(()) + } + + /// The parse, and the limit counter, which outlives a failed parse. + fn parse_tree(mut self) -> Counted { + let parsed = self.parse_document(); + let limits = self.limits.take(); + let finished = parsed.map(|()| { + // Sync diagnostics from input to document before returning. + self.doc.diagnostics = self.input.diagnostics; + // Parsing is complete and the parser is consumed. Moving the arena + // avoids Document::default allocating a replacement tree on return. + (self.doc, self.positions) + }); + (finished, limits) + } + + fn parse_document(&mut self) -> Result<(), ParseError> { // Parse optional XML declaration — must be at the very start of the // document with no leading whitespace (XML 1.0 §2.8). if self.input.looking_at(b" XmlParser<'a, S> { if !self.input.at_end() && !self.options.recover { return Err(self.input.fatal("content after document element")); } - - // Sync diagnostics from input to document before returning. - self.doc.diagnostics = self.input.diagnostics; - - // Parsing is complete and the parser is consumed. Moving the arena - // avoids Document::default allocating a replacement tree on return. - Ok((self.doc, self.positions)) + Ok(()) } // --- XML Declaration --- @@ -246,6 +295,9 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { // regardless of how many blank lines appear in the source. let ws = self.input.consume_whitespace()?; if !ws.is_empty() { + if let Some(limits) = &mut self.limits { + limits.markup(); + } let ws_node = self.doc.try_create_node(NodeKind::Text { content: self.storage.text("\n")?, })?; @@ -781,10 +833,7 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { &mut self, parent: NodeId, ) -> Result<(NodeId, Option), ParseError> { - self.input.increment_depth()?; - if let Some(positions) = &mut self.positions { - positions.start_element(self.input.pos())?; - } + let start = self.input.pos(); self.input.expect_byte(b'<')?; let name = self.input.parse_name()?; let mut attributes = self.storage.values(0)?; @@ -802,6 +851,29 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { attributes.push(attr)?; } + // The tag is read; it is refused for a limit before it is checked + // or allocated, as a pass before the parser refused it. + let empty = self.input.looking_at(b"/>"); + self.limit(|limits| { + limits.start_element( + attributes.iter().filter_map(|attribute| { + let prefix = match attribute.prefix.as_deref() { + Some("xmlns") => attribute.name.as_ref(), + None if attribute.name.as_ref() == "xmlns" => "", + _ => return None, + }; + Some((prefix, attribute.value.is_empty())) + }), + empty, + ) + })?; + // The absolute nesting ceilings follow the policy's own depth limit, + // which may equal them and is the refusal a document reports. + self.input.increment_depth()?; + if let Some(positions) = &mut self.positions { + positions.start_element(start)?; + } + self.check_attribute_spellings(&attributes)?; self.apply_attribute_defaults(name, &mut attributes)?; @@ -935,6 +1007,7 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { if let Some(positions) = &mut self.positions { positions.end_element(self.input.pos()); } + self.limit(LimitCounter::end_element)?; // Pop the namespace scope when leaving this element (only if we pushed). if has_ns_decls { @@ -973,6 +1046,7 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { // See XML 1.0 §2.4: [14] CharData fn parse_char_data(&mut self, parent: NodeId) -> Result<(), ParseError> { + self.limit(LimitCounter::character_data)?; let source_start = self.input.pos(); let mut text = self.storage.text("")?; @@ -1403,6 +1477,7 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { fn parse_comment(&mut self, parent: NodeId) -> Result<(), ParseError> { let start = self.input.pos(); let content = parse_comment_content(&mut self.input)?; + self.limit(LimitCounter::node)?; let comment_id = self.doc.try_create_node(NodeKind::Comment { content: self.storage.text(content)?, })?; @@ -1419,6 +1494,7 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { fn parse_cdata(&mut self, parent: NodeId) -> Result<(), ParseError> { let start = self.input.pos(); let content = parse_cdata_content(&mut self.input)?; + self.limit(LimitCounter::character_data)?; let cdata_id = self.doc.try_create_node(NodeKind::CData { content: self.storage.text(content)?, })?; @@ -1435,6 +1511,7 @@ impl<'a, S: NameStorage> XmlParser<'a, S> { fn parse_processing_instruction(&mut self, parent: NodeId) -> Result<(), ParseError> { let start = self.input.pos(); let (target, data) = parse_pi_content(&mut self.input)?; + self.limit(LimitCounter::node)?; let pi_id = self.doc.try_create_node(NodeKind::ProcessingInstruction { target: self.storage.text(target)?, data: self.storage.optional_text(data)?, diff --git a/src/xml/dom/limits.rs b/src/xml/dom/limits.rs new file mode 100644 index 00000000..e1dfb1ad --- /dev/null +++ b/src/xml/dom/limits.rs @@ -0,0 +1,196 @@ +//! Policy limits a parser enforces while it reads a document, in place of a +//! separate lexical pass over the source before it. +//! +//! The counts follow the semantic tree both backends build: the document root +//! is a node, a run of character data (text, references and CDATA sections +//! that no markup separates) is one node, and whitespace outside the document +//! element is none. An element applies its namespace declarations, then counts +//! as a node, then as a level of nesting, and is refused at the first of these +//! limits it passes, before the parser allocates it. + +use super::ParseError; +use super::storage::Values; +use super::storage::names::{NameStorage, Names}; + +/// The structural limits of one parse. +#[derive(Clone, Copy, Debug)] +pub(crate) struct ParseLimits { + /// Semantic nodes, the document root included. + pub(crate) nodes: u32, + /// Element nesting. + pub(crate) depth: usize, + /// Namespace bindings a document declares that are in scope at once. + pub(crate) namespace_bindings: usize, +} + +/// The limit a parse stopped at. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum LimitRefusal { + Nodes, + Depth { maximum: usize, actual: usize }, + NamespaceBindings { maximum: usize, actual: usize }, +} + +/// Counts a document against its [`ParseLimits`] as a parser reports what it +/// reads. +pub(crate) struct LimitCounter { + limits: ParseLimits, + nodes: u32, + depth: usize, + in_character_data: bool, + /// Prefixes bound in scope, the default namespace as the empty prefix. + active: S::Names, + peak_namespace_bindings: usize, + /// Each open element's binding changes, as a prefix and whether it was + /// bound before the element. + scopes: S::Values>, + storage: S, + refusal: Option, +} + +impl LimitCounter { + pub(crate) fn new(limits: ParseLimits, storage: S) -> Result { + Ok(Self { + limits, + // The document root. + nodes: 1, + depth: 0, + in_character_data: false, + active: storage.names(), + peak_namespace_bindings: 0, + scopes: storage.values(0)?, + storage, + refusal: None, + }) + } + + /// The limit the parse stopped at, if it stopped at one. + pub(crate) fn refusal(&self) -> Option { + self.refusal + } + + /// The most namespace bindings the document had in scope at once. + pub(crate) fn peak_namespace_bindings(&self) -> usize { + self.peak_namespace_bindings + } + + /// Record `refusal` and return the error that stops the parser. + fn refuse(&mut self, refusal: LimitRefusal) -> ParseError { + self.refusal = Some(refusal); + match refusal { + LimitRefusal::Nodes => ParseError::NodesLimitReached, + LimitRefusal::Depth { maximum, actual } => { + ParseError::DepthLimitReached { maximum, actual } + } + LimitRefusal::NamespaceBindings { maximum, actual } => { + ParseError::NamespaceBindingLimitReached { maximum, actual } + } + } + } + + fn count(&mut self) -> Result<(), ParseError> { + // A count past `u32::MAX` is past any limit. + match self.nodes.checked_add(1) { + Some(nodes) if nodes <= self.limits.nodes => { + self.nodes = nodes; + Ok(()) + } + _ => Err(self.refuse(LimitRefusal::Nodes)), + } + } + + /// Markup that is no node and ends a run of character data: an XML + /// declaration, or whitespace outside the document element. + pub(crate) fn markup(&mut self) { + self.in_character_data = false; + } + + /// A comment or processing instruction. + pub(crate) fn node(&mut self) -> Result<(), ParseError> { + self.in_character_data = false; + self.count() + } + + /// Character data, a node unless it continues a run. + pub(crate) fn character_data(&mut self) -> Result<(), ParseError> { + if self.in_character_data { + return Ok(()); + } + self.in_character_data = true; + self.count() + } + + /// An element's start tag with the namespace declarations it makes, each + /// a prefix (empty for the default namespace) and whether it undeclares + /// it; an empty-element tag closes at once. + pub(crate) fn start_element<'d>( + &mut self, + declarations: impl Iterator, + empty: bool, + ) -> Result<(), ParseError> { + self.in_character_data = false; + let storage = self.storage; + let mut changes = storage.values(0)?; + for (prefix, undeclares) in declarations { + storage.charge(1)?; + let was_active = if undeclares { + self.active.remove(prefix, storage)?.is_some() + } else if self.active.get(prefix, storage)?.is_some() { + true + } else { + self.active.insert(storage.text(prefix)?, ())?; + false + }; + changes.push((storage.text(prefix)?, was_active))?; + } + let actual = self.active.len(); + self.peak_namespace_bindings = self.peak_namespace_bindings.max(actual); + let maximum = self.limits.namespace_bindings; + if actual > maximum { + return Err(self.refuse(LimitRefusal::NamespaceBindings { maximum, actual })); + } + self.count()?; + // Nesting is bounded by the source length, far below `usize::MAX`. + let depth = self.depth + 1; + if depth > self.limits.depth { + let maximum = self.limits.depth; + return Err(self.refuse(LimitRefusal::Depth { + maximum, + actual: depth, + })); + } + if empty { + self.restore(changes) + } else { + self.depth = depth; + self.scopes.push(changes) + } + } + + /// The end tag of the innermost open element. + pub(crate) fn end_element(&mut self) -> Result<(), ParseError> { + self.in_character_data = false; + debug_assert!(self.depth > 0, "an end tag follows its start tag"); + // The parser reports an end tag only for an element it opened. + self.depth -= 1; + match self.scopes.pop() { + Some(changes) => self.restore(changes), + None => Ok(()), + } + } + + fn restore(&mut self, mut changes: S::Values<(S::Text, bool)>) -> Result<(), ParseError> { + let storage = self.storage; + while let Some((prefix, was_active)) = changes.pop() { + storage.charge(1)?; + if was_active { + if self.active.get(prefix.as_ref(), storage)?.is_none() { + self.active.insert(prefix, ())?; + } + } else { + self.active.remove(prefix.as_ref(), storage)?; + } + } + Ok(()) + } +} diff --git a/src/xml/dom/mod.rs b/src/xml/dom/mod.rs index ab42b796..c97c7bec 100644 --- a/src/xml/dom/mod.rs +++ b/src/xml/dom/mod.rs @@ -8,6 +8,8 @@ pub(crate) mod benchmark; #[cfg(all(feature = "xml-backend-roxmltree", feature = "xml-backend-xmloxide"))] mod differential; +#[cfg(feature = "xml-backend-xmloxide")] +mod limits; mod preflight; #[cfg(feature = "xml-backend-roxmltree")] mod roxmltree; @@ -20,6 +22,8 @@ mod xmloxide; use alloc::string::String; use core::fmt; +#[cfg(feature = "xml-backend-xmloxide")] +pub(crate) use self::limits::{LimitCounter, LimitRefusal, ParseLimits}; use self::preflight::LexicalPreflight; #[cfg(feature = "xml-backend-xmloxide")] pub(crate) use self::preflight::{PositionRecorder, SourceKind, SourceNode}; @@ -38,6 +42,15 @@ pub struct ParsingOptions { pub nodes_limit: u32, } +/// Why a parse that enforced its policy limits produced no document. +#[cfg(feature = "xml-backend-xmloxide")] +pub(crate) enum Refused { + /// The document passes one of its limits. + Limit(LimitRefusal), + /// The parse failed otherwise. + Parse(E), +} + impl Default for ParsingOptions { fn default() -> Self { Self { @@ -94,9 +107,10 @@ impl XmlBackend { } /// Whether this parse needs no lexical scan: xmloxide knows every source - /// range, and only a DOCTYPE, whose keyword is case-sensitive (XML 1.0 - /// production 28), brings declarations and entity expansion. - fn records_positions(self, input: &str) -> bool { + /// range and enforces the policy limits as it reads, and only a DOCTYPE, + /// whose keyword is case-sensitive (XML 1.0 production 28), brings + /// declarations and entity expansion, which a scan must bound first. + pub(crate) fn records_positions(self, input: &str) -> bool { cfg!(feature = "xml-backend-xmloxide") && self == Self::Xmloxide && !input.contains(" Document<'input> { backend.parse(input, options, &preflight) } + /// Parses an input whose policy limits the backend enforces as it reads + /// (see [`XmlBackend::records_positions`]), returning the document with + /// the most namespace bindings it had in scope at once. + #[cfg(feature = "xml-backend-xmloxide")] + pub(crate) fn parse_enforcing_limits( + input: &'input str, + options: ParsingOptions, + limits: super::ParseLimits, + ) -> Result<(Self, usize), super::Refused> { + super::xmloxide::XmloxideBackend::parse_enforcing_limits(input, options, limits) + } + + /// [`parse_enforcing_limits`](Self::parse_enforcing_limits) in the + /// current query's storage. + #[cfg(all(feature = "xmldsig", feature = "xml-backend-xmloxide"))] + pub(crate) fn parse_enforcing_limits_in<'w, W: structured_asn1::memory::Work>( + input: &'input str, + options: ParsingOptions, + limits: super::ParseLimits, + storage: super::storage::Accounted<'w, W>, + ) -> Result< + (Document<'input, super::storage::Accounted<'w, W>>, usize), + super::Refused, + > { + super::xmloxide::XmloxideBackend::parse_enforcing_limits_in(input, options, limits, storage) + } + #[cfg(feature = "xmldsig")] pub(crate) fn parse_after_limit_preflight_in<'w, W: structured_asn1::memory::Work>( input: &'input str, diff --git a/src/xml/dom/xmloxide.rs b/src/xml/dom/xmloxide.rs index 362e5396..fa62f93b 100644 --- a/src/xml/dom/xmloxide.rs +++ b/src/xml/dom/xmloxide.rs @@ -9,13 +9,20 @@ use crate::parser_xmloxide as xmloxide; use xmloxide::tree::{NodeId as XmloxNodeId, NodeKind as XmloxNodeKind}; use super::{ - Document, LexicalPreflight, ParseError, ParsingOptions, XmlBackendImplementation, + Document, LexicalPreflight, ParseError, ParseLimits, ParsingOptions, Refused, + XmlBackendImplementation, preflight::{PositionCursor, SourceKind, SourceNode}, tree::{AttributeData, NamespaceData, NamespaceSet, NodeId, NodeKind, TreeBuilder}, }; pub(super) struct XmloxideBackend; +/// The parser's failure in storage `S`. +type XmloxideError = xmloxide::error::ParseError< + ::FrozenText, + ::Values::FrozenText>>, +>; + #[cfg(feature = "xmldsig")] impl XmloxideBackend { pub(super) fn parse_in<'input, 'w, W: structured_asn1::memory::Work>( @@ -56,6 +63,80 @@ impl XmloxideBackend { input, parsed, options, preflight, positions, storage, )?) } + + /// [`parse_enforcing_limits`](Self::parse_enforcing_limits) in the + /// current query's storage. + pub(super) fn parse_enforcing_limits_in<'input, 'w, W: structured_asn1::memory::Work>( + input: &'input str, + options: ParsingOptions, + limits: ParseLimits, + storage: super::storage::Accounted<'w, W>, + ) -> Result< + Limited<'input, super::storage::Accounted<'w, W>>, + Refused, + > { + let failure = |error: XmloxideError>| match error { + xmloxide::error::ParseError::Resource(error) => { + super::AccountedParseError::Semantic(error) + } + xmloxide::error::ParseError::Syntax { + message, + location, + diagnostics, + } => super::AccountedParseError::Xmloxide(xmloxide::error::ParseError::Syntax { + message, + location, + diagnostics: diagnostics.into_owned(), + }), + }; + let (parsed, counter) = + xmloxide::parser::XmlParser::with_storage(input, &backend_options(), storage) + .and_then(|parser| parser.parse_enforcing_limits(limits)) + .map_err(|error| Refused::Parse(failure(error)))?; + let (parsed, positions) = parsed.map_err(|error| match counter.refusal() { + Some(refusal) => Refused::Limit(refusal), + None => Refused::Parse(failure(error)), + })?; + let preflight = + LexicalPreflight::unscanned(storage).map_err(|error| Refused::Parse(error.into()))?; + let document = project_in(input, parsed, options, &preflight, &positions, storage) + .map_err(|error| Refused::Parse(error.into()))?; + Ok((document, counter.peak_namespace_bindings())) + } +} + +/// A document parsed under its limits, with the most namespace bindings it had +/// in scope at once. +pub(super) type Limited<'input, S> = (Document<'input, S>, usize); + +impl XmloxideBackend { + /// Parses a document without a DOCTYPE, refusing it at the first of + /// `limits` it passes as the parser reads it, so no pass over the source + /// precedes the parse. + pub(super) fn parse_enforcing_limits( + input: &str, + options: ParsingOptions, + limits: ParseLimits, + ) -> Result, Refused> { + let failure = |error| match error { + xmloxide::error::ParseError::Resource(error) => error, + error @ xmloxide::error::ParseError::Syntax { .. } => ParseError::Backend { + backend: "xmloxide", + message: error.to_string(), + }, + }; + let (parsed, counter) = + xmloxide::parser::parse_str_enforcing_limits(input, &backend_options(), limits) + .map_err(|error| Refused::Parse(failure(error)))?; + let (parsed, positions) = parsed.map_err(|error| match counter.refusal() { + Some(refusal) => Refused::Limit(refusal), + None => Refused::Parse(failure(error)), + })?; + let preflight = LexicalPreflight::unscanned(Native).map_err(Refused::Parse)?; + let document = project_in(input, parsed, options, &preflight, &positions, Native) + .map_err(Refused::Parse)?; + Ok((document, counter.peak_namespace_bindings())) + } } /// Takes a backend text into the semantic tree. The two trees share one storage From 4e0f35672f22c324a37724eda5bced89e060d86a Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 3 Nov 2026 17:02:49 +0200 Subject: [PATCH 2/5] perf(xml): check the limits in the pass that records positions 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 --- src/document.rs | 1170 ++++++++++++++++-------------- src/document/scan_limit_tests.rs | 134 ++++ src/xml/dom/limits.rs | 16 +- src/xml/dom/mod.rs | 8 +- src/xml/dom/preflight.rs | 304 +++++--- src/xml/dom/tree.rs | 48 +- src/xml/dom/xmloxide.rs | 1 + src/xmlenc/parse.rs | 2 +- tests/owned_document_pipeline.rs | 15 +- 9 files changed, 1020 insertions(+), 678 deletions(-) create mode 100644 src/document/scan_limit_tests.rs diff --git a/src/document.rs b/src/document.rs index 25f3d502..0c7b9252 100644 --- a/src/document.rs +++ b/src/document.rs @@ -28,7 +28,9 @@ use crate::xml::dom::storage::names::{NameStorage, Names}; use crate::xml::dom::storage::{ Native as NativeXmlStorage, Storage as XmlStorage, Values as XmlValues, }; -use crate::xml::dom::{Document, Node, NodeId, ParseError, ParsingOptions, XmlBackend}; +use crate::xml::dom::{ + Document, LimitCounter, Node, NodeId, ParseError, ParsingOptions, ScanObserver, XmlBackend, +}; use crate::xml_input as xml_sec_xml_input; use self_cell::self_cell; @@ -51,7 +53,7 @@ const VALIDATION_WRAPPER_NODE_OVERHEAD: u32 = 3; #[cfg(all(test, any(feature = "xmldsig", feature = "xmlenc")))] pub(crate) fn selected_parser_passes() -> usize { - 3 + 2 } /// Process-local provenance of one owned XML document. @@ -942,32 +944,13 @@ impl XmlDocument { budget: Option<&XmlParseWorkBudget>, prior_work: usize, ) -> Result { - if xml.len() > settings.max_bytes { - return Err(XmlDocumentError::DocumentTooLarge { - maximum: settings.max_bytes, - actual: xml.len(), - }); - } - let ((peak_namespace_bindings, requires_internal_dtd), preflight_work) = - metered(budget, |budget| { - let peak = - limit_pass_unless_parser_enforces(&xml, settings, budget, NativeXmlStorage)?; - #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] - let requires_internal_dtd = document_requires_internal_dtd(&xml, settings, budget)?; - #[cfg(not(any(feature = "xmldsig", feature = "xmlenc")))] - let requires_internal_dtd = (); - Ok((peak, requires_internal_dtd)) - })?; + // Parse provenance records what the source actually requires, not merely + // whether its creator used a permissive policy: a later strict operation + // accepts ordinary XML and rejects a document that declares a DTD. + let (cell, requires_internal_dtd) = + build_cell_recording(xml, settings, budget, prior_work)?; #[cfg(not(any(feature = "xmldsig", feature = "xmlenc")))] - let () = requires_internal_dtd; - let cell = build_cell_after_preflight( - xml, - settings, - budget, - peak_namespace_bindings, - // Both terms are bounded by the input and expansion ceilings. - prior_work + preflight_work, - )?; + let _ = requires_internal_dtd; let identity = allocate_document_identity(&NEXT_DOCUMENT_ID)?; Ok(Self { identity, @@ -2451,51 +2434,39 @@ fn build_cell( settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, ) -> Result { - let (peak_namespace_bindings, preflight_work) = metered(budget, |budget| { - limit_pass_unless_parser_enforces(&xml, settings, budget, NativeXmlStorage) - })?; - build_cell_after_preflight( - xml, - settings, - budget, - peak_namespace_bindings, - preflight_work, - ) + build_cell_recording(xml, settings, budget, 0).map(|(cell, _)| cell) } -/// Builds a generation whose limit pass, if one ran, already charged -/// `preflight_work` and found `peak_namespace_bindings`. -fn build_cell_after_preflight( +/// Builds a generation that records `prior_work` beside the work of its own +/// parse, and returns whether its source declares a document type. +fn build_cell_recording( xml: String, settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, - peak_namespace_bindings: Option, - preflight_work: usize, -) -> Result { - let ((), semantic_work) = metered(budget, |budget| { - charge_semantic_parser_work(budget, xml.len()) + prior_work: usize, +) -> Result<(DocumentCell, bool), XmlDocumentError> { + check_document_size(&xml, settings)?; + let ((mut cell, doctype), work) = metered(budget, |budget| { + charge_semantic_parser_work(budget, xml.len())?; + build_semantic_cell(xml, settings, budget) })?; // Both terms are charges already admitted by one budget below usize::MAX. - build_semantic_cell( - xml, - settings, - peak_namespace_bindings, - preflight_work + semantic_work, - ) + record_work(&mut cell, prior_work + work); + Ok((cell, doctype)) } +/// Builds a generation, with no parse work recorded yet, and returns whether +/// its source declares a document type. fn build_semantic_cell( xml: String, settings: DocumentParseSettings, - preflight_peak: Option, - parse_work: usize, -) -> Result { - // Only XMLDSig entry points take over a caller's parse and charge its work. - #[cfg(not(feature = "xmldsig"))] - let _ = parse_work; - DocumentCell::try_new(xml, |source| { - let (document, metrics, peak_namespace_bindings) = - parse_semantic_document(source, settings, preflight_peak)?; + budget: Option<&XmlParseWorkBudget>, +) -> Result<(DocumentCell, bool), XmlDocumentError> { + let mut declares_doctype = false; + let cell = DocumentCell::try_new(xml, |source| { + let (document, metrics, peak_namespace_bindings, doctype) = + parse_semantic_document(source, settings, budget)?; + declares_doctype = doctype; #[cfg(not(any(feature = "xmldsig", feature = "xmlenc")))] let _ = peak_namespace_bindings; debug_assert!( @@ -2513,9 +2484,10 @@ fn build_semantic_cell( #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] peak_namespace_bindings, #[cfg(feature = "xmldsig")] - parse_work, + parse_work: 0, }) - }) + })?; + Ok((cell, declares_doctype)) } pub(crate) fn parse_borrowed_with_settings_and_budget<'a>( @@ -2523,10 +2495,9 @@ pub(crate) fn parse_borrowed_with_settings_and_budget<'a>( settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, ) -> Result, XmlDocumentError> { - let preflight_peak = - limit_pass_unless_parser_enforces(xml, settings, budget, NativeXmlStorage)?; + check_document_size(xml, settings)?; charge_semantic_parser_work(budget, xml.len())?; - let (document, _, _) = parse_semantic_document(xml, settings, preflight_peak)?; + let (document, ..) = parse_semantic_document(xml, settings, budget)?; Ok(document) } @@ -2537,54 +2508,70 @@ pub(crate) fn parse_borrowed_in<'input, 'w, W: structured_asn1::memory::Work>( budget: Option<&XmlParseWorkBudget>, storage: crate::xml::dom::storage::Accounted<'w, W>, ) -> Result>, XmlDocumentError> { - let preflight_peak = limit_pass_unless_parser_enforces(xml, settings, budget, storage)?; + check_document_size(xml, settings)?; charge_semantic_parser_work(budget, xml.len())?; - let options = ParsingOptions { - allow_dtd: settings.allow_dtd, - nodes_limit: settings.nodes_limit, - }; - let document = match preflight_peak { - Some(_) => { - Document::parse_after_limit_preflight_in(xml, options, settings.backend, storage)? - } - #[cfg(feature = "xml-backend-xmloxide")] - None => { + let options = parsing_options(settings); + #[cfg(feature = "xml-backend-xmloxide")] + if settings.backend.records_positions(xml) { + let (document, _) = Document::parse_enforcing_limits_in(xml, options, parse_limits(settings), storage) .map_err(|refused| match refused { crate::xml::dom::Refused::Limit(refusal) => limit_refusal(refusal), crate::xml::dom::Refused::Parse(error) => error.into(), - })? - .0 - } - #[cfg(not(feature = "xml-backend-xmloxide"))] - None => unreachable!("only xmloxide enforces limits while it parses"), - }; + })?; + validate_document_metrics_in(&document, settings.depth_limit, storage)?; + return Ok(document); + } + let mut pass = LimitPass::new(settings, budget, storage)?; + let document = Document::parse_observed_in(xml, options, settings.backend, storage, &mut pass) + .map_err(|error| pass.refusal_or(error.into()))?; validate_document_metrics_in(&document, settings.depth_limit, storage)?; Ok(document) } -fn preflight_document_limits( - xml: &str, +/// Parses `xml` under `settings`, its limits checked by the parser as it +/// reads or by the lexical pass that records the source positions, and +/// returns the document's peak namespace bindings and whether it declares a +/// document type. +fn parse_checked<'a>( + xml: &'a str, settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, -) -> Result { - preflight_document_limits_in(xml, settings, budget, NativeXmlStorage) +) -> Result<(Document<'a>, usize, bool), XmlDocumentError> { + let options = parsing_options(settings); + #[cfg(feature = "xml-backend-xmloxide")] + if settings.backend.records_positions(xml) { + let (document, peak) = + Document::parse_enforcing_limits(xml, options, parse_limits(settings)).map_err( + |refused| match refused { + crate::xml::dom::Refused::Limit(refusal) => limit_refusal(refusal), + crate::xml::dom::Refused::Parse(error) => XmlDocumentError::Parse(error), + }, + )?; + return Ok((document, peak, false)); + } + let mut pass = LimitPass::new(settings, budget, NativeXmlStorage)?; + let document = Document::parse_observed_with_backend(xml, options, settings.backend, &mut pass) + .map_err(|error| pass.refusal_or(XmlDocumentError::Parse(error)))?; + Ok((document, pass.peak_namespace_bindings(), pass.doctype)) +} + +fn parsing_options(settings: DocumentParseSettings) -> ParsingOptions { + ParsingOptions { + allow_dtd: settings.allow_dtd, + nodes_limit: settings.nodes_limit, + } } -/// The limit pass before the parser, returning the peak namespace bindings, -/// or `None` for a document whose parser enforces the limits as it reads it, -/// which leaves only the size check before the parse. -fn limit_pass_unless_parser_enforces( +/// The limit pass by itself, the reference the parses that check the limits +/// as they read are tested against. +#[cfg(test)] +fn preflight_document_limits( xml: &str, settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, - storage: S, -) -> Result, XmlDocumentError> { - if settings.backend.records_positions(xml) { - check_document_size(xml, settings)?; - return Ok(None); - } - preflight_document_limits_in(xml, settings, budget, storage).map(Some) +) -> Result { + preflight_document_limits_in(xml, settings, budget, NativeXmlStorage) } fn check_document_size(xml: &str, settings: DocumentParseSettings) -> Result<(), XmlDocumentError> { @@ -2598,7 +2585,6 @@ fn check_document_size(xml: &str, settings: DocumentParseSettings) -> Result<(), } /// The limits a parser enforces as it reads a document under `settings`. -#[cfg(feature = "xml-backend-xmloxide")] fn parse_limits(settings: DocumentParseSettings) -> crate::xml::dom::ParseLimits { crate::xml::dom::ParseLimits { nodes: settings.nodes_limit, @@ -2608,7 +2594,6 @@ fn parse_limits(settings: DocumentParseSettings) -> crate::xml::dom::ParseLimits } /// The refusal for the limit a parse stopped at, as the limit pass reports it. -#[cfg(feature = "xml-backend-xmloxide")] fn limit_refusal(refusal: crate::xml::dom::LimitRefusal) -> XmlDocumentError { use crate::xml::dom::LimitRefusal; match refusal { @@ -2634,63 +2619,418 @@ pub(crate) fn preflight_xml_limits( preflight_document_limits_in(xml, settings, Some(budget), storage).map(drop) } +#[cfg(any(feature = "xslt", test))] fn preflight_document_limits_in( xml: &str, settings: DocumentParseSettings, budget: Option<&XmlParseWorkBudget>, storage: S, ) -> Result { - // This bounded lexical pass precedes any DOM construction wherever the - // parser does not enforce the limits itself. + // This bounded lexical pass precedes a parser that neither enforces the + // limits nor reads the source positions the strict pass records. check_document_size(xml, settings)?; charge_parse_work(budget, xml.len())?; - let mut dtd = InternalDtd::new(storage); - let mut expansion_stack = storage.names(); - let mut state = DocumentPreflightState::new(storage)?; - preflight_xml_fragment( - xml, - settings, - &mut dtd, - &mut expansion_stack, - &mut state, - budget, - true, - )?; - Ok(state.peak_namespace_bindings) + let mut pass = LimitPass::new(settings, budget, storage)?; + pass.document(xml)?; + Ok(pass.peak_namespace_bindings()) } +/// The node, nesting and namespace-binding limits checked in a lexical pass +/// before the parser, through the internal entities the document expands. +/// +/// It runs over the whole document by itself ([`Self::document`]), or as the +/// [`ScanObserver`] of the strict scan that records the source positions, so +/// that one pass serves both. +struct LimitPass<'b, S: NameStorage = NativeXmlStorage> { + dtd: InternalDtd, + run: LimitRun<'b, S>, + /// The refusal behind the stop signal a strict scan returned. + refusal: Option, + /// The scan read a document type declaration. + doctype: bool, +} + +/// The counts of a [`LimitPass`], apart from the declarations they read. +struct LimitRun<'b, S: NameStorage> { + settings: DocumentParseSettings, + budget: Option<&'b XmlParseWorkBudget>, + counter: LimitCounter, + expansion_stack: S::Names, + state: DocumentPreflightState, +} + +/// Entity expansion counts, which the absolute ceilings bound. struct DocumentPreflightState { - depth: usize, - nodes: u32, - in_character_data: bool, entity_expansions: u32, entity_expansion_work: usize, - active_namespace_bindings: S::Names, - peak_namespace_bindings: usize, - namespace_scopes: S::Values>, storage: S, } impl DocumentPreflightState { - fn new(storage: S) -> Result { - Ok(Self { - depth: 0, - nodes: 0, - in_character_data: false, + fn new(storage: S) -> Self { + Self { entity_expansions: 0, entity_expansion_work: 0, - active_namespace_bindings: storage.names(), - peak_namespace_bindings: 0, - namespace_scopes: storage.values(0)?, storage, + } + } +} + +impl<'b, S: NameStorage> LimitPass<'b, S> { + fn new( + settings: DocumentParseSettings, + budget: Option<&'b XmlParseWorkBudget>, + storage: S, + ) -> Result { + Ok(Self { + dtd: InternalDtd::new(storage), + run: LimitRun { + settings, + budget, + counter: LimitCounter::new(parse_limits(settings), storage)?, + expansion_stack: storage.names(), + state: DocumentPreflightState::new(storage), + }, + refusal: None, + doctype: false, }) } + + fn peak_namespace_bindings(&self) -> usize { + self.run.counter.peak_namespace_bindings() + } + + /// The refusal a strict scan stopped at, in place of `signal`, the error + /// the scan returned. + fn refusal_or(&mut self, signal: XmlDocumentError) -> XmlDocumentError { + self.refusal.take().unwrap_or(signal) + } + + /// The pass over a whole document, by itself. + #[cfg(any(feature = "xslt", test))] + fn document(&mut self, xml: &str) -> Result<(), XmlDocumentError> { + let storage = self.run.state.storage; + // Disjoint lexical event windows make these source-sized passes linear; + // dictionary probes, expansion revisits and copies are admitted separately. + storage.charge(xml.len())?; + storage.charge(xml.len())?; + storage.charge(xml.len())?; + let mut fragments: S::Values> = storage.values(1)?; + fragments.push(FragmentFrame::new( + FragmentSource::Document(xml), + FragmentContext::Content, + true, + storage, + )?)?; + while let Some(doctype) = self.run.drain(&self.dtd, &mut fragments)? { + storage.charge(doctype.len())?; + storage.charge(doctype.len())?; + storage.charge(doctype.len())?; + collect_internal_dtd(&doctype, &mut self.dtd, |_, _, _| Ok(()))?; + } + Ok(()) + } + + /// Stop a strict scan at `result`'s refusal, which the caller takes back + /// with [`Self::refusal_or`]. + fn stop(&mut self, result: Result<(), XmlDocumentError>) -> Result<(), ParseError> { + match result { + Ok(()) => Ok(()), + Err(XmlDocumentError::Parse(error)) => Err(error), + Err(error) => { + let signal = dom_parse_error(&error); + self.refusal = Some(error); + Err(signal) + } + } + } } -impl Default for DocumentPreflightState { - fn default() -> Self { - Self::new(NativeXmlStorage).expect("empty XML preflight does not allocate") +impl<'input, S: NameStorage> ScanObserver<'input> for LimitPass<'_, S> { + fn doctype(&mut self, doctype: &'input str, attribute: F) -> Result<(), ParseError> + where + F: FnMut(&'input str, &'input str, DtdAttributeType) -> Result<(), ParseError>, + { + self.doctype = true; + let result = collect_internal_dtd(doctype, &mut self.dtd, attribute); + self.stop(result) + } + + fn start_element( + &mut self, + tag: &'input str, + tag_start: usize, + attributes: &[xml_sec_xml_input::lexical::Attribute<'input>], + empty: bool, + ) -> Result<(), ParseError> { + let result = self + .run + .start_tag(&self.dtd, tag, tag_start, attributes, empty); + self.stop(result) + } + + fn end_element(&mut self) -> Result<(), ParseError> { + let result = self.run.counter.end_element(); + let result = limit_checked(&self.run.counter, result); + self.stop(result) + } + + fn character_data(&mut self) -> Result<(), ParseError> { + let result = self.run.counter.character_data(); + let result = limit_checked(&self.run.counter, result); + self.stop(result) + } + + fn entity_reference(&mut self, name: &'input str) -> Result<(), ParseError> { + let result = self.run.expand(&self.dtd, name, FragmentContext::Content); + self.stop(result) + } + + fn node(&mut self) -> Result<(), ParseError> { + let result = self.run.counter.node(); + let result = limit_checked(&self.run.counter, result); + self.stop(result) } + + fn markup(&mut self) { + self.run.counter.markup(); + } +} + +impl LimitRun<'_, S> { + /// Count an element with the namespace declarations it makes. + fn open( + &mut self, + declarations: &[NamespaceDeclaration], + empty: bool, + ) -> Result<(), XmlDocumentError> { + let result = self.counter.start_element( + declarations + .iter() + .map(|declaration| (declaration.prefix.as_ref(), declaration.is_undeclaration)), + empty, + ); + limit_checked(&self.counter, result) + } + + /// A start tag the strict scan read: `tag` is its source, which starts at + /// `tag_start` in the document, and `attributes` its attributes. + fn start_tag( + &mut self, + dtd: &InternalDtd, + tag: &str, + tag_start: usize, + attributes: &[xml_sec_xml_input::lexical::Attribute<'_>], + empty: bool, + ) -> Result<(), XmlDocumentError> { + let storage = self.state.storage; + storage.charge(tag.len())?; + let pairs = attributes.iter().map(|attribute| { + // The scanner's ranges lie inside the tag it read. + let start = attribute.range.start - tag_start; + ( + &tag[start..start + qualified_len(attribute.name)], + attribute.value, + ) + }); + let has_defaults = dtd.attribute_defaults.len() != 0; + let element = if has_defaults { element_name(tag) } else { "" }; + let declarations = if has_defaults + || attributes + .iter() + .any(|attribute| is_namespace_declaration(attribute.name)) + { + namespace_declarations(element, pairs.clone(), dtd, &mut self.state, self.budget)? + } else { + storage.values(0)? + }; + self.open(&declarations, empty)?; + // Attribute values expand their references once the element counts. + for (_, value) in pairs.clone() { + for name in general_references(value.as_bytes()) { + self.expand(dtd, name, FragmentContext::Attribute)?; + } + } + if has_defaults && let Some(defaults) = dtd.attribute_defaults.get(element, storage)? { + for (index, default) in defaults.iter().enumerate() { + storage.charge(default.value.len())?; + // An attribute the tag gives, or an earlier default supplies, + // takes no value from this default. + let name = default.attribute_name.as_ref(); + if pairs.clone().any(|(given, _)| given == name) + || defaults[..index] + .iter() + .any(|earlier| earlier.attribute_name.as_ref() == name) + { + continue; + } + for name in general_references(default.value.as_bytes()) { + self.expand(dtd, name, FragmentContext::Attribute)?; + } + } + } + Ok(()) + } + + /// Expand the general entity `name` referenced in `context`. + fn expand( + &mut self, + dtd: &InternalDtd, + name: &str, + context: FragmentContext, + ) -> Result<(), XmlDocumentError> { + let storage = self.state.storage; + let mut fragments: S::Values> = storage.values(1)?; + enter_preflight_entity( + storage.text(name)?, + context, + dtd, + &mut self.expansion_stack, + &mut fragments, + &mut self.state, + self.budget, + )?; + let doctype = self.drain(dtd, &mut fragments)?; + debug_assert!(doctype.is_none(), "entity frames collect no DOCTYPE"); + Ok(()) + } + + /// Read `fragments` to their end, or to a document type declaration to + /// collect, which is returned with the frames left to read after it. + fn drain( + &mut self, + dtd: &InternalDtd, + fragments: &mut S::Values>, + ) -> Result, XmlDocumentError> { + let storage = self.state.storage; + // Syntax diagnostics and exact parser precedence belong to the selected + // DOM pipeline; this pass stops at malformed syntax after bounding its prefix. + while !fragments.is_empty() { + storage.charge(1)?; + let (event, collect_doctype, context) = { + let frame = fragments.last_mut().expect("fragment stack is not empty"); + frame.next_event(dtd, &mut self.state, self.budget, self.counter.depth())? + }; + + match event { + PreflightEvent::DocType(doctype) => { + self.counter.markup(); + if collect_doctype + && self.settings.allow_dtd + && let Some(doctype) = doctype + { + return Ok(Some(doctype)); + } + } + PreflightEvent::GeneralRef { + name, + is_character_reference, + } => { + let Some(name) = name else { + continue; + }; + if is_character_reference { + if context == FragmentContext::Content { + let result = self.counter.character_data(); + limit_checked(&self.counter, result)?; + } + continue; + } + enter_preflight_entity( + name, + context, + dtd, + &mut self.expansion_stack, + fragments, + &mut self.state, + self.budget, + )?; + } + PreflightEvent::Done => { + let frame = fragments.pop().expect("fragment stack is not empty"); + match frame.source { + FragmentSource::Entity(name, _) => { + self.expansion_stack.remove(&name, storage)?; + } + #[cfg(any(feature = "xslt", test))] + FragmentSource::Document(_) => {} + } + } + PreflightEvent::CharacterData { xml_whitespace } => { + if context == FragmentContext::Attribute { + continue; + } + let result = if self.counter.depth() == 0 && xml_whitespace { + self.counter.markup(); + Ok(()) + } else { + self.counter.character_data() + }; + limit_checked(&self.counter, result)?; + } + PreflightEvent::Node => { + let result = self.counter.node(); + limit_checked(&self.counter, result)?; + } + PreflightEvent::Markup => self.counter.markup(), + PreflightEvent::Start { + attribute_source, + namespace_declarations, + empty, + } => { + if context == FragmentContext::Attribute { + continue; + } + let frame = fragments + .last_mut() + .expect("active element frame remains registered"); + frame.pending_attribute_source = attribute_source; + frame.pending_attribute_offset = 0; + self.open(&namespace_declarations, empty)?; + } + PreflightEvent::End => { + if context == FragmentContext::Attribute { + continue; + } + let result = self.counter.end_element(); + limit_checked(&self.counter, result)?; + } + } + } + Ok(None) + } +} + +/// `result` of a limit count, with the refusal `counter` stopped at, if any, +/// in place of its stop signal. +fn limit_checked( + counter: &LimitCounter, + result: Result<(), ParseError>, +) -> Result<(), XmlDocumentError> { + result.map_err(|error| counter.refusal().map_or(error.into(), limit_refusal)) +} + +/// The length of `name` as written, prefix and colon included. +fn qualified_len(name: xml_sec_xml_input::lexical::Name<'_>) -> usize { + name.prefix().map_or(0, |prefix| prefix.len() + 1) + name.local().len() +} + +/// Whether the attribute `name` declares a namespace. +fn is_namespace_declaration(name: xml_sec_xml_input::lexical::Name<'_>) -> bool { + match name.prefix() { + Some(prefix) => prefix == "xmlns", + None => name.local() == "xmlns", + } +} + +/// The element name of the start tag `tag`. +fn element_name(tag: &str) -> &str { + let name = &tag[1..]; + let end = name + .bytes() + .position(|byte| matches!(byte, b' ' | b'\t' | b'\r' | b'\n' | b'/' | b'>')) + .unwrap_or(name.len()); + &name[..end] } struct InternalDtd { @@ -2735,13 +3075,11 @@ enum PreflightEvent { CharacterData { xml_whitespace: bool, }, + /// A start tag, or with `empty` an empty-element tag. Start { attribute_source: S::Values, namespace_declarations: S::Values>, - }, - Empty { - attribute_source: S::Values, - namespace_declarations: S::Values>, + empty: bool, }, End, Node, @@ -2749,13 +3087,17 @@ enum PreflightEvent { Done, } -enum FragmentSource { - Document, - Entity(S::Text), +enum FragmentSource<'a, S: NameStorage> { + /// The document itself, read only by the pass that runs by itself. + #[cfg(any(feature = "xslt", test))] + Document(&'a str), + /// The replacement text of the named internal entity; the marker keeps + /// the document's lifetime where no document frame is compiled. + Entity(S::Text, core::marker::PhantomData<&'a str>), } -struct FragmentFrame { - source: FragmentSource, +struct FragmentFrame<'a, S: NameStorage> { + source: FragmentSource<'a, S>, offset: usize, collect_doctype: bool, context: FragmentContext, @@ -2763,13 +3105,30 @@ struct FragmentFrame { pending_attribute_offset: usize, } -impl FragmentFrame { +impl<'a, S: NameStorage> FragmentFrame<'a, S> { + fn new( + source: FragmentSource<'a, S>, + context: FragmentContext, + collect_doctype: bool, + storage: S, + ) -> Result { + Ok(Self { + source, + offset: 0, + collect_doctype, + context, + pending_attribute_source: storage.values(0)?, + pending_attribute_offset: 0, + }) + } + + /// The next event of this fragment, read with `depth` elements open. fn next_event( &mut self, - xml: &str, dtd: &InternalDtd, state: &mut DocumentPreflightState, budget: Option<&XmlParseWorkBudget>, + depth: usize, ) -> Result<(PreflightEvent, bool, FragmentContext), XmlDocumentError> { let storage = state.storage; let mut pending = @@ -2793,15 +3152,22 @@ impl FragmentFrame { self.pending_attribute_source.clear(); self.pending_attribute_offset = 0; let source = match &self.source { - FragmentSource::Document => xml, - FragmentSource::Entity(name) => dtd + #[cfg(any(feature = "xslt", test))] + FragmentSource::Document(xml) => xml, + FragmentSource::Entity(name, _) => dtd .entities .get(name, storage)? .expect("active entity replacement remains registered") .as_ref(), }; - let (event, consumed) = - scan_preflight_event(&source[self.offset..], self.context, dtd, state, budget)?; + let (event, consumed) = scan_preflight_event( + &source[self.offset..], + self.context, + dtd, + state, + budget, + depth, + )?; self.offset = self.offset.saturating_add(consumed); Ok((event, self.collect_doctype, self.context)) } @@ -2812,7 +3178,7 @@ fn enter_preflight_entity( context: FragmentContext, dtd: &InternalDtd, expansion_stack: &mut S::Names, - fragments: &mut S::Values>, + fragments: &mut S::Values>, state: &mut DocumentPreflightState, budget: Option<&XmlParseWorkBudget>, ) -> Result<(), XmlDocumentError> { @@ -2834,14 +3200,12 @@ fn enter_preflight_entity( storage.charge(replacement.len())?; storage.charge(replacement.len())?; storage.charge(replacement.len())?; - fragments.push(FragmentFrame { - source: FragmentSource::Entity(name), - offset: 0, - collect_doctype: false, + fragments.push(FragmentFrame::new( + FragmentSource::Entity(name, core::marker::PhantomData), context, - pending_attribute_source: storage.values(0)?, - pending_attribute_offset: 0, - })?; + false, + storage, + )?)?; } else { expansion_stack.remove(&name, storage)?; } @@ -2849,196 +3213,6 @@ fn enter_preflight_entity( Ok(()) } -fn preflight_xml_fragment( - xml: &str, - settings: DocumentParseSettings, - dtd: &mut InternalDtd, - expansion_stack: &mut S::Names, - state: &mut DocumentPreflightState, - budget: Option<&XmlParseWorkBudget>, - collect_doctype: bool, -) -> Result<(), XmlDocumentError> { - if state.nodes == 0 { - // roxmltree exposes its document root as the first semantic node. - state.nodes = 1; - } - let storage = state.storage; - // Disjoint lexical event windows make these source-sized passes linear; - // dictionary probes, expansion revisits and copies are admitted separately. - storage.charge(xml.len())?; - storage.charge(xml.len())?; - storage.charge(xml.len())?; - let mut fragments: S::Values> = storage.values(1)?; - fragments.push(FragmentFrame { - source: FragmentSource::Document, - offset: 0, - collect_doctype, - context: FragmentContext::Content, - pending_attribute_source: storage.values(0)?, - pending_attribute_offset: 0, - })?; - // Syntax diagnostics and exact parser precedence belong to the selected - // DOM pipeline; this pass stops at malformed syntax after bounding its prefix. - while !fragments.is_empty() { - storage.charge(1)?; - let (event, collect_doctype, context) = { - let frame = fragments.last_mut().expect("fragment stack is not empty"); - frame.next_event(xml, dtd, state, budget)? - }; - - match event { - PreflightEvent::DocType(doctype) => { - if collect_doctype - && settings.allow_dtd - && let Some(doctype) = doctype - { - collect_internal_dtd(&doctype, dtd)?; - } - state.in_character_data = false; - continue; - } - PreflightEvent::GeneralRef { - name, - is_character_reference, - } => { - let Some(name) = name else { - continue; - }; - if is_character_reference { - if context == FragmentContext::Content { - observe_preflight_node(state, settings, true)?; - } - continue; - } - enter_preflight_entity( - name, - context, - dtd, - expansion_stack, - &mut fragments, - state, - budget, - )?; - continue; - } - PreflightEvent::Done => { - let frame = fragments.pop().expect("fragment stack is not empty"); - if let FragmentSource::Entity(name) = frame.source { - expansion_stack.remove(&name, storage)?; - } - continue; - } - PreflightEvent::CharacterData { xml_whitespace } => { - if context == FragmentContext::Attribute { - continue; - } - if state.depth == 0 && xml_whitespace { - state.in_character_data = false; - continue; - } - let starts_semantic_node = !state.in_character_data; - state.in_character_data = true; - if starts_semantic_node { - observe_preflight_node(state, settings, false)?; - } - continue; - } - PreflightEvent::Node => { - state.in_character_data = false; - observe_preflight_node(state, settings, false)?; - continue; - } - PreflightEvent::Markup => { - state.in_character_data = false; - continue; - } - PreflightEvent::Start { .. } | PreflightEvent::Empty { .. } | PreflightEvent::End => {} - } - - if context == FragmentContext::Attribute { - continue; - } - state.in_character_data = false; - observe_preflight_markup(event, settings, &mut fragments, state)?; - } - Ok(()) -} - -fn observe_preflight_markup( - event: PreflightEvent, - settings: DocumentParseSettings, - fragments: &mut S::Values>, - state: &mut DocumentPreflightState, -) -> Result<(), XmlDocumentError> { - let storage = state.storage; - match event { - PreflightEvent::Start { - attribute_source, - namespace_declarations, - } => { - let frame = fragments - .last_mut() - .expect("active element frame remains registered"); - frame.pending_attribute_source = attribute_source; - frame.pending_attribute_offset = 0; - let changes = apply_preflight_namespace_declarations( - state, - namespace_declarations, - settings.namespace_bindings_limit, - )?; - state.namespace_scopes.push(changes)?; - observe_preflight_node(state, settings, false)?; - state.depth = state.depth.saturating_add(1); - if state.depth > settings.depth_limit { - return Err(XmlDocumentError::DocumentTooDeep { - maximum: settings.depth_limit, - actual: state.depth, - }); - } - } - PreflightEvent::Empty { - attribute_source, - namespace_declarations, - } => { - let frame = fragments - .last_mut() - .expect("active element frame remains registered"); - frame.pending_attribute_source = attribute_source; - frame.pending_attribute_offset = 0; - let changes = apply_preflight_namespace_declarations( - state, - namespace_declarations, - settings.namespace_bindings_limit, - )?; - observe_preflight_node(state, settings, false)?; - let actual = state.depth.saturating_add(1); - if actual > settings.depth_limit { - return Err(XmlDocumentError::DocumentTooDeep { - maximum: settings.depth_limit, - actual, - }); - } - restore_preflight_namespace_bindings( - &mut state.active_namespace_bindings, - changes, - storage, - )?; - } - PreflightEvent::End => { - state.depth = state.depth.saturating_sub(1); - if let Some(changes) = state.namespace_scopes.pop() { - restore_preflight_namespace_bindings( - &mut state.active_namespace_bindings, - changes, - storage, - )?; - } - } - _ => unreachable!("non-structural events continue above"), - } - Ok(()) -} - fn preflight_pi_event(remaining: &str) -> (PreflightEvent, usize) { find_bytes(&remaining.as_bytes()[2..], b"?>").map_or( (PreflightEvent::Done, remaining.len()), @@ -3066,6 +3240,7 @@ fn scan_preflight_event( dtd: &InternalDtd, state: &mut DocumentPreflightState, budget: Option<&XmlParseWorkBudget>, + depth: usize, ) -> Result<(PreflightEvent, usize), XmlDocumentError> { let storage = state.storage; if remaining.is_empty() { @@ -3157,7 +3332,7 @@ fn scan_preflight_event( Ok(( PreflightEvent::CharacterData { // Only text outside the document element is judged by this. - xml_whitespace: state.depth == 0 + xml_whitespace: depth == 0 && bytes[..end] .iter() .all(|byte| matches!(byte, b' ' | b'\t' | b'\r' | b'\n')), @@ -3179,31 +3354,26 @@ fn scan_preflight_opening( let opening = &remaining[..=end]; // Attributes matter here for their namespace declarations and DTD defaults. // A tag with neither has nothing to split, however many attributes it has. - let (attribute_source, namespace_declarations) = - if dtd.attribute_defaults.len() == 0 && !opening.contains("xmlns") { - ( - element_attribute_reference_source(opening, "", &[], dtd)?, - storage.values(0)?, - ) - } else { - let (element_name, attributes) = opening_tag_attributes(opening, storage)?; - ( - element_attribute_reference_source(opening, element_name, &attributes, dtd)?, - namespace_declarations(element_name, &attributes, dtd, state, budget)?, - ) - }; + let (attribute_source, namespace_declarations) = if dtd.attribute_defaults.len() == 0 + && !opening.contains("xmlns") + { + ( + element_attribute_reference_source(opening, "", &[], dtd)?, + storage.values(0)?, + ) + } else { + let (element_name, attributes) = opening_tag_attributes(opening, storage)?; + ( + element_attribute_reference_source(opening, element_name, &attributes, dtd)?, + namespace_declarations(element_name, attributes.iter().copied(), dtd, state, budget)?, + ) + }; // The frame later revisits this attribute/reference window. storage.charge(attribute_source.len())?; - let event = if opening[..opening.len() - 1].trim_end().ends_with('/') { - PreflightEvent::Empty { - attribute_source, - namespace_declarations, - } - } else { - PreflightEvent::Start { - attribute_source, - namespace_declarations, - } + let event = PreflightEvent::Start { + attribute_source, + namespace_declarations, + empty: opening[..opening.len() - 1].trim_end().ends_with('/'), }; Ok((event, end + 1)) } @@ -3251,16 +3421,19 @@ struct NamespaceDeclaration { is_undeclaration: bool, } -fn namespace_declarations( +/// The namespace declarations an element named `element_name` makes, with +/// its `attributes` as name and raw value, and the defaults `dtd` gives it. +fn namespace_declarations<'a, S: NameStorage>( element_name: &str, - attributes: &[(&str, &str)], + attributes: impl Iterator + Clone, dtd: &InternalDtd, state: &mut DocumentPreflightState, budget: Option<&XmlParseWorkBudget>, ) -> Result>, XmlDocumentError> { let storage = state.storage; let mut declarations = storage.values(0)?; - for &(name, value) in attributes { + let given = attributes.clone(); + for (name, value) in attributes { storage.charge(name.len())?; let prefix = if name == "xmlns" { "" @@ -3278,7 +3451,7 @@ fn namespace_declarations( if let Some(defaults) = dtd.attribute_defaults.get(element_name, storage)? { for default in &**defaults { let mut present = false; - for &(name, _) in attributes { + for (name, _) in given.clone() { storage.charge( name.len() .min(default.attribute_name.len()) @@ -3368,62 +3541,6 @@ fn namespace_value_expands_to_empty<'a, S: NameStorage>( Ok(true) } -fn apply_preflight_namespace_declarations( - state: &mut DocumentPreflightState, - declarations: S::Values>, - maximum: usize, -) -> Result, XmlDocumentError> { - let storage = state.storage; - let mut changes = storage.values(declarations.len())?; - for declaration in declarations { - storage.charge(1)?; - let was_active = if declaration.is_undeclaration { - state - .active_namespace_bindings - .remove(&declaration.prefix, storage)? - .is_some() - } else if state - .active_namespace_bindings - .get(&declaration.prefix, storage)? - .is_some() - { - true - } else { - state - .active_namespace_bindings - .insert(storage.text(&declaration.prefix)?, ())?; - false - }; - changes.push((declaration.prefix, was_active))?; - } - let actual = state.active_namespace_bindings.len(); - state.peak_namespace_bindings = state.peak_namespace_bindings.max(actual); - if actual > maximum { - return Err(XmlDocumentError::Parse( - ParseError::NamespaceBindingLimitReached { maximum, actual }, - )); - } - Ok(changes) -} - -fn restore_preflight_namespace_bindings( - active: &mut S::Names, - mut changes: S::Values<(S::Text, bool)>, - storage: S, -) -> Result<(), XmlDocumentError> { - while let Some((prefix, was_active)) = changes.pop() { - storage.charge(1)?; - if was_active { - if active.get(&prefix, storage)?.is_none() { - active.insert(prefix, ())?; - } - } else { - active.remove(&prefix, storage)?; - } - } - Ok(()) -} - fn element_attribute_reference_source( opening: &str, element_name: &str, @@ -3595,56 +3712,51 @@ fn charge_entity_expansion_work( Ok(()) } -pub(crate) fn preflight_dom_limits( +/// Parses `xml` for the DOM API, under `options` and the absolute ceilings, +/// with the limits checked in one lexical pass or by the parser itself. +pub(crate) fn parse_dom_checked( xml: &str, options: ParsingOptions, -) -> Result { - let effective = ParsingOptions { + backend: XmlBackend, +) -> Result, ParseError> { + let options = ParsingOptions { allow_dtd: options.allow_dtd, nodes_limit: options .nodes_limit .min(crate::hard_limits::XML_DOCUMENT_NODE_CEILING), }; let settings = DocumentParseSettings { - backend: XmlBackend::default(), - allow_dtd: effective.allow_dtd, - nodes_limit: effective.nodes_limit, + backend, + allow_dtd: options.allow_dtd, + nodes_limit: options.nodes_limit, depth_limit: crate::hard_limits::XML_DOCUMENT_DEPTH_CEILING, namespace_bindings_limit: crate::hard_limits::XML_NAMESPACE_BINDING_CEILING, max_bytes: crate::hard_limits::XML_DOCUMENT_BYTE_CEILING, }; - match preflight_document_limits(xml, settings, None) { - Ok(_) => Ok(effective), - Err(XmlDocumentError::Parse(error)) => Err(error), - Err(XmlDocumentError::DocumentTooDeep { maximum, actual }) => { - Err(ParseError::DepthLimitReached { maximum, actual }) + check_document_size(xml, settings) + .and_then(|()| parse_checked(xml, settings, None)) + .map(|(document, ..)| document) + .map_err(|error| match error { + XmlDocumentError::Parse(error) => error, + error => dom_parse_error(&error), + }) +} + +/// The DOM API's error for a refusal of the document layer. +fn dom_parse_error(error: &XmlDocumentError) -> ParseError { + match error { + XmlDocumentError::Parse(error) => error.clone(), + &XmlDocumentError::DocumentTooDeep { maximum, actual } => { + ParseError::DepthLimitReached { maximum, actual } } - Err(XmlDocumentError::DocumentTooLarge { maximum, actual }) => { - Err(ParseError::ByteLimitReached { maximum, actual }) + &XmlDocumentError::DocumentTooLarge { maximum, actual } => { + ParseError::ByteLimitReached { maximum, actual } } - Err(error) => Err(ParseError::Backend { + error => ParseError::Backend { backend: "xml-limit-preflight", message: error.to_string(), - }), - } -} - -fn observe_preflight_node( - state: &mut DocumentPreflightState, - settings: DocumentParseSettings, - character_data: bool, -) -> Result<(), XmlDocumentError> { - if character_data { - if state.in_character_data { - return Ok(()); - } - state.in_character_data = true; - } - state.nodes = state.nodes.saturating_add(1); - if state.nodes > settings.nodes_limit { - return Err(XmlDocumentError::Parse(ParseError::NodesLimitReached)); + }, } - Ok(()) } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -3654,14 +3766,15 @@ pub(crate) enum DtdAttributeType { Tokenized, } -fn collect_internal_dtd( - doctype: &str, +/// Collect the internal entities and attribute defaults the document type +/// declaration `doctype` declares, and hand each attribute declaration to +/// `declare`. The caller admits the work of reading `doctype`. +fn collect_internal_dtd<'a, S: NameStorage>( + doctype: &'a str, dtd: &mut InternalDtd, + mut declare: impl FnMut(&'a str, &'a str, DtdAttributeType) -> Result<(), ParseError>, ) -> Result<(), XmlDocumentError> { let storage = dtd.storage; - storage.charge(doctype.len())?; - storage.charge(doctype.len())?; - storage.charge(doctype.len())?; let mut failure = None; visit_internal_dtd(doctype, |declaration| { if failure.is_some() { @@ -3679,11 +3792,12 @@ fn collect_internal_dtd( } } else if let Some(declaration) = dtd_declaration_body(declaration, "ATTLIST") { let mut attribute_failure = None; - visit_attribute_declarations(declaration, |element, attribute, _, value| { + visit_attribute_declarations(declaration, |element, attribute, kind, value| { if attribute_failure.is_some() { return; } let result = (|| -> Result<(), ParseError> { + declare(element, attribute, kind)?; if let Some(value) = value { let default = InternalAttributeDefault { attribute_name: storage.text(attribute)?, @@ -3724,6 +3838,7 @@ fn collect_internal_dtd( /// Visit actual internal-subset attribute declarations, not declaration-like /// strings inside comments, PIs, or quoted entity replacement text. +#[cfg(any(feature = "benchmark-internals", test))] pub(crate) fn visit_dtd_attributes<'a>( doctype: &'a str, mut visitor: impl FnMut(&'a str, &'a str, DtdAttributeType), @@ -4009,36 +4124,19 @@ fn find_unquoted_byte(bytes: &[u8], needle: u8, start: usize) -> Option { None } -/// Parses `source` after the limit pass that found `preflight_peak` namespace -/// bindings, or, for `None`, with a parser that enforces the limits as it -/// reads; returns the document's peak namespace bindings either way. -fn parse_semantic_document( - source: &str, +/// A semantic document with its metrics, peak namespace bindings, and +/// whether its source declares a document type. +type SemanticParse<'a> = (Document<'a>, DocumentMetrics, usize, bool); + +/// Parses `source` (see [`parse_checked`]) and measures it. +fn parse_semantic_document<'a>( + source: &'a str, settings: DocumentParseSettings, - preflight_peak: Option, -) -> Result<(Document<'_>, DocumentMetrics, usize), XmlDocumentError> { - let options = ParsingOptions { - allow_dtd: settings.allow_dtd, - nodes_limit: settings.nodes_limit, - }; - let (document, peak) = match preflight_peak { - Some(peak) => ( - Document::parse_after_limit_preflight_with_backend(source, options, settings.backend) - .map_err(XmlDocumentError::Parse)?, - peak, - ), - #[cfg(feature = "xml-backend-xmloxide")] - None => Document::parse_enforcing_limits(source, options, parse_limits(settings)).map_err( - |refused| match refused { - crate::xml::dom::Refused::Limit(refusal) => limit_refusal(refusal), - crate::xml::dom::Refused::Parse(error) => XmlDocumentError::Parse(error), - }, - )?, - #[cfg(not(feature = "xml-backend-xmloxide"))] - None => unreachable!("only xmloxide enforces limits while it parses"), - }; + budget: Option<&XmlParseWorkBudget>, +) -> Result, XmlDocumentError> { + let (document, peak, doctype) = parse_checked(source, settings, budget)?; let metrics = validate_document_metrics(&document, settings.depth_limit)?; - Ok((document, metrics, peak)) + Ok((document, metrics, peak, doctype)) } fn validate_document_metrics( @@ -4140,30 +4238,6 @@ fn allocate_document_identity(counter: &AtomicU64) -> Result, -) -> Result { - // Parse provenance records what the source actually requires, not merely - // whether its creator used a permissive policy. This lets a later strict - // operation accept ordinary XML while rejecting DTD-dependent documents. - if !settings.allow_dtd { - return Ok(false); - } - charge_semantic_parser_work(budget, xml.len())?; - Ok(Document::parse_after_limit_preflight_with_backend( - xml, - ParsingOptions { - allow_dtd: false, - nodes_limit: settings.nodes_limit, - }, - settings.backend, - ) - .is_err()) -} - /// What a node replacement must leave in its parent. #[derive(Clone, Copy, PartialEq, Eq)] enum ReplacementShape { @@ -4423,9 +4497,16 @@ mod entity_ceiling_tests; #[cfg(test)] mod replacement_tests; -#[cfg(all(test, feature = "xml-backend-xmloxide"))] +#[cfg(all( + test, + feature = "xml-backend-xmloxide", + any(feature = "xmldsig", feature = "xmlenc") +))] mod parser_limit_tests; +#[cfg(all(test, any(feature = "xmldsig", feature = "xmlenc")))] +mod scan_limit_tests; + #[cfg(test)] mod tests { use super::*; @@ -4823,15 +4904,13 @@ mod tests { #[cfg(feature = "xmldsig")] #[test] fn byte_decoding_and_parsing_share_one_work_allowance() { - // UTF-16 decoding consumes its source bytes; the limit pass, where - // the parser does not enforce the limits as it reads, and the two - // parser/projection passes then consume normalized UTF-8 bytes. + // UTF-16 decoding consumes its source bytes; the parse allowance, + // the lexical pass and the parser, then consumes the UTF-8 bytes. let xml = ""; let mut bytes = vec![0xff, 0xfe]; bytes.extend(xml.encode_utf16().flat_map(u16::to_le_bytes)); + let required = bytes.len() + selected_parser_passes() * xml.len(); for backend in XmlBackend::available() { - let passes = if backend.records_positions(xml) { 2 } else { 3 }; - let required = bytes.len() + passes * xml.len(); let mut policy = crate::policy::VerificationPolicy::default(); policy.resources.max_xml_parse_work_bytes = required - 1; assert!(matches!( @@ -4856,7 +4935,7 @@ mod tests { let settings = DocumentParseSettings::default(); let xml = r#""#; - let retained = build_semantic_cell(xml.to_owned(), settings, Some(1), 0) + let (retained, _) = build_semantic_cell(xml.to_owned(), settings, None) .expect("selected backend semantic projection must parse"); retained.with_dependent(|_, parsed| { let item = parsed @@ -4884,11 +4963,12 @@ mod tests { #[test] #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] fn node_preflight_rejects_wide_input_before_any_dom_pass() { - // The allowance covers only the streaming preflight. Reaching either - // DOM would exhaust parser work before it could report the node bound. + // The allowance covers exactly one parse: the lexical pass and the + // parser reading the source once each. A second parse, or work past + // the node bound, would exhaust it before the node refusal. let xml = ""; let resources = crate::policy::ResourcePolicy { - max_xml_parse_work_bytes: xml.len(), + max_xml_parse_work_bytes: 2 * xml.len(), ..crate::policy::ResourcePolicy::default() }; let budget = XmlParseWorkBudget::from_resources(&resources); @@ -4911,7 +4991,7 @@ mod tests { for value in [ "&", "&", "&", "<", ">", """, "'", ] { - let mut state = DocumentPreflightState::default(); + let mut state = DocumentPreflightState::new(NativeXmlStorage); let budget = XmlParseWorkBudget::with_limit(0); assert!( !namespace_value_expands_to_empty(value, &dtd, &mut state, Some(&budget)) @@ -5275,25 +5355,26 @@ mod tests { #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] fn entity_preflight_charges_nested_replacement_work() { // Repeated nested references must exhaust parser work during the - // streaming pass, before either DOM parser receives the document. + // lexical pass, before either DOM parser builds the document. The + // parse allowance, the pass and the parser each reading the source + // once, is charged first. let xml = r#" ]>&b;"#; let first_replacement = "&a;&a;&a;&a;".len(); let nested_replacement = "0123456789".len(); - let maximum = xml.len() + first_replacement + nested_replacement - 1; - let budget = XmlParseWorkBudget::with_limit(maximum); + let required = 2 * xml.len() + first_replacement + nested_replacement; + let budget = XmlParseWorkBudget::with_limit(required - 1); let settings = DocumentParseSettings::new_with_depth(true, 32, 4, xml.len()); assert!(matches!( build_cell(xml.to_owned(), settings, Some(&budget)), Err(XmlDocumentError::Policy(crate::policy::PolicyViolation::ResourceLimit { resource: crate::policy::resource_name::XML_PARSE_WORK_BYTES, - maximum: observed_maximum, + maximum, actual, - })) if observed_maximum == maximum - && actual == xml.len() + first_replacement + nested_replacement + })) if maximum == required - 1 && actual == required )); } @@ -5467,11 +5548,12 @@ mod tests { #[test] #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] fn owned_parse_preflights_depth_before_dtd_provenance() { - // Enabling internal DTD syntax must not move the provenance probe ahead - // of the allocation-free depth boundary for an ordinary document. + // Enabling internal DTD syntax adds no parse of its own: whether the + // source declares a DTD comes from the one parse, whose allowance + // (the lexical pass and the parser) is all the depth refusal needs. let xml = ""; let settings = DocumentParseSettings::new_with_depth(true, 16, 2, xml.len()); - let budget = XmlParseWorkBudget::with_limit(xml.len()); + let budget = XmlParseWorkBudget::with_limit(2 * xml.len()); assert!(matches!( XmlDocument::parse_with_settings_and_optional_budget( @@ -5496,11 +5578,11 @@ mod tests { fn selected_backend_enforces_exact_depth_boundary() { let settings = DocumentParseSettings::new_with_depth(false, 128, 2, 4_096); let accepted = nested_document(2); - build_semantic_cell(accepted, settings, Some(0), 0).expect("exact depth must parse"); + build_semantic_cell(accepted, settings, None).expect("exact depth must parse"); let rejected = nested_document(3); assert!(matches!( - build_semantic_cell(rejected, settings, Some(0), 0), + build_semantic_cell(rejected, settings, None), Err(XmlDocumentError::DocumentTooDeep { maximum: 2, actual: 3, @@ -6130,11 +6212,7 @@ mod tests { .into_iter() .filter(|backend| backend.is_available()) { - // The limit pass, where the parser does not enforce the limits - // as it reads, and the parse and its projection each charge the - // source length. - let passes = if backend.records_positions(xml) { 2 } else { 3 }; - let expected = xml.len() * passes; + let expected = xml.len() * selected_parser_passes(); let budget = XmlParseWorkBudget::with_limit(expected); parse_borrowed_with_settings_and_budget( xml, diff --git a/src/document/scan_limit_tests.rs b/src/document/scan_limit_tests.rs new file mode 100644 index 00000000..129560a3 --- /dev/null +++ b/src/document/scan_limit_tests.rs @@ -0,0 +1,134 @@ +use super::*; + +/// Documents whose limits the lexical pass that records source positions +/// checks: with a DOCTYPE, entities and attribute defaults count where they +/// expand, in content and in attribute values. The last has no DOCTYPE, for +/// a backend that reads positions from the pass whatever the document. +const DOCUMENTS: &[&str] = &[ + concat!( + r#"t">]>"#, + "&e;x&e;&e;" + ), + concat!( + r#"]>"#, + r#""# + ), + concat!( + r#"]>"#, + r#"&v;&w;"# + ), + concat!( + "\n\n", + r#"a&bg"#, + "\n" + ), +]; + +fn settings( + backend: XmlBackend, + nodes: u32, + depth: usize, + bindings: usize, +) -> DocumentParseSettings { + DocumentParseSettings { + backend, + allow_dtd: true, + nodes_limit: nodes, + depth_limit: depth, + namespace_bindings_limit: bindings, + max_bytes: 1 << 20, + } +} + +/// The backend and the node, depth and namespace-binding limits of `settings`. +fn limits(settings: DocumentParseSettings) -> (XmlBackend, u32, usize, usize) { + ( + settings.backend, + settings.nodes_limit, + settings.depth_limit, + settings.namespace_bindings_limit, + ) +} + +/// The limit refusal the scan itself makes for `xml`, before the backend +/// receives the document. +fn scan_refusal(xml: &str, settings: DocumentParseSettings) -> Option { + let mut pass = LimitPass::new(settings, None, NativeXmlStorage).expect("the pass sets up"); + let parsed = Document::parse_observed_with_backend( + xml, + parsing_options(settings), + settings.backend, + &mut pass, + ); + let refusal = pass + .run + .counter + .refusal() + .map(|refusal| format!("{:?}", limit_refusal(refusal))); + assert!(refusal.is_none() || parsed.is_err(), "{xml}"); + refusal +} + +#[test] +fn the_scan_refuses_each_limit_where_the_limit_pass_does() { + // The pass that records source positions checks the limits in place of + // a separate pass before it. For every document, backend that reads the + // positions, and value of each limit from none past what the document + // needs, the parse must refuse with the error the separate pass reports, + // from the scan, or accept with the same peak bindings. + // Differential mode also compares the backends' source ranges, which + // differ for markup an entity expands; the limits are each backend's. + for backend in XmlBackend::available().filter(|backend| *backend != XmlBackend::Differential) { + let (mut refusals, mut acceptances) = (0, 0); + for &xml in DOCUMENTS { + #[cfg(feature = "xml-backend-xmloxide")] + if backend.records_positions(xml) { + continue; + } + let cases = (0..40) + .map(|nodes| settings(backend, nodes, 64, 64)) + .chain((0..6).map(|depth| settings(backend, u32::MAX, depth, 64))) + .chain((0..5).map(|bindings| settings(backend, u32::MAX, 64, bindings))); + for case in cases { + let expected = preflight_document_limits(xml, case, None) + .map_err(|error| format!("{error:?}")); + let actual = XmlDocument::parse_with_settings(xml.to_owned(), case) + .map(|document| document.with_view(|view| view.parsed.peak_namespace_bindings)) + .map_err(|error| format!("{error:?}")); + assert_eq!(actual, expected, "{xml}: {:?}", limits(case)); + assert_eq!( + scan_refusal(xml, case), + actual.as_ref().err().cloned(), + "{xml}: {:?}", + limits(case) + ); + refusals += usize::from(actual.is_err()); + acceptances += usize::from(actual.is_ok()); + } + } + // Both outcomes must occur for the comparison to mean anything. + assert!( + refusals >= 20 && acceptances >= 20, + "{backend:?}: {refusals} refusals, {acceptances} acceptances" + ); + } +} + +#[test] +fn a_document_declaring_a_dtd_records_it_from_the_one_parse() { + // Whether the source requires its DTD comes from the parse itself; a + // comment that only mentions a DOCTYPE declares none. + for (xml, declares) in [ + ("", true), + (r#"]>&e;"#, true), + ("", false), + ("", false), + ] { + for backend in XmlBackend::available() { + let parse = settings(backend, u32::MAX, 64, 64); + let (_, doctype) = build_cell_recording(xml.to_owned(), parse, None, 0) + .unwrap_or_else(|error| panic!("{xml}: {backend:?}: {error:?}")); + assert_eq!(doctype, declares, "{xml}: {backend:?}"); + } + } +} diff --git a/src/xml/dom/limits.rs b/src/xml/dom/limits.rs index e1dfb1ad..956f6efa 100644 --- a/src/xml/dom/limits.rs +++ b/src/xml/dom/limits.rs @@ -74,6 +74,11 @@ impl LimitCounter { self.peak_namespace_bindings } + /// The elements open now. + pub(crate) fn depth(&self) -> usize { + self.depth + } + /// Record `refusal` and return the error that stops the parser. fn refuse(&mut self, refusal: LimitRefusal) -> ParseError { self.refusal = Some(refusal); @@ -167,12 +172,15 @@ impl LimitCounter { } } - /// The end tag of the innermost open element. + /// The end tag of the innermost open element. A lexical pass, which does + /// not check well-formedness, may report an end tag nothing opened; the + /// parser that follows refuses that document. pub(crate) fn end_element(&mut self) -> Result<(), ParseError> { self.in_character_data = false; - debug_assert!(self.depth > 0, "an end tag follows its start tag"); - // The parser reports an end tag only for an element it opened. - self.depth -= 1; + let Some(depth) = self.depth.checked_sub(1) else { + return Ok(()); + }; + self.depth = depth; match self.scopes.pop() { Some(changes) => self.restore(changes), None => Ok(()), diff --git a/src/xml/dom/mod.rs b/src/xml/dom/mod.rs index c97c7bec..f1f3938b 100644 --- a/src/xml/dom/mod.rs +++ b/src/xml/dom/mod.rs @@ -8,7 +8,6 @@ pub(crate) mod benchmark; #[cfg(all(feature = "xml-backend-roxmltree", feature = "xml-backend-xmloxide"))] mod differential; -#[cfg(feature = "xml-backend-xmloxide")] mod limits; mod preflight; #[cfg(feature = "xml-backend-roxmltree")] @@ -22,9 +21,9 @@ mod xmloxide; use alloc::string::String; use core::fmt; -#[cfg(feature = "xml-backend-xmloxide")] pub(crate) use self::limits::{LimitCounter, LimitRefusal, ParseLimits}; use self::preflight::LexicalPreflight; +pub(crate) use self::preflight::ScanObserver; #[cfg(feature = "xml-backend-xmloxide")] pub(crate) use self::preflight::{PositionRecorder, SourceKind, SourceNode}; @@ -110,10 +109,9 @@ impl XmlBackend { /// range and enforces the policy limits as it reads, and only a DOCTYPE, /// whose keyword is case-sensitive (XML 1.0 production 28), brings /// declarations and entity expansion, which a scan must bound first. + #[cfg(feature = "xml-backend-xmloxide")] pub(crate) fn records_positions(self, input: &str) -> bool { - cfg!(feature = "xml-backend-xmloxide") - && self == Self::Xmloxide - && !input.contains("( diff --git a/src/xml/dom/preflight.rs b/src/xml/dom/preflight.rs index 32e9c22a..95efb812 100644 --- a/src/xml/dom/preflight.rs +++ b/src/xml/dom/preflight.rs @@ -161,6 +161,119 @@ impl<'input, 'w, W: structured_asn1::memory::Work> } } +/// The checks the pass before the parser makes on what it reads, beside +/// recording source positions, so that one lexical pass serves both. +pub(crate) trait ScanObserver<'input> { + /// A document type declaration, read only where DTDs are allowed; every + /// attribute declaration of its internal subset goes to `attribute`. + /// + /// # Errors + /// The check's refusal, or `attribute`'s. + fn doctype(&mut self, doctype: &'input str, attribute: F) -> Result<(), ParseError> + where + F: FnMut( + &'input str, + &'input str, + crate::document::DtdAttributeType, + ) -> Result<(), ParseError>; + + /// A start tag or, with `empty`, an empty-element tag: `tag` is its + /// source, which starts at `tag_start` in the document. + /// + /// # Errors + /// The check's refusal. + fn start_element( + &mut self, + tag: &'input str, + tag_start: usize, + attributes: &[xml_sec_xml_input::lexical::Attribute<'input>], + empty: bool, + ) -> Result<(), ParseError>; + + /// An end tag. + /// + /// # Errors + /// The check's refusal. + fn end_element(&mut self) -> Result<(), ParseError>; + + /// Character data inside the document element: text, CDATA, or a + /// predefined or character reference. + /// + /// # Errors + /// The check's refusal. + fn character_data(&mut self) -> Result<(), ParseError>; + + /// A reference to the general entity `name` in content. + /// + /// # Errors + /// The check's refusal. + fn entity_reference(&mut self, name: &'input str) -> Result<(), ParseError>; + + /// A comment or processing instruction. + /// + /// # Errors + /// The check's refusal. + fn node(&mut self) -> Result<(), ParseError>; + + /// Markup that is no node: the XML declaration, a document type + /// declaration, or whitespace outside the document element. + fn markup(&mut self); +} + +/// The scan of a parse that checks nothing beyond the parser's own ceilings. +#[cfg(any(feature = "benchmark-internals", test))] +pub(crate) struct Unchecked; + +#[cfg(any(feature = "benchmark-internals", test))] +impl<'input> ScanObserver<'input> for Unchecked { + fn doctype(&mut self, doctype: &'input str, mut attribute: F) -> Result<(), ParseError> + where + F: FnMut( + &'input str, + &'input str, + crate::document::DtdAttributeType, + ) -> Result<(), ParseError>, + { + let mut failure = None; + crate::document::visit_dtd_attributes(doctype, |element, name, kind| { + if failure.is_none() + && let Err(error) = attribute(element, name, kind) + { + failure = Some(error); + } + }); + failure.map_or(Ok(()), Err) + } + + fn start_element( + &mut self, + _: &'input str, + _: usize, + _: &[xml_sec_xml_input::lexical::Attribute<'input>], + _: bool, + ) -> Result<(), ParseError> { + Ok(()) + } + + fn end_element(&mut self) -> Result<(), ParseError> { + Ok(()) + } + + fn character_data(&mut self) -> Result<(), ParseError> { + Ok(()) + } + + fn entity_reference(&mut self, _: &'input str) -> Result<(), ParseError> { + Ok(()) + } + + fn node(&mut self) -> Result<(), ParseError> { + Ok(()) + } + + fn markup(&mut self) {} +} + pub(super) struct LexicalPreflight<'input, S: Storage = Native> { nodes: S::Values, attributes: S::Declarations<'input>, @@ -173,14 +286,24 @@ pub(super) struct LexicalPreflight<'input, S: Storage = Native> { } impl<'input> LexicalPreflight<'input> { + #[cfg(any(feature = "benchmark-internals", test))] pub(super) fn scan(input: &'input str, allow_dtd: bool) -> Result { Self::scan_in(input, allow_dtd, Native) } + + pub(super) fn scan_observed>( + input: &'input str, + allow_dtd: bool, + observer: &mut O, + ) -> Result { + Self::scan_observed_in(input, allow_dtd, Native, observer) + } } impl<'input, S: Storage> LexicalPreflight<'input, S> { /// The preflight of a document that declares nothing: no attribute /// declarations exist, and the parser supplies the positions. + #[cfg(feature = "xml-backend-xmloxide")] pub(super) fn unscanned(storage: S) -> Result { Ok(Self { nodes: storage.values(0)?, @@ -198,10 +321,22 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { self.unscanned } + #[cfg(any(feature = "benchmark-internals", test))] pub(super) fn scan_in( input: &'input str, allow_dtd: bool, storage: S, + ) -> Result { + Self::scan_observed_in(input, allow_dtd, storage, &mut Unchecked) + } + + /// The scan, with `observer` checking each event before its position is + /// recorded, so the observer's refusal precedes the position ceilings. + pub(super) fn scan_observed_in>( + input: &'input str, + allow_dtd: bool, + storage: S, + observer: &mut O, ) -> Result { let mut reader = Scanner::with_storage(input, storage); let mut nodes: S::Values = storage.values(0)?; @@ -213,31 +348,30 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { storage.charge(1)?; match event { Event::Start(element) => { - let depth = elements.len() + 1; - enforce_depth(depth)?; - let index = nodes.len(); - push_source_position( - &mut nodes, - SourceNode { - kind: SourceKind::Element, - range: element.range, - }, + observer.start_element( + &input[element.range.clone()], + element.range.start, + &element.attributes, + false, )?; - elements.push(index)?; + enforce_depth(elements.len() + 1)?; + elements.push(nodes.len())?; + push_leaf(&mut nodes, SourceKind::Element, element.range)?; reader.recycle(element.attributes); } Event::Empty(element) => { - enforce_depth(elements.len() + 1)?; - push_source_position( - &mut nodes, - SourceNode { - kind: SourceKind::Element, - range: element.range, - }, + observer.start_element( + &input[element.range.clone()], + element.range.start, + &element.attributes, + true, )?; + enforce_depth(elements.len() + 1)?; + push_leaf(&mut nodes, SourceKind::Element, element.range)?; reader.recycle(element.attributes); } Event::End { range, .. } => { + observer.end_element()?; if let Some(index) = elements.pop() { nodes[index].range.end = range.end; } @@ -245,40 +379,30 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { // Both supported DOMs omit whitespace outside the document // element, so it must not shift the semantic sidecar. Event::Text { range, .. } if !elements.is_empty() => { + observer.character_data()?; push_text_position(&mut nodes, range)?; } - Event::Text { .. } | Event::Declaration { .. } => {} - Event::CData { range, .. } => push_source_position( - &mut nodes, - SourceNode { - kind: SourceKind::CData, - range, - }, - )?, + Event::Text { .. } | Event::Declaration { .. } => observer.markup(), + Event::CData { range, .. } => { + observer.character_data()?; + push_leaf(&mut nodes, SourceKind::CData, range)?; + } Event::Reference { name, range } if is_builtin_or_character_reference(name) => { + observer.character_data()?; push_text_position(&mut nodes, range)?; } - Event::Reference { range, .. } => push_source_position( - &mut nodes, - SourceNode { - kind: SourceKind::EntityRef, - range, - }, - )?, - Event::Comment { range, .. } => push_source_position( - &mut nodes, - SourceNode { - kind: SourceKind::Comment, - range, - }, - )?, - Event::ProcessingInstruction { range, .. } => push_source_position( - &mut nodes, - SourceNode { - kind: SourceKind::Pi, - range, - }, - )?, + Event::Reference { name, range } => { + observer.entity_reference(name)?; + push_leaf(&mut nodes, SourceKind::EntityRef, range)?; + } + Event::Comment { range, .. } => { + observer.node()?; + push_leaf(&mut nodes, SourceKind::Comment, range)?; + } + Event::ProcessingInstruction { range, .. } => { + observer.node()?; + push_leaf(&mut nodes, SourceKind::Pi, range)?; + } Event::DocType { .. } if !allow_dtd => return Err(ParseError::DtdDetected.into()), Event::DocType { range, .. } => { // Internal declaration scanning is bounded by the actual @@ -286,11 +410,10 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { storage.charge(range.len())?; storage.charge(range.len())?; storage.charge(range.len())?; - collect_attribute_declarations( - &input[range.clone()], - &mut attributes, - storage, - )?; + observer.markup(); + observer.doctype(&input[range.clone()], |element, attribute, kind| { + declare_attribute(&mut attributes, storage, element, attribute, kind) + })?; #[cfg(feature = "xml-backend-roxmltree")] { doctype = Some(range); @@ -374,49 +497,40 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { } } -fn collect_attribute_declarations<'input, S: Storage>( - doctype: &'input str, +/// Record the internal-subset declaration of `attribute` on `element`. +fn declare_attribute<'input, S: Storage>( attributes: &mut S::Declarations<'input>, storage: S, + element: &'input str, + attribute: &'input str, + kind: crate::document::DtdAttributeType, ) -> Result<(), ParseError> { - let mut failure = None; - crate::document::visit_dtd_attributes(doctype, |element, attribute, kind| { - if failure.is_some() { - return; - } - let result = (|| -> Result<(), ParseError> { - storage.charge(element.len())?; - storage.charge(attribute.len())?; - let (element_prefix, element_local) = split_name(element); - let (attribute_prefix, attribute_local) = split_name(attribute); - // XML 1.0 3.3: the first declaration is binding. - // https://www.w3.org/TR/2008/REC-xml-20081126/#attdecls - let key = DtdAttributeName( - element_prefix, - element_local, - attribute_prefix, - attribute_local, - ); - if let Some(info) = attributes.get_mut(&key, storage)? { - // xml:id 4 constrains all declarations, even shadowed ones. - // https://www.w3.org/TR/2005/REC-xml-id-20050909/#processing - info.has_non_id_declaration |= kind != crate::document::DtdAttributeType::Id; - } else { - attributes.insert( - key, - DtdAttributeInfo { - kind, - has_non_id_declaration: kind != crate::document::DtdAttributeType::Id, - }, - )?; - } - Ok(()) - })(); - if let Err(error) = result { - failure = Some(error); - } - }); - failure.map_or(Ok(()), Err) + storage.charge(element.len())?; + storage.charge(attribute.len())?; + let (element_prefix, element_local) = split_name(element); + let (attribute_prefix, attribute_local) = split_name(attribute); + // XML 1.0 3.3: the first declaration is binding. + // https://www.w3.org/TR/2008/REC-xml-20081126/#attdecls + let key = DtdAttributeName( + element_prefix, + element_local, + attribute_prefix, + attribute_local, + ); + if let Some(info) = attributes.get_mut(&key, storage)? { + // xml:id 4 constrains all declarations, even shadowed ones. + // https://www.w3.org/TR/2005/REC-xml-id-20050909/#processing + info.has_non_id_declaration |= kind != crate::document::DtdAttributeType::Id; + } else { + attributes.insert( + key, + DtdAttributeInfo { + kind, + has_non_id_declaration: kind != crate::document::DtdAttributeType::Id, + }, + )?; + } + Ok(()) } fn split_name(name: &str) -> (Option<&str>, &str) { @@ -477,6 +591,14 @@ fn push_text_position>( Ok(()) } +fn push_leaf>( + nodes: &mut V, + kind: SourceKind, + range: Range, +) -> Result<(), ParseError> { + push_source_position(nodes, SourceNode { kind, range }) +} + fn push_source_position>( nodes: &mut V, node: SourceNode, diff --git a/src/xml/dom/tree.rs b/src/xml/dom/tree.rs index 724772f4..a98f79c7 100644 --- a/src/xml/dom/tree.rs +++ b/src/xml/dom/tree.rs @@ -489,23 +489,39 @@ impl<'input> Document<'input> { options: ParsingOptions, backend: XmlBackend, ) -> Result { - let options = crate::document::preflight_dom_limits(input, options)?; - Self::parse_after_limit_preflight_with_backend(input, options, backend) + crate::document::parse_dom_checked(input, options, backend) } - pub(crate) fn parse_after_limit_preflight_with_backend( + /// Parses an input whose lexical pass before the parser `observer` + /// checks as it records the source positions. + pub(crate) fn parse_observed_with_backend>( input: &'input str, options: ParsingOptions, backend: XmlBackend, + observer: &mut O, ) -> Result { - let preflight = if backend.records_positions(input) { - super::LexicalPreflight::unscanned(super::storage::Native)? - } else { - super::LexicalPreflight::scan(input, options.allow_dtd)? - }; + let preflight = super::LexicalPreflight::scan_observed(input, options.allow_dtd, observer)?; backend.parse(input, options, &preflight) } + /// [`parse_observed_with_backend`](Self::parse_observed_with_backend) in + /// the current query's storage. + #[cfg(feature = "xmldsig")] + pub(crate) fn parse_observed_in<'w, W: structured_asn1::memory::Work, O>( + input: &'input str, + options: ParsingOptions, + backend: XmlBackend, + storage: super::storage::Accounted<'w, W>, + observer: &mut O, + ) -> Result>, super::AccountedParseError> + where + O: super::ScanObserver<'input>, + { + let preflight = + super::LexicalPreflight::scan_observed_in(input, options.allow_dtd, storage, observer)?; + backend.parse_in(input, options, &preflight, storage) + } + /// Parses an input whose policy limits the backend enforces as it reads /// (see [`XmlBackend::records_positions`]), returning the document with /// the most namespace bindings it had in scope at once. @@ -532,22 +548,6 @@ impl<'input> Document<'input> { > { super::xmloxide::XmloxideBackend::parse_enforcing_limits_in(input, options, limits, storage) } - - #[cfg(feature = "xmldsig")] - pub(crate) fn parse_after_limit_preflight_in<'w, W: structured_asn1::memory::Work>( - input: &'input str, - options: ParsingOptions, - backend: XmlBackend, - storage: super::storage::Accounted<'w, W>, - ) -> Result>, super::AccountedParseError> - { - let preflight = if backend.records_positions(input) { - super::LexicalPreflight::unscanned(storage)? - } else { - super::LexicalPreflight::scan_in(input, options.allow_dtd, storage)? - }; - backend.parse_in(input, options, &preflight, storage) - } } impl<'input, S: Storage> Document<'input, S> { diff --git a/src/xml/dom/xmloxide.rs b/src/xml/dom/xmloxide.rs index fa62f93b..aa973579 100644 --- a/src/xml/dom/xmloxide.rs +++ b/src/xml/dom/xmloxide.rs @@ -18,6 +18,7 @@ use super::{ pub(super) struct XmloxideBackend; /// The parser's failure in storage `S`. +#[cfg(feature = "xmldsig")] type XmloxideError = xmloxide::error::ParseError< ::FrozenText, ::Values::FrozenText>>, diff --git a/src/xmlenc/parse.rs b/src/xmlenc/parse.rs index b2c339f3..096ff466 100644 --- a/src/xmlenc/parse.rs +++ b/src/xmlenc/parse.rs @@ -2462,7 +2462,7 @@ mod tests { // switch to the build default when its containing XML is revalidated. let backend = crate::XmlBackend::Roxmltree; let document = Document::parse_with_backend(DATA, backend).expect("test XML must parse"); - let expected_work = DATA.len() * 3; + let expected_work = DATA.len() * crate::document::selected_parser_passes(); let resources = crate::policy::ResourcePolicy { max_xml_parse_work_bytes: expected_work, ..crate::policy::ResourcePolicy::default() diff --git a/tests/owned_document_pipeline.rs b/tests/owned_document_pipeline.rs index 5530469d..dec8c0e3 100644 --- a/tests/owned_document_pipeline.rs +++ b/tests/owned_document_pipeline.rs @@ -96,28 +96,29 @@ fn public_owned_parser_enforces_cumulative_parse_work() { )) if actual == xml.len() )); - let dtd_capable = SigningPolicy { + // Allowing a DTD adds no parse of its own: one parse, the lexical pass + // and the parser reading the source once each, is the whole allowance. + let dtd_capable = |max_xml_parse_work_bytes| SigningPolicy { xml: xml_sec::policy::XmlInputPolicy { allow_internal_dtd: true, }, resources: ResourcePolicy { - max_xml_parse_work_bytes: xml.len() - * (1 + usize::from(cfg!(feature = "xml-backend-xmloxide"))), + max_xml_parse_work_bytes, ..ResourcePolicy::default() }, ..SigningPolicy::default() }; + XmlDocument::parse_with_policy(xml, &dtd_capable(2 * xml.len())) + .expect("one parse fits its exact allowance"); assert!(matches!( - XmlDocument::parse_with_policy(xml, &dtd_capable), + XmlDocument::parse_with_policy(xml, &dtd_capable(2 * xml.len() - 1)), Err(XmlDocumentError::Policy( xml_sec::policy::PolicyViolation::ResourceLimit { resource: "cumulative XML parse-work bytes", maximum, actual, } - )) if maximum - == xml.len() * (1 + usize::from(cfg!(feature = "xml-backend-xmloxide"))) - && actual == maximum + xml.len() + )) if maximum == 2 * xml.len() - 1 && actual == 2 * xml.len() )); } From 4b21537d8e25cfcba8465d5ab7ecc9e75e6ed9db Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 3 Nov 2026 18:16:03 +0200 Subject: [PATCH 3/5] fix(xml): skip supplied attribute defaults through a metered index 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 --- src/document.rs | 19 ++++++++++--------- src/document/scan_limit_tests.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/document.rs b/src/document.rs index 0c7b9252..2a3a257a 100644 --- a/src/document.rs +++ b/src/document.rs @@ -2852,18 +2852,19 @@ impl LimitRun<'_, S> { } } if has_defaults && let Some(defaults) = dtd.attribute_defaults.get(element, storage)? { - for (index, default) in defaults.iter().enumerate() { - storage.charge(default.value.len())?; - // An attribute the tag gives, or an earlier default supplies, - // takes no value from this default. + // An attribute the tag gives, or an earlier default supplies, takes + // no value from a default; the storage meters this index. + let mut present = storage.names(); + for (name, _) in pairs { + present.insert(name, ())?; + } + for default in &**defaults { let name = default.attribute_name.as_ref(); - if pairs.clone().any(|(given, _)| given == name) - || defaults[..index] - .iter() - .any(|earlier| earlier.attribute_name.as_ref() == name) - { + if present.get(name, storage)?.is_some() { continue; } + present.insert(name, ())?; + storage.charge(default.value.len())?; for name in general_references(default.value.as_bytes()) { self.expand(dtd, name, FragmentContext::Attribute)?; } diff --git a/src/document/scan_limit_tests.rs b/src/document/scan_limit_tests.rs index 129560a3..e4be2336 100644 --- a/src/document/scan_limit_tests.rs +++ b/src/document/scan_limit_tests.rs @@ -17,6 +17,11 @@ const DOCUMENTS: &[&str] = &[ r#"]>"#, r#"&v;&w;"# ), + concat!( + r#"">"#, + r#"]>"#, + r#""# + ), concat!( "\n\n", r#"a&bg"#, @@ -114,6 +119,32 @@ fn the_scan_refuses_each_limit_where_the_limit_pass_does() { } } +#[test] +fn the_scan_charges_the_entity_work_the_limit_pass_charges() { + // References expand in content, in given attribute values, and in the + // defaults of attributes a tag omits: the first declared default of each, + // never one the tag gives. Beyond the parse allowance (twice the source + // against the separate pass's once), both charge the same expansion work. + for backend in XmlBackend::available().filter(|backend| *backend != XmlBackend::Differential) { + for &xml in DOCUMENTS { + #[cfg(feature = "xml-backend-xmloxide")] + if backend.records_positions(xml) { + continue; + } + let case = settings(backend, u32::MAX, 64, 64); + let separate = XmlParseWorkBudget::with_limit(usize::MAX); + preflight_document_limits(xml, case, Some(&separate)).expect("the pass accepts"); + let scanned = XmlParseWorkBudget::with_limit(usize::MAX); + build_cell(xml.to_owned(), case, Some(&scanned)).expect("the parse accepts"); + assert_eq!( + scanned.consumed() - 2 * xml.len(), + separate.consumed() - xml.len(), + "{xml}: {backend:?}" + ); + } + } +} + #[test] fn a_document_declaring_a_dtd_records_it_from_the_one_parse() { // Whether the source requires its DTD comes from the parse itself; a From 99ffb0ba840d2426e330d308253136bdb711d29c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 3 Nov 2026 20:49:05 +0200 Subject: [PATCH 4/5] docs(xml): note why the parser limit tests need no xmldsig gate 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 --- src/document.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/document.rs b/src/document.rs index 2a3a257a..5900b04a 100644 --- a/src/document.rs +++ b/src/document.rs @@ -4498,6 +4498,8 @@ mod entity_ceiling_tests; #[cfg(test)] mod replacement_tests; +// `xmlenc` enables `xmldsig` through `xmldsig-sign`, so every build of this +// module has the verification policy its tests use. #[cfg(all( test, feature = "xml-backend-xmloxide", From af60543277611a9fcb1d200409684518b4a55436 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 3 Nov 2026 22:10:03 +0200 Subject: [PATCH 5/5] build(xml): record the limit-enforcing parse in the xmloxide delta 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 --- scripts/patches/xmloxide-0.5.0.patch | 236 ++++++++++++++++++--------- 1 file changed, 163 insertions(+), 73 deletions(-) diff --git a/scripts/patches/xmloxide-0.5.0.patch b/scripts/patches/xmloxide-0.5.0.patch index 1d158dce..aa9b8c86 100644 --- a/scripts/patches/xmloxide-0.5.0.patch +++ b/scripts/patches/xmloxide-0.5.0.patch @@ -88,7 +88,7 @@ f.debug_struct("ParseOptions") .field("recover", &self.recover) .field("no_blanks", &self.no_blanks) -@@ -238,6 +239,14 @@ +@@ -238,6 +239,24 @@ /// Returns `ParseError` if the input is not well-formed XML and recovery /// mode is not enabled. pub fn parse_str_with_options(input: &str, options: &ParseOptions) -> Result { @@ -104,6 +104,16 @@ +) -> Result<(Document, alloc::vec::Vec), ParseError> { + xml::XmlParser::new(input, options)?.parse_recording_positions() +} ++ ++/// Parses a document without a DOCTYPE as [`parse_str_recording_positions`] ++/// does, enforcing `limits` as it reads. ++pub(crate) fn parse_str_enforcing_limits( ++ input: &str, ++ options: &ParseOptions, ++ limits: crate::xml::dom::ParseLimits, ++) -> Result, ParseError> { ++ xml::XmlParser::new(input, options)?.parse_enforcing_limits(limits) ++} --- a/xmloxide/src/parser/input.rs +++ b/xmloxide/src/parser/input.rs @@ -19,10 +19,99 @@ @@ -4115,7 +4125,7 @@ -use crate::validation::dtd::{parse_dtd, serialize_dtd, AttributeDecl, AttributeType, EntityKind}; +use crate::xml::dom::storage::names::{NameStorage, Names}; +use crate::xml::dom::storage::{Native, Storage, Values}; -+use crate::xml::dom::{PositionRecorder, SourceKind, SourceNode}; ++use crate::xml::dom::{LimitCounter, ParseLimits, PositionRecorder, SourceKind, SourceNode}; + +use crate::parser_xmloxide::error::{ErrorSeverity, ParseDiagnostic}; +use crate::parser_xmloxide::tree::{Attribute, Document, NodeId, NodeKind}; @@ -4171,13 +4181,18 @@ /// Default maximum amplification factor for entity/attribute expansion. /// -@@ -32,56 +69,91 @@ +@@ -32,56 +69,146 @@ /// resolver-provided external entities and stack safety. const MAX_ENTITY_DEPTH: u32 = 32; +/// A parsed document with the source ranges of its nodes. +pub(crate) type Located = (Document, ::Values); +type Finished = (Document, Option>); ++/// A parse, with the limit counter that outlives a failed parse. ++type Counted = (Result, ParseError>, Option>); ++/// A parse that enforced its limits, with the counter that names a limit it ++/// stopped at. ++pub(crate) type Limited = (Result, ParseError>, LimitCounter); + /// The core XML parser. -pub(crate) struct XmlParser<'a> { @@ -4212,6 +4227,8 @@ entity_depth: u32, + /// Source ranges of the nodes, when the caller has no lexical sidecar. + positions: Option>, ++ /// The policy limits, when no pass before the parser enforced them. ++ limits: Option>, } impl<'a> XmlParser<'a> { @@ -4259,29 +4276,77 @@ entity_depth: 0, - } + positions: None, ++ limits: None, + }) } /// Main parse entry point. Parses the entire document. - pub fn parse(&mut self) -> Result { + pub fn parse(self) -> Result, ParseError> { -+ self.parse_tree().map(|(document, _)| document) ++ self.parse_tree().0.map(|(document, _)| document) + } + + /// Parses a document without a DOCTYPE and returns its nodes' source ranges, + /// in the order the projector visits them. + pub(crate) fn parse_recording_positions(mut self) -> Result, ParseError> { + self.positions = Some(PositionRecorder::new(self.storage)?); -+ let (document, positions) = self.parse_tree()?; ++ let (document, positions) = self.parse_tree().0?; + let positions = positions.expect("recording was enabled above"); + Ok((document, positions.into_nodes())) + } + -+ fn parse_tree(mut self) -> Result, ParseError> { ++ /// Parses a document without a DOCTYPE as ++ /// [`parse_recording_positions`](Self::parse_recording_positions) does, ++ /// refusing it at the first of `limits` it passes before allocating the ++ /// node that passes it. ++ /// ++ /// # Errors ++ /// Returns a resource refusal when the counter cannot be set up; the ++ /// parse's own outcome is the first member of the result. ++ pub(crate) fn parse_enforcing_limits( ++ mut self, ++ limits: ParseLimits, ++ ) -> Result, ParseError> { ++ self.positions = Some(PositionRecorder::new(self.storage)?); ++ self.limits = Some(LimitCounter::new(limits, self.storage)?); ++ let (parsed, counter) = self.parse_tree(); ++ let located = parsed.map(|(document, positions)| { ++ let positions = positions.expect("recording was enabled above"); ++ (document, positions.into_nodes()) ++ }); ++ Ok((located, counter.expect("limits were enabled above"))) ++ } ++ ++ /// Report a construct to the limit counter, if the parse enforces limits. ++ fn limit( ++ &mut self, ++ report: impl FnOnce(&mut LimitCounter) -> Result<(), crate::xml::dom::ParseError>, ++ ) -> Result<(), ParseError> { ++ if let Some(limits) = &mut self.limits { ++ report(limits)?; ++ } ++ Ok(()) ++ } ++ ++ /// The parse, and the limit counter, which outlives a failed parse. ++ fn parse_tree(mut self) -> Counted { ++ let parsed = self.parse_document(); ++ let limits = self.limits.take(); ++ let finished = parsed.map(|()| { ++ // Sync diagnostics from input to document before returning. ++ self.doc.diagnostics = self.input.diagnostics; ++ // Parsing is complete and the parser is consumed. Moving the arena ++ // avoids Document::default allocating a replacement tree on return. ++ (self.doc, self.positions) ++ }); ++ (finished, limits) ++ } ++ ++ fn parse_document(&mut self) -> Result<(), ParseError> { // Parse optional XML declaration — must be at the very start of the // document with no leading whitespace (XML 1.0 §2.8). if self.input.looking_at(b" Result<(NodeId, Option), ParseError> { -+ self.input.increment_depth()?; -+ if let Some(positions) = &mut self.positions { -+ positions.start_element(self.input.pos())?; -+ } ++ let start = self.input.pos(); + self.input.expect_byte(b'<')?; + let name = self.input.parse_name()?; + let mut attributes = self.storage.values(0)?; @@ -5312,20 +5374,27 @@ - attr.namespace = resolved; - } - } -+ self.check_attribute_spellings(&attributes)?; -+ self.apply_attribute_defaults(name, &mut attributes)?; -+ -+ // --- Namespace processing (Namespaces in XML 1.0 section 3) --- -+ -+ // Check for namespace declarations to skip namespace scope push/pop -+ // when not needed. Skip the scan entirely when there are no attributes. -+ let has_ns_decls = !attributes.is_empty() -+ && attributes.iter().any(|a| { -+ a.prefix.as_deref() == Some("xmlns") -+ || (a.prefix.is_none() && a.name.as_ref() == "xmlns") -+ }); -+ if has_ns_decls { -+ self.ns.push_scope()?; ++ // The tag is read; it is refused for a limit before it is checked ++ // or allocated, as a pass before the parser refused it. ++ let empty = self.input.looking_at(b"/>"); ++ self.limit(|limits| { ++ limits.start_element( ++ attributes.iter().filter_map(|attribute| { ++ let prefix = match attribute.prefix.as_deref() { ++ Some("xmlns") => attribute.name.as_ref(), ++ None if attribute.name.as_ref() == "xmlns" => "", ++ _ => return None, ++ }; ++ Some((prefix, attribute.value.is_empty())) ++ }), ++ empty, ++ ) ++ })?; ++ // The absolute nesting ceilings follow the policy's own depth limit, ++ // which may equal them and is the refusal a document reports. ++ self.input.increment_depth()?; ++ if let Some(positions) = &mut self.positions { ++ positions.start_element(start)?; } - // Namespace-aware attribute uniqueness: two attributes with the same @@ -5368,6 +5437,22 @@ - } - } - } ++ self.check_attribute_spellings(&attributes)?; ++ self.apply_attribute_defaults(name, &mut attributes)?; ++ ++ // --- Namespace processing (Namespaces in XML 1.0 section 3) --- ++ ++ // Check for namespace declarations to skip namespace scope push/pop ++ // when not needed. Skip the scan entirely when there are no attributes. ++ let has_ns_decls = !attributes.is_empty() ++ && attributes.iter().any(|a| { ++ a.prefix.as_deref() == Some("xmlns") ++ || (a.prefix.is_none() && a.name.as_ref() == "xmlns") ++ }); ++ if has_ns_decls { ++ self.ns.push_scope()?; ++ } ++ + // Split into prefix and local name for namespace processing. + let (prefix, local_name) = split_name(name); + @@ -5518,6 +5603,7 @@ + if let Some(positions) = &mut self.positions { + positions.end_element(self.input.pos()); + } ++ self.limit(LimitCounter::end_element)?; // Pop the namespace scope when leaving this element (only if we pushed). if has_ns_decls { @@ -5531,7 +5617,7 @@ } /// Normalizes an attribute value based on its DTD-declared type. -@@ -921,105 +955,61 @@ +@@ -921,105 +1028,62 @@ element_name: &str, attr_name: &str, value: &str, @@ -5601,6 +5687,7 @@ - fn parse_char_data(&mut self, parent: NodeId) -> Result<(), ParseError> { - let mut text = String::new(); + fn parse_char_data(&mut self, parent: NodeId) -> Result<(), ParseError> { ++ self.limit(LimitCounter::character_data)?; + let source_start = self.input.pos(); + let mut text = self.storage.text("")?; @@ -5663,7 +5750,7 @@ } } continue; -@@ -1034,10 +1024,10 @@ +@@ -1034,10 +1098,10 @@ if self.options.recover { self.input.push_diagnostic( ErrorSeverity::Error, @@ -5678,7 +5765,7 @@ continue; } return Err(self.input.fatal("']]>' not allowed in character data")); -@@ -1049,15 +1039,13 @@ +@@ -1049,15 +1113,13 @@ // node is created and the entity's replacement text is parsed // as content, attached as children of the EntityRef node. // Skip the peek for builtins at byte level to avoid String allocation. @@ -5699,7 +5786,7 @@ self.doc.append_child(parent, text_id); } self.parse_entity_ref_in_content(parent)?; -@@ -1066,17 +1054,24 @@ +@@ -1066,17 +1128,24 @@ self.input.parse_reference_into(&mut text)?; } else { let ch = self.input.next_char()?; @@ -5728,7 +5815,7 @@ } Ok(()) -@@ -1089,60 +1084,82 @@ +@@ -1089,60 +1158,82 @@ /// replacement text (XML 1.0 §4.5: character references expanded at /// declaration time) as content, attaching the resulting nodes as /// children of the `EntityRef` node. @@ -5855,7 +5942,7 @@ self.doc.append_child(parent, ref_id); let Some(replacement) = replacement else { -@@ -1160,18 +1177,16 @@ +@@ -1160,18 +1251,16 @@ .fatal("maximum entity amplification factor exceeded")); } @@ -5883,7 +5970,7 @@ Ok(()) } -@@ -1187,14 +1202,15 @@ +@@ -1187,14 +1276,15 @@ entity_name: &str, replacement: &str, ref_id: NodeId, @@ -5902,7 +5989,7 @@ sub.entity_depth = self.entity_depth + 1; sub.input_size = self.input_size; sub.expansion_size = self.expansion_size; -@@ -1204,51 +1220,50 @@ +@@ -1204,51 +1294,50 @@ // enough to overflow the stack). sub.input.set_depth(self.input.depth()); // Move shared state into the sub-parser (returned below). @@ -5974,7 +6061,7 @@ Ok(()) } -@@ -1258,7 +1273,7 @@ +@@ -1258,7 +1347,7 @@ /// Unlike [`parse_content`](Self::parse_content), end of input is the /// normal termination and an end tag (` bool { let remaining = self.input.remaining(); -@@ -1306,58 +1310,29 @@ +@@ -1306,58 +1384,29 @@ || after_amp.starts_with(b"quot;") } @@ -6076,7 +6163,7 @@ } i = j + 1; } else { -@@ -1367,17 +1342,17 @@ +@@ -1367,17 +1416,17 @@ i += 1; } } @@ -6098,7 +6185,7 @@ // Capture raw attribute value text (before entity expansion) so we // can preserve entity references during serialization. -@@ -1390,13 +1365,20 @@ +@@ -1390,13 +1439,20 @@ // contain no '&' (most attributes have no entity references). let raw_value = if raw_end > raw_start + 2 { let raw_bytes = self.input.slice(raw_start + 1, raw_end - 1); @@ -6121,7 +6208,7 @@ } else { None } -@@ -1404,12 +1386,12 @@ +@@ -1404,12 +1460,12 @@ None }; @@ -6137,7 +6224,7 @@ namespace: None, raw_value, }) -@@ -1418,51 +1400,51 @@ +@@ -1418,51 +1474,54 @@ // --- Comments --- // See XML 1.0 §2.5: [15] Comment @@ -6146,6 +6233,7 @@ + let start = self.input.pos(); let content = parse_comment_content(&mut self.input)?; - let comment_id = self.doc.create_node(NodeKind::Comment { content }); ++ self.limit(LimitCounter::node)?; + let comment_id = self.doc.try_create_node(NodeKind::Comment { + content: self.storage.text(content)?, + })?; @@ -6164,6 +6252,7 @@ + let start = self.input.pos(); let content = parse_cdata_content(&mut self.input)?; - let cdata_id = self.doc.create_node(NodeKind::CData { content }); ++ self.limit(LimitCounter::character_data)?; + let cdata_id = self.doc.try_create_node(NodeKind::CData { + content: self.storage.text(content)?, + })?; @@ -6184,6 +6273,7 @@ - let pi_id = self - .doc - .create_node(NodeKind::ProcessingInstruction { target, data }); ++ self.limit(LimitCounter::node)?; + let pi_id = self.doc.try_create_node(NodeKind::ProcessingInstruction { + target: self.storage.text(target)?, + data: self.storage.optional_text(data)?, @@ -6214,7 +6304,7 @@ } /// Strips an optional leading text declaration (``) from external -@@ -1471,10 +1453,10 @@ +@@ -1471,10 +1530,10 @@ let rest = s.strip_prefix(""); let root = doc.root_element().unwrap(); -@@ -1660,7 +1750,7 @@ +@@ -1660,7 +1827,7 @@ fn test_roundtrip_simple() { let input = "text"; let doc = parse(input); @@ -6363,7 +6453,7 @@ assert_eq!(output, format!("{DECL}{input}\n")); } -@@ -1668,7 +1758,7 @@ +@@ -1668,7 +1835,7 @@ fn test_roundtrip_attributes() { let input = ""; let doc = parse(input); @@ -6372,7 +6462,7 @@ assert_eq!(output, format!("{DECL}{input}\n")); } -@@ -1676,7 +1766,7 @@ +@@ -1676,7 +1843,7 @@ fn test_roundtrip_entities() { let input = "& < >"; let doc = parse(input); @@ -6381,7 +6471,7 @@ // After parsing, entities are resolved to characters. // Serialization re-escapes them. assert_eq!(output, format!("{DECL}& < >\n")); -@@ -1686,7 +1776,7 @@ +@@ -1686,7 +1853,7 @@ fn test_roundtrip_comment() { let input = ""; let doc = parse(input); @@ -6390,7 +6480,7 @@ assert_eq!(output, format!("{DECL}{input}\n")); } -@@ -1694,7 +1784,7 @@ +@@ -1694,7 +1861,7 @@ fn test_roundtrip_cdata() { let input = ""; let doc = parse(input); @@ -6399,7 +6489,7 @@ assert_eq!(output, format!("{DECL}{input}\n")); } -@@ -1702,7 +1792,7 @@ +@@ -1702,7 +1869,7 @@ fn test_roundtrip_pi() { let input = ""; let doc = parse(input); @@ -6408,7 +6498,7 @@ assert_eq!(output, format!("{DECL}{input}\n")); } -@@ -1710,7 +1800,7 @@ +@@ -1710,7 +1877,7 @@ fn test_roundtrip_xml_declaration() { let input = ""; let doc = parse(input); @@ -6417,7 +6507,7 @@ assert_eq!( output, "\n\n" -@@ -1898,13 +1988,13 @@ +@@ -1898,13 +2065,13 @@ // Simple DOCTYPE (no whitespace between DOCTYPE and root in input) let input = ""; let doc = parse(input); @@ -6433,7 +6523,7 @@ assert_eq!( output, format!("{DECL}\n") -@@ -1914,7 +2004,7 @@ +@@ -1914,7 +2081,7 @@ let input = ""; let doc = parse(input); @@ -6442,7 +6532,7 @@ assert_eq!( output, format!( -@@ -2061,7 +2151,7 @@ +@@ -2061,7 +2228,7 @@ let element = doc.first_child(entity_ref).unwrap(); assert_eq!(doc.node_name(element), Some("b")); // Serialization re-emits the entity reference, preserving roundtrip. @@ -6451,7 +6541,7 @@ assert!( out.contains("&e;"), "unexpected serialization: {out}" -@@ -2069,6 +2159,20 @@ +@@ -2069,6 +2236,20 @@ } #[test] @@ -6472,7 +6562,7 @@ fn test_parse_entity_charref_markup() { // "<b>hi</b>" expands to "hi" at declaration time // (§4.5) and must be parsed as markup at reference time. -@@ -2086,7 +2190,7 @@ +@@ -2086,7 +2267,7 @@ // inside the entity expansion. let input = "hi\">]>&e;"; let doc = parse(input); @@ -6481,7 +6571,7 @@ let doc2 = parse(&out); let root2 = doc2.root_element().unwrap(); assert_eq!(doc2.text_content(root2), "hi"); -@@ -2144,9 +2248,9 @@ +@@ -2144,9 +2325,9 @@ panic!("expected depth-limited parse to fail"); }; assert!( @@ -6493,7 +6583,7 @@ ); } -@@ -2169,9 +2273,10 @@ +@@ -2169,9 +2350,10 @@ panic!("expected amplification attack to be rejected"); }; assert!(