From b5076dd3f1de99d8496453dbeace637a840bca58 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 06:56:18 +0200 Subject: [PATCH 01/11] feat(x509): evaluate delta CRLs with their complete CRL CRL checking refused every delta as unpaired, even beside its complete CRL. Each authenticated delta now binds to the complete CRL of its issuer through the shared mechanism. The newest bound delta decides, by CRL number, and its window governs the pair: a current delta keeps an expired complete CRL usable, an expired one is refused. An applicable delta that binds to nothing still fails closed. OpenSSL-issued fixtures carry its own verifier's answer for both outcomes. Closes #346 --- docs/key-management.md | 3 +- docs/xmldsig.md | 12 +- scripts/generate-delta-crl-fixtures.sh | 88 ++++ src/xmldsig/pkix.rs | 427 ++++++++++++--- tests/delta_crl.rs | 579 +++++++++++++++++++++ tests/fixtures/x509/delta-crl/ca.der | Bin 0 -> 392 bytes tests/fixtures/x509/delta-crl/complete.crl | Bin 0 -> 219 bytes tests/fixtures/x509/delta-crl/leaf.der | Bin 0 -> 475 bytes tests/fixtures/x509/delta-crl/revoking.crl | Bin 0 -> 277 bytes tests/fixtures/x509/delta-crl/silent.crl | Bin 0 -> 234 bytes 10 files changed, 1029 insertions(+), 80 deletions(-) create mode 100755 scripts/generate-delta-crl-fixtures.sh create mode 100644 tests/delta_crl.rs create mode 100644 tests/fixtures/x509/delta-crl/ca.der create mode 100644 tests/fixtures/x509/delta-crl/complete.crl create mode 100644 tests/fixtures/x509/delta-crl/leaf.der create mode 100644 tests/fixtures/x509/delta-crl/revoking.crl create mode 100644 tests/fixtures/x509/delta-crl/silent.crl diff --git a/docs/key-management.md b/docs/key-management.md index 352addaa..2016a6d9 100644 --- a/docs/key-management.md +++ b/docs/key-management.md @@ -125,7 +125,8 @@ table from the host, network or a built-in default. Missing facts return `CrlTimeUncertain`, never non-revocation. Cached complete CRLs permit the exact `nextUpdate` boundary, but even one nanosecond later fails freshness. Revocation dates are checked against the same lossless operation time. Unsupported critical -demands, entry conflicts and unpaired delta CRLs cannot authorize a key. +demands, entry conflicts and delta CRLs without their complete CRL cannot +authorize a key; a bound complete-plus-delta pair is evaluated as one source. Applicable partial lists must collectively cover all revocation reasons; otherwise `IncompleteCrlCoverage` is returned. This checks supplied evidence, not an implicit online revocation fetch or a requirement to supply a CRL for diff --git a/docs/xmldsig.md b/docs/xmldsig.md index 04a62eac..4f042e2b 100644 --- a/docs/xmldsig.md +++ b/docs/xmldsig.md @@ -624,11 +624,17 @@ CRL checking is meaningful only inside authenticated X.509 path validation. A po CRLs without enabling certificate-chain validation is rejected during context construction rather than silently accepting a control the resolver cannot enforce. CRL structure is validated before authority-key applicability is selected: duplicate CRL or -CRL-entry extension OIDs fail closed, and `deltaCRLIndicator` is rejected regardless of criticality. +CRL-entry extension OIDs fail closed. Applicable CRLs must include the RFC 5280 `nextUpdate` field and the verification time must remain inside the bounded `thisUpdate` through `nextUpdate` validity window. -The `removeFromCRL` reason is rejected in complete CRLs because it is meaningful only in a delta -CRL. URI subject alternative names +A delta CRL is evaluated together with the complete CRL it binds to under RFC 5280 section 5.2.4: +same issuer, key and scope, a critical `deltaCRLIndicator`, and a complete CRL number from +`BaseCRLNumber` up to but excluding the delta's own. The newest bound delta decides, and its +validity window governs the pair, so a current delta keeps an expired complete CRL usable while an +expired delta is refused even over a current one. An applicable delta bound to no supplied complete +CRL fails closed. +The `removeFromCRL` reason lifts an entry only from a delta CRL and is rejected in complete CRLs. +URI subject alternative names likewise require one RFC 3986 authority, including syntactically valid userinfo, before their host can participate in `NameConstraints` matching. Critical `NameConstraints` also retain their DER structure during validation: the extension must diff --git a/scripts/generate-delta-crl-fixtures.sh b/scripts/generate-delta-crl-fixtures.sh new file mode 100755 index 00000000..6ab75fd5 --- /dev/null +++ b/scripts/generate-delta-crl-fixtures.sh @@ -0,0 +1,88 @@ +#!/usr/bin/env bash +# Regenerate the delta CRL fixtures of tests/delta_crl.rs. +# +# OpenSSL acts as an independent certificate authority: it issues one complete +# CRL and two delta CRLs against it, one silent and one revoking the leaf. Its +# own verifier checks both pairs here, so the fixtures carry its answer. It only +# reads a delta for a certificate that has freshestCRL (RFC 5280 4.2.1.15). +# +# Usage: scripts/generate-delta-crl-fixtures.sh +# DELTA_CRL_FIXTURE_OUTPUT optionally selects an absolute output directory. +# Requires OpenSSL 3.4 or later for fixed certificate validity dates. +set -euo pipefail + +repository="$(cd "$(dirname "$0")/.." && pwd)" +output="${DELTA_CRL_FIXTURE_OUTPUT:-$repository/tests/fixtures/x509/delta-crl}" +mkdir -p "$output" +work="$(mktemp -d)" +trap 'rm -rf "$work"' EXIT +cd "$work" + +NOT_BEFORE=20260101000000Z +NOT_AFTER=20450101000000Z +# 2040-01-01T00:00:00Z, after any revocation made while generating. +VERIFICATION_TIME=2208988800 + +cat >ca.cnf <<'EOF' +[ca] +default_ca = authority + +[authority] +database = index.txt +crlnumber = crlnumber +default_md = sha256 +crl_extensions = complete + +[complete] +authorityKeyIdentifier = keyid + +[delta] +authorityKeyIdentifier = keyid +2.5.29.27 = critical,ASN1:INTEGER:5 +EOF +: >index.txt +echo 05 >crlnumber + +openssl genpkey -algorithm EC -pkeyopt ec_paramgen_curve:P-256 -out ca.key 2>/dev/null +openssl req -x509 -new -key ca.key -out ca.pem \ + -not_before "$NOT_BEFORE" -not_after "$NOT_AFTER" \ + -subj "/CN=xml-sec Test Delta CA" \ + -addext "basicConstraints=critical,CA:TRUE" \ + -addext "keyUsage=critical,keyCertSign,cRLSign" 2>/dev/null +openssl genpkey -algorithm EC -pkeyopt ec_paramgen_curve:P-256 -out leaf.key 2>/dev/null +openssl req -x509 -new -key leaf.key -CA ca.pem -CAkey ca.key -out leaf.pem \ + -not_before "$NOT_BEFORE" -not_after "$NOT_AFTER" \ + -subj "/CN=xml-sec Test Delta Leaf" \ + -addext "basicConstraints=critical,CA:FALSE" \ + -addext "keyUsage=critical,digitalSignature" \ + -addext "freshestCRL=URI:https://example.test/delta.crl" 2>/dev/null + +# list : the next CRL of this authority. +list() { + openssl ca -gencrl -config ca.cnf -keyfile ca.key -cert ca.pem \ + -crlexts "$2" -crl_lastupdate "$NOT_BEFORE" -crl_nextupdate "$NOT_AFTER" \ + -out "$1" +} + +list complete.pem complete +list silent.pem delta +openssl ca -revoke leaf.pem -config ca.cnf -keyfile ca.key -cert ca.pem 2>/dev/null +list revoking.pem delta + +# accepted : the verifier's answer for the leaf under that pair. +accepted() { + openssl verify -CAfile ca.pem -CRLfile complete.pem -CRLfile "$1" \ + -crl_check -use_deltas -attime "$VERIFICATION_TIME" leaf.pem >/dev/null 2>&1 +} + +accepted silent.pem +if accepted revoking.pem; then + echo "OpenSSL accepted a leaf its delta CRL revokes" >&2 + exit 1 +fi + +openssl x509 -in ca.pem -outform DER -out "$output/ca.der" +openssl x509 -in leaf.pem -outform DER -out "$output/leaf.der" +for name in complete silent revoking; do + openssl crl -in "$name.pem" -outform DER -out "$output/$name.crl" +done diff --git a/src/xmldsig/pkix.rs b/src/xmldsig/pkix.rs index e46d9b9b..27f16f8e 100644 --- a/src/xmldsig/pkix.rs +++ b/src/xmldsig/pkix.rs @@ -1,6 +1,7 @@ //! Adapter-owned execution domain for the published ASN.1 and PKIX mechanisms. use alloc::{borrow::ToOwned, string::String, sync::Arc, vec::Vec}; +use core::cell::Cell; use structured_asn1::memory::{Memory, MemoryAccount, MemoryError, OwnedVec, Work}; use structured_asn1::{Encoding, Limits, Parser}; @@ -280,8 +281,6 @@ pub(crate) fn verify_crls<'a>( provider: &dyn crate::provider::CryptoProvider, session: &'a CertificateSession, ) -> Result<(), X509ChainError> { - use structured_pkix::crl::{Authentication, AuthenticationFailure}; - use structured_pkix::error::CallbackError; use structured_pkix::path::WorkingKey; if embedded.is_empty() && additional.is_empty() { @@ -305,54 +304,19 @@ pub(crate) fn verify_crls<'a>( let issuer = &path[position + 1]; let target = &path[position]; let mut reasons = structured_pkix::crl::Reasons::NONE; - for (index, crl) in crls.iter().enumerate() { - if !crl - .tbs() - .issuer() - .matches(issuer.tbs().subject(), execution) - .map_err(|error| session.error(error))? - { - continue; - } - let identified = match crl_key_identifier_match(crl, issuer, session)? { - KeyIdentifierMatch::Mismatch => continue, - KeyIdentifierMatch::Identified => true, - KeyIdentifierMatch::Unidentified => false, + let lists = authenticated_crls(&crls, issuer, issuer_key, position, &verifier, session)?; + for list in lists.iter().filter(|list| !list.delta) { + let evaluation = CrlEvaluation { + utc: options.utc_facts.as_ref(), + now, + index: list.index, + position, }; - let proof = crl - .authenticate(issuer, issuer_key, &verifier, execution) - .map_err(|error| match error { - CallbackError::Processing(error) => session.error(error), - CallbackError::Callback(error) => error, - })?; - let proof = match proof { - Authentication::Authenticated(proof) => proof, - Authentication::Rejected(AuthenticationFailure::KeyUsage) => { - return Err(X509ChainError::InvalidKeyUsage { - position: position + 1, - required: "cRLSign", - }); - } - Authentication::Rejected(AuthenticationFailure::Signature) if !identified => { - continue; - } - Authentication::Rejected(_) | Authentication::Unsupported => { - return Err(X509ChainError::InvalidCrl(index)); - } + let covered = match newest_delta(list, &lists, session)? { + Some((delta, pair)) => delta_reasons(pair, target, delta, evaluation, session)?, + None => crl_reasons(&list.proof, target, evaluation, session)?, }; - let Some(covered) = crl_reasons( - &proof, - crl, - target, - CrlEvaluation { - utc: options.utc_facts.as_ref(), - now, - index, - position, - }, - session, - )? - else { + let Some(covered) = covered else { continue; }; // RFC 5280 §6.3.3(d),(l): partial-list absence cannot establish @@ -361,6 +325,17 @@ pub(crate) fn verify_crls<'a>( // https://www.rfc-editor.org/rfc/rfc5280#section-6.3.3 reasons = reasons.union(covered); } + for list in lists.iter().filter(|list| list.delta && !list.bound.get()) { + // A delta alone lists changes, not status (RFC 5280 §5.2.4), and it + // may carry a revocation, so an applicable unbound one fails closed. + let scope = list + .proof + .scope(target, execution) + .map_err(|error| session.error(error))?; + if matches!(scope, structured_pkix::crl::ScopeOutcome::Applicable(_)) { + return Err(X509ChainError::InvalidCrl(list.index)); + } + } // The existing policy validates supplied applicable lists, rather than // requiring callers to provide a list for every path certificate. if !reasons.is_empty() && !reasons.is_complete() { @@ -373,6 +348,170 @@ pub(crate) fn verify_crls<'a>( Ok(()) } +/// One supplied list authenticated by the selected issuer key. +struct AuthenticatedCrl<'r, 'a> { + /// Original embedded/inventory source index. + index: usize, + proof: structured_pkix::crl::Authenticated<'r, 'a>, + delta: bool, + /// A delta bound to some complete list of this issuer. + bound: Cell, +} + +/// The supplied lists of `issuer` that its key authenticates. A same-name list +/// of another key is skipped; a list naming this key must verify. +fn authenticated_crls<'r, 'a>( + crls: &'r [structured_pkix::crl::CertificateList<'a>], + issuer: &'r Certificate<'a>, + issuer_key: structured_pkix::path::WorkingKey<'a>, + position: usize, + verifier: &ProviderVerifier<'_>, + session: &'r CertificateSession, +) -> Result>, X509ChainError> { + use structured_pkix::crl::{Authentication, AuthenticationFailure}; + use structured_pkix::error::CallbackError; + let execution = session.memory(); + let mut lists = execution.vector(); + for (index, crl) in crls.iter().enumerate() { + if !crl + .tbs() + .issuer() + .matches(issuer.tbs().subject(), execution) + .map_err(|error| session.error(error))? + { + continue; + } + let identified = match crl_key_identifier_match(crl, issuer, session)? { + KeyIdentifierMatch::Mismatch => continue, + KeyIdentifierMatch::Identified => true, + KeyIdentifierMatch::Unidentified => false, + }; + let proof = crl + .authenticate(issuer, issuer_key, verifier, execution) + .map_err(|error| match error { + CallbackError::Processing(error) => session.error(error), + CallbackError::Callback(error) => error, + })?; + let proof = match proof { + Authentication::Authenticated(proof) => proof, + Authentication::Rejected(AuthenticationFailure::KeyUsage) => { + return Err(X509ChainError::InvalidKeyUsage { + position: position + 1, + required: "cRLSign", + }); + } + Authentication::Rejected(AuthenticationFailure::Signature) if !identified => { + continue; + } + Authentication::Rejected(_) | Authentication::Unsupported => { + return Err(X509ChainError::InvalidCrl(index)); + } + }; + validate_crl_serials(crl, session)?; + let delta = crl + .tbs() + .extensions() + .get(structured_pkix::oid::DELTA_CRL_INDICATOR, execution) + .map_err(|error| session.error(error))? + .is_some(); + lists + .try_push(AuthenticatedCrl { + index, + proof, + delta, + bound: Cell::new(false), + }) + .map_err(|reason| { + session.error(structured_pkix::Error::resource( + reason, + "certificateList.pool", + crl.span().start, + )) + })?; + } + Ok(lists.into_owned()) +} + +/// The newest delta bound to `base`, with its source index. Deltas accumulate +/// from their base, so an older one is superseded and takes no part. +fn newest_delta<'b, 'r, 'a>( + base: &'b AuthenticatedCrl<'r, 'a>, + lists: &'b [AuthenticatedCrl<'r, 'a>], + session: &CertificateSession, +) -> Result)>, X509ChainError> { + use structured_pkix::crl::DeltaBinding; + let mut newest = None; + let mut newest_number = &[][..]; + for candidate in lists.iter().filter(|list| list.delta) { + let binding = base + .proof + .bind_delta(&candidate.proof, session.memory()) + .map_err(|error| session.error(error))?; + let DeltaBinding::Bound(pair) = binding else { + continue; + }; + candidate.bound.set(true); + let number = crl_number(candidate, session)?; + if newest.is_none() || crl_number_after(number, newest_number) { + newest = Some((candidate.index, pair)); + newest_number = number; + } + } + Ok(newest) +} + +/// Original `cRLNumber` content octets of a list `bind_delta` has validated. +fn crl_number<'a>( + list: &AuthenticatedCrl<'_, 'a>, + session: &CertificateSession, +) -> Result<&'a [u8], X509ChainError> { + let execution = session.memory(); + let crl = list.proof.crl(); + let number = crl + .tbs() + .extensions() + .get(structured_pkix::oid::CRL_NUMBER, execution) + .map_err(|error| session.error(error))? + .ok_or(X509ChainError::InvalidCrl(list.index))?; + let integer = number + .reader_scoped(execution) + .and_then(|mut reader| reader.read_integer()) + .map_err(|error| { + session.error(structured_pkix::Error::new( + structured_pkix::ErrorCode::Encoding(error.code()), + "extensions.cRLNumber", + error.offset(), + )) + })?; + execution + .charge_work(integer.as_bytes().len() as u64 + 1) + .map_err(|reason| { + session.error(structured_pkix::Error::resource( + reason, + "extensions.cRLNumber", + number.span().start, + )) + })?; + Ok(integer.as_bytes()) +} + +/// RFC 5280 §5.2.3: CRL numbers increase monotonically, so the larger integer +/// is the newer list. `bind_delta` admitted both as nonnegative minimal DER. +/// +fn crl_number_after(left: &[u8], right: &[u8]) -> bool { + fn magnitude(bytes: &[u8]) -> &[u8] { + match bytes { + [0, rest @ ..] => rest, + all => all, + } + } + let (left, right) = (magnitude(left), magnitude(right)); + left.len() + .cmp(&right.len()) + .then_with(|| left.cmp(right)) + .is_gt() +} + /// How a CRL's authority key identifier relates to a candidate issuer. enum KeyIdentifierMatch { /// Both carry key identifiers and they are equal. @@ -431,29 +570,39 @@ struct CrlEvaluation<'u> { position: usize, } -/// Reason coverage an authenticated CRL contributes for `target`, or `None` -/// when it does not apply. Revocation and unusable lists are errors. +/// The caller's UTC history, a fact and no implicit clock/table default: `None` +/// when the list does not apply anyway, so that absence costs nothing. +fn required_utc<'u, 'a>( + proof: &structured_pkix::crl::Authenticated<'_, 'a>, + target: &'a Certificate<'a>, + evaluation: CrlEvaluation<'u>, + session: &CertificateSession, +) -> Result>, X509ChainError> { + use structured_pkix::crl::ScopeOutcome; + if evaluation.utc.is_some() { + return Ok(evaluation.utc); + } + match proof + .scope(target, session.memory()) + .map_err(|error| session.error(error))? + { + ScopeOutcome::NotApplicable(_) => Ok(None), + ScopeOutcome::Applicable(_) => Err(X509ChainError::MissingUtcFacts), + } +} + +/// Reason coverage an authenticated complete CRL contributes for `target`, or +/// `None` when it does not apply. Revocation and unusable lists are errors. fn crl_reasons<'a>( proof: &structured_pkix::crl::Authenticated<'_, 'a>, - crl: &structured_pkix::crl::CertificateList<'_>, target: &'a Certificate<'a>, evaluation: CrlEvaluation<'_>, session: &CertificateSession, ) -> Result, X509ChainError> { - use structured_pkix::crl::{CompleteEvidence, Evaluation, ScopeOutcome, SourceOrigin}; + use structured_pkix::crl::{CompleteEvidence, Evaluation, SourceOrigin}; let execution = session.memory(); - validate_crl_serials(crl, session)?; - // UTC history is a caller fact, not an implicit clock/table default. - let Some(utc) = evaluation.utc else { - if matches!( - proof - .scope(target, execution) - .map_err(|error| session.error(error))?, - ScopeOutcome::NotApplicable(_) - ) { - return Ok(None); - } - return Err(X509ChainError::MissingUtcFacts); + let Some(utc) = required_utc(proof, target, evaluation, session)? else { + return Ok(None); }; let evidence = proof .evaluate( @@ -485,12 +634,143 @@ fn crl_reasons<'a>( session.error(structured_pkix::Error::resource( reason, "certificateList.reasons", - crl.span().start, + proof.crl().span().start, )) })?; Ok(Some(facts.reasons())) } +/// Reason coverage a bound complete-plus-delta pair contributes for `target`, +/// or `None` when it does not apply. `evaluation.index` names the complete list +/// and `delta` the updating one. +fn delta_reasons<'a>( + pair: structured_pkix::crl::BoundDelta<'_, '_, 'a>, + target: &'a Certificate<'a>, + delta: usize, + evaluation: CrlEvaluation<'_>, + session: &CertificateSession, +) -> Result, X509ChainError> { + use structured_pkix::crl::{DeltaEvidence, Evaluation, ScopeOutcome, SourceOrigin}; + let execution = session.memory(); + let Some(utc) = required_utc(pair.base(), target, evaluation, session)? else { + return Ok(None); + }; + let evidence = pair + .evaluate( + target, + &Evaluation { + now: evaluation.now, + utc, + execution, + origin: SourceOrigin::Cached, + }, + ) + .map_err(|error| session.error(error))?; + let facts = match evidence { + DeltaEvidence::Facts(facts) => facts, + DeltaEvidence::NotApplicable(_) => return Ok(None), + }; + validate_delta_window(&facts, delta, evaluation.index)?; + validate_delta_entry(&facts, delta, utc, evaluation, session)?; + // The pair shares the base's scope; its facts expose reasons only through + // absence, which a not yet effective entry does not establish. + match pair + .base() + .scope(target, execution) + .map_err(|error| session.error(error))? + { + ScopeOutcome::Applicable(reasons) => Ok(Some(reasons)), + ScopeOutcome::NotApplicable(_) => Ok(None), + } +} + +/// RFC 5280 §5.2.4: the delta's window governs the combined list. An expired +/// complete list stays usable under it, one not yet valid does not. +/// +fn validate_delta_window( + facts: &structured_pkix::crl::DeltaFacts<'_, '_, '_>, + delta: usize, + base: usize, +) -> Result<(), X509ChainError> { + use structured_pkix::crl::Freshness; + match facts.delta_freshness() { + Freshness::Current | Freshness::AtNextUpdate => (), + Freshness::Uncertain(reason) => { + return Err(X509ChainError::CrlTimeUncertain { + index: delta, + reason, + }); + } + _ => return Err(X509ChainError::InvalidCrl(delta)), + } + match facts.base_freshness() { + Freshness::Uncertain(reason) => Err(X509ChainError::CrlTimeUncertain { + index: base, + reason, + }), + Freshness::NotYetCurrent | Freshness::InvalidUpdateOrder => { + Err(X509ChainError::InvalidCrl(base)) + } + _ => Ok(()), + } +} + +fn validate_delta_entry( + facts: &structured_pkix::crl::DeltaFacts<'_, '_, '_>, + delta: usize, + utc: &structured_pkix::temporal::LeapTable<'_>, + evaluation: CrlEvaluation<'_>, + session: &CertificateSession, +) -> Result<(), X509ChainError> { + use structured_pkix::crl::{CombinedEntry, EntryOrigin}; + let source = |origin| match origin { + EntryOrigin::Complete => evaluation.index, + EntryOrigin::Delta => delta, + }; + match *facts.entry() { + CombinedEntry::Absent | CombinedEntry::Removed { .. } => Ok(()), + CombinedEntry::Revoked { origin, entry, .. } => revocation_in_effect( + entry, + source(origin), + RevocationCheck { + now: evaluation.now, + utc, + position: evaluation.position, + }, + session, + ), + CombinedEntry::Conflict { origin, .. } | CombinedEntry::Unsupported { origin, .. } => { + Err(X509ChainError::InvalidCrl(source(origin))) + } + } +} + +/// The instant and path member a found revocation entry is checked against. +#[derive(Clone, Copy)] +struct RevocationCheck<'u> { + now: structured_pkix::temporal::Reading, + utc: &'u structured_pkix::temporal::LeapTable<'u>, + position: usize, +} + +/// An entry revokes from its date on; a later date is not yet in effect. +fn revocation_in_effect( + entry: &structured_pkix::crl::RevokedCertificate<'_>, + index: usize, + check: RevocationCheck<'_>, + session: &CertificateSession, +) -> Result<(), X509ChainError> { + use structured_pkix::time::Relation; + match entry + .time_relation(check.now, check.utc, session.memory()) + .map_err(|error| session.error(error))? + { + Relation::Before | Relation::At => Err(X509ChainError::Revoked(check.position)), + Relation::After => Ok(()), + Relation::Uncertain(reason) => Err(X509ChainError::CrlTimeUncertain { index, reason }), + } +} + fn read_crl_pool<'a>( embedded: &'a [Vec], additional: &'a [Vec], @@ -525,7 +805,6 @@ fn validate_complete_crl( position: usize, ) -> Result<(), X509ChainError> { use structured_pkix::crl::{EntryLookup, Freshness}; - use structured_pkix::time::Relation; match facts.freshness() { Freshness::Current | Freshness::AtNextUpdate => (), Freshness::Uncertain(reason) => { @@ -536,16 +815,12 @@ fn validate_complete_crl( match facts.entry() { EntryLookup::Absent => (), EntryLookup::Found(entry) => { - match entry - .time_relation(now, utc, session.memory()) - .map_err(|error| session.error(error))? - { - Relation::Before | Relation::At => return Err(X509ChainError::Revoked(position)), - Relation::After => (), - Relation::Uncertain(reason) => { - return Err(X509ChainError::CrlTimeUncertain { index, reason }); - } - } + revocation_in_effect( + entry, + index, + RevocationCheck { now, utc, position }, + session, + )?; } EntryLookup::Conflict { .. } | EntryLookup::Unsupported { .. } => { return Err(X509ChainError::InvalidCrl(index)); diff --git a/tests/delta_crl.rs b/tests/delta_crl.rs new file mode 100644 index 00000000..043f5098 --- /dev/null +++ b/tests/delta_crl.rs @@ -0,0 +1,579 @@ +#![cfg(feature = "xmldsig")] +//! RFC 5280 5.2.4 and 6.3.3 complete-plus-delta evaluation through the public +//! chain API. Every list is really signed by the path's issuing key. + +use der::{Decode as _, Encode as _, asn1::BitString}; +use rcgen::SigningKey as _; +use rcgen::{ + BasicConstraints, CertificateParams, CertificateRevocationListParams, CertifiedIssuer, IsCa, + KeyIdMethod, KeyPair, KeyUsagePurpose, RevocationReason, RevokedCertParams, SerialNumber, + date_time_ymd, +}; +use time::OffsetDateTime; +use x509_cert::ext::pkix::crl::dp::{IssuingDistributionPoint, Reasons}; +use xml_sec::policy::Instant; +use xml_sec::xmldsig::{ + X509ChainError, X509ChainOptions, X509DataInfo, verify_x509_certificate_chain, +}; + +const VERIFICATION_TIME: i64 = 1_773_964_800; // 2026-03-20T00:00:00Z +const LEAF_SERIAL: u64 = 0x51; +const INTERMEDIATE_SERIAL: u64 = 0x52; +const DELTA_CRL_INDICATOR: &str = "2.5.29.27"; +const ISSUING_DISTRIBUTION_POINT: &str = "2.5.29.28"; + +type Issuer = CertifiedIssuer<'static, KeyPair>; + +fn current() -> (OffsetDateTime, OffsetDateTime) { + (date_time_ymd(2026, 3, 15), date_time_ymd(2026, 4, 15)) +} + +fn expired() -> (OffsetDateTime, OffsetDateTime) { + (date_time_ymd(2026, 2, 1), date_time_ymd(2026, 3, 1)) +} + +fn root_named(name: &str) -> Issuer { + let mut params = CertificateParams::new(Vec::new()).unwrap(); + params + .distinguished_name + .push(rcgen::DnType::CommonName, name); + params.is_ca = IsCa::Ca(BasicConstraints::Unconstrained); + params.key_usages = vec![KeyUsagePurpose::KeyCertSign, KeyUsagePurpose::CrlSign]; + params.use_authority_key_identifier_extension = true; + CertifiedIssuer::self_signed(params, KeyPair::generate().unwrap()).unwrap() +} + +/// Leaf directly under `issuer`, with a serial the lists below can name. +fn leaf_of(issuer: &Issuer) -> Vec { + let mut params = CertificateParams::new(Vec::new()).unwrap(); + params.serial_number = Some(SerialNumber::from(LEAF_SERIAL)); + params.use_authority_key_identifier_extension = true; + params + .signed_by(&KeyPair::generate().unwrap(), issuer) + .unwrap() + .der() + .to_vec() +} + +fn entry(serial: u64, reason: Option) -> RevokedCertParams { + RevokedCertParams { + serial_number: SerialNumber::from(serial), + revocation_time: date_time_ymd(2026, 3, 10), + reason_code: reason, + invalidity_date: None, + } +} + +/// One list as rcgen writes it: crlNumber and authorityKeyIdentifier included. +fn list( + issuer: &Issuer, + number: u64, + (this_update, next_update): (OffsetDateTime, OffsetDateTime), + revoked_certs: Vec, +) -> Vec { + CertificateRevocationListParams { + this_update, + next_update, + crl_number: SerialNumber::from(number), + issuing_distribution_point: None, + revoked_certs, + key_identifier_method: KeyIdMethod::Sha256, + } + .signed_by(issuer) + .unwrap() + .der() + .to_vec() +} + +/// Add extensions to a signed list and sign the result again. +fn extended( + issuer: &Issuer, + list: &[u8], + extensions: impl IntoIterator, +) -> Vec { + let mut encoded: x509_cert::crl::CertificateList = + x509_cert::crl::CertificateList::from_der(list).unwrap(); + encoded + .tbs_cert_list + .crl_extensions + .as_mut() + .unwrap() + .extend(extensions); + let signature = issuer + .key() + .sign(&encoded.tbs_cert_list.to_der().unwrap()) + .unwrap(); + encoded.signature = BitString::from_bytes(&signature).unwrap(); + encoded.to_der().unwrap() +} + +fn extension(oid: &str, critical: bool, value: Vec) -> x509_cert::ext::Extension { + x509_cert::ext::Extension { + extn_id: der::asn1::ObjectIdentifier::new_unwrap(oid), + critical, + extn_value: der::asn1::OctetString::new(value).unwrap(), + } +} + +/// The `BaseCRLNumber` value of `deltaCRLIndicator`, as a DER INTEGER. +fn base_number(base: u8) -> Vec { + assert!(base < 0x80, "one positive content octet"); + vec![0x02, 0x01, base] +} + +fn indicator(base: u8) -> x509_cert::ext::Extension { + extension(DELTA_CRL_INDICATOR, true, base_number(base)) +} + +fn delta( + issuer: &Issuer, + number: u64, + base: u8, + window: (OffsetDateTime, OffsetDateTime), + revoked: Vec, +) -> Vec { + extended( + issuer, + &list(issuer, number, window, revoked), + [indicator(base)], + ) +} + +fn scope(point: &IssuingDistributionPoint) -> x509_cert::ext::Extension { + extension( + ISSUING_DISTRIBUTION_POINT, + true, + point.to_der().unwrap().clone(), + ) +} + +fn reasons_scope(reason: Reasons) -> x509_cert::ext::Extension { + scope(&IssuingDistributionPoint { + distribution_point: None, + only_contains_user_certs: false, + only_contains_ca_certs: false, + only_some_reasons: Some(reason.into()), + indirect_crl: false, + only_contains_attribute_certs: false, + }) +} + +fn user_scope(ca_only: bool) -> x509_cert::ext::Extension { + scope(&IssuingDistributionPoint { + distribution_point: None, + only_contains_user_certs: !ca_only, + only_contains_ca_certs: ca_only, + only_some_reasons: None, + indirect_crl: false, + only_contains_attribute_certs: false, + }) +} + +fn chain(certificates: Vec>, crls: Vec>) -> X509DataInfo { + let mut info = X509DataInfo::default(); + info.certificate_chain = (0..certificates.len()).collect(); + info.certificates = certificates; + info.crls = crls; + info +} + +fn verify(info: &X509DataInfo, root: &Issuer) -> Result<(), X509ChainError> { + verify_with_utc(info, root, true) +} + +fn verify_with_utc(info: &X509DataInfo, root: &Issuer, utc: bool) -> Result<(), X509ChainError> { + let anchors = [root.der().to_vec()]; + verify_x509_certificate_chain( + info, + &X509ChainOptions { + // Explicit synthetic UTC coverage belongs to this test request. + utc_facts: utc.then(|| { + structured_pkix::temporal::LeapTable::new(-2_208_988_800, 4_102_444_800, &[]) + .expect("test UTC facts") + }), + trusted_certs: &anchors, + verification_time: Instant::new(VERIFICATION_TIME, 0).unwrap(), + max_chain_depth: 3, + check_crls: true, + allowed_extended_key_usages: None, + rsa_keys: xml_sec::policy::RsaKeyPolicy::default(), + dsa_keys: xml_sec::policy::DsaKeyPolicy::default(), + certificate_signature_algorithms: None, + }, + ) +} + +fn leaf_chain(root: &Issuer, crls: Vec>) -> X509DataInfo { + chain(vec![leaf_of(root), root.der().to_vec()], crls) +} + +#[test] +fn delta_with_its_complete_list_accepts_an_unlisted_certificate() { + // The pair must establish absence, not fail as an unpaired delta. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = delta(&root, 4, 3, current(), Vec::new()); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Ok(()) + ); +} + +#[test] +fn revocation_listed_only_in_the_delta_rejects_the_certificate() { + // The update, not the older complete list, carries this revocation. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = delta(&root, 4, 3, current(), vec![entry(LEAF_SERIAL, None)]); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Err(X509ChainError::Revoked(0)) + ); +} + +#[test] +fn revocation_in_the_complete_list_survives_a_delta_that_omits_it() { + // A delta lists changes only; silence does not lift the base entry. + let root = root_named("delta root"); + let base = list(&root, 3, current(), vec![entry(LEAF_SERIAL, None)]); + let update = delta(&root, 4, 3, current(), Vec::new()); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Err(X509ChainError::Revoked(0)) + ); +} + +#[test] +fn remove_from_crl_in_the_delta_lifts_a_hold_in_the_complete_list() { + // RFC 5280 5.3.1: removeFromCRL appears only in delta CRLs. + let root = root_named("delta root"); + let hold = Some(RevocationReason::CertificateHold); + let base = list(&root, 3, current(), vec![entry(LEAF_SERIAL, hold)]); + let removal = Some(RevocationReason::RemoveFromCrl); + let update = delta(&root, 4, 3, current(), vec![entry(LEAF_SERIAL, removal)]); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Ok(()) + ); +} + +#[test] +fn current_delta_keeps_an_expired_complete_list_usable() { + // RFC 5280 5.2.4: the delta's update window governs the combined list. + let root = root_named("delta root"); + let base = list(&root, 3, expired(), Vec::new()); + let update = delta(&root, 4, 3, current(), Vec::new()); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Ok(()) + ); +} + +#[test] +fn expired_delta_is_refused_even_over_a_current_complete_list() { + // The delta may carry a revocation the base lacks, so it cannot be skipped. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = delta(&root, 4, 3, expired(), Vec::new()); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Err(X509ChainError::InvalidCrl(1)) + ); +} + +#[test] +fn delta_without_a_complete_list_is_refused() { + let root = root_named("delta root"); + let update = delta(&root, 4, 3, current(), Vec::new()); + assert_eq!( + verify(&leaf_chain(&root, vec![update]), &root), + Err(X509ChainError::InvalidCrl(0)) + ); +} + +#[test] +fn delta_that_cannot_bind_its_complete_list_is_refused() { + // RFC 5280 5.2.4: baseCRLNumber <= complete number < delta number. + let root = root_named("delta root"); + let cases = [ + ("complete list older than the delta's base", 3, 5, 6), + ("delta not newer than the complete list", 4, 3, 4), + ]; + for (case, complete, base, number) in cases { + let info = leaf_chain( + &root, + vec![ + list(&root, complete, current(), Vec::new()), + delta(&root, number, base, current(), Vec::new()), + ], + ); + assert_eq!( + verify(&info, &root), + Err(X509ChainError::InvalidCrl(1)), + "{case}" + ); + } +} + +#[test] +fn delta_indicator_must_be_critical_to_bind() { + // RFC 5280 5.2.4 requires criticality; a noncritical marker binds nothing. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = extended( + &root, + &list(&root, 4, current(), Vec::new()), + [extension(DELTA_CRL_INDICATOR, false, base_number(3))], + ); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Err(X509ChainError::InvalidCrl(1)) + ); +} + +#[test] +fn delta_of_another_scope_does_not_update_the_complete_list() { + // RFC 5280 6.3.3(c): the pair needs the same distribution scope. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = extended( + &root, + &list(&root, 4, current(), Vec::new()), + [indicator(3), user_scope(false)], + ); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Err(X509ChainError::InvalidCrl(1)) + ); +} + +#[test] +fn unbound_delta_outside_the_target_scope_is_ignored() { + // A list covering only CA certificates says nothing about this leaf. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = extended( + &root, + &list(&root, 4, current(), Vec::new()), + [indicator(3), user_scope(true)], + ); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Ok(()) + ); +} + +#[test] +fn delta_signed_by_a_rolled_over_key_is_ignored_for_the_selected_issuer() { + // Equal names are not key identities: its authority key identifier selects + // another issuer, so this list is no evidence about the selected path. + let root = root_named("delta root"); + let previous = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = delta(&previous, 4, 3, current(), vec![entry(LEAF_SERIAL, None)]); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Ok(()) + ); +} + +#[test] +fn newest_delta_supersedes_an_older_one_in_either_order() { + // Deltas accumulate from their base, so the older hold is already resolved + // by the newer removal. Supply order must not change the answer. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let hold = Some(RevocationReason::CertificateHold); + let removal = Some(RevocationReason::RemoveFromCrl); + let older = delta(&root, 4, 3, expired(), vec![entry(LEAF_SERIAL, hold)]); + let newer = delta(&root, 5, 3, current(), vec![entry(LEAF_SERIAL, removal)]); + for crls in [ + vec![base.clone(), older.clone(), newer.clone()], + vec![newer, older, base], + ] { + assert_eq!(verify(&leaf_chain(&root, crls), &root), Ok(())); + } +} + +#[test] +fn newest_delta_decides_by_number_beyond_one_octet() { + // CRL numbers are integers: 0x0100 is newer than 0xFF though its first + // content octet is smaller. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let older = delta(&root, 0xFF, 3, current(), vec![entry(LEAF_SERIAL, None)]); + let newer = delta(&root, 0x0100, 3, current(), Vec::new()); + assert_eq!( + verify(&leaf_chain(&root, vec![base, newer, older]), &root), + Ok(()) + ); +} + +#[test] +fn pair_coverage_is_limited_to_the_reasons_of_its_scope() { + // RFC 5280 6.3.3(d): a bound pair qualifies only the base's reasons. + let root = root_named("delta root"); + let reason = reasons_scope(Reasons::KeyCompromise); + let base = extended( + &root, + &list(&root, 3, current(), Vec::new()), + [reason.clone()], + ); + let update = extended( + &root, + &list(&root, 4, current(), Vec::new()), + [indicator(3), reason], + ); + assert_eq!( + verify(&leaf_chain(&root, vec![base, update]), &root), + Err(X509ChainError::IncompleteCrlCoverage(0)) + ); +} + +#[test] +fn delta_evaluation_requires_caller_utc_facts() { + // Without UTC history no absence can be established for an applicable pair. + let root = root_named("delta root"); + let base = list(&root, 3, current(), Vec::new()); + let update = delta(&root, 4, 3, current(), Vec::new()); + assert_eq!( + verify_with_utc(&leaf_chain(&root, vec![base, update]), &root, false), + Err(X509ChainError::MissingUtcFacts) + ); +} + +#[test] +fn exhausted_work_during_delta_evaluation_stays_a_resource_refusal() { + // Spent budget must not read as a bad list or as an established status. + let root = root_named("delta root"); + let info = leaf_chain( + &root, + vec![ + list(&root, 3, current(), Vec::new()), + delta(&root, 4, 3, current(), Vec::new()), + delta(&root, 5, 3, current(), Vec::new()), + ], + ); + let anchors = [root.der().to_vec()]; + let options = X509ChainOptions { + utc_facts: Some( + structured_pkix::temporal::LeapTable::new(-2_208_988_800, 4_102_444_800, &[]) + .expect("test UTC facts"), + ), + trusted_certs: &anchors, + verification_time: Instant::new(VERIFICATION_TIME, 0).unwrap(), + max_chain_depth: 3, + check_crls: true, + allowed_extended_key_usages: None, + rsa_keys: xml_sec::policy::RsaKeyPolicy::default(), + dsa_keys: xml_sec::policy::DsaKeyPolicy::default(), + certificate_signature_algorithms: None, + }; + let mut completed = false; + for max_x509_work in (1..200_000).step_by(64) { + let resources = xml_sec::policy::ResourcePolicy { + max_x509_work, + ..Default::default() + }; + match xml_sec::xmldsig::verify_x509_certificate_chain_with_resources( + &info, + &options, + &resources, + xml_sec::provider::default_provider(), + ) { + Ok(()) => { + completed = true; + break; + } + Err(X509ChainError::Policy(_)) => (), + Err(X509ChainError::Processing(error)) if error.is_resource() => (), + Err(other) => panic!("work {max_x509_work}: {other:?}"), + } + } + assert!(completed, "the sweep must reach a sufficient budget"); +} + +/// Independent issuer: `scripts/generate-delta-crl-fixtures.sh`, whose OpenSSL +/// verifier accepted the silent pair and refused the revoking one. +fn openssl_pair(delta: &[u8]) -> Result<(), X509ChainError> { + let root = include_bytes!("fixtures/x509/delta-crl/ca.der").to_vec(); + let info = chain( + vec![ + include_bytes!("fixtures/x509/delta-crl/leaf.der").to_vec(), + root.clone(), + ], + vec![ + include_bytes!("fixtures/x509/delta-crl/complete.crl").to_vec(), + delta.to_vec(), + ], + ); + let anchors = [root]; + verify_x509_certificate_chain( + &info, + &X509ChainOptions { + utc_facts: Some( + structured_pkix::temporal::LeapTable::new(-2_208_988_800, 4_102_444_800, &[]) + .expect("test UTC facts"), + ), + trusted_certs: &anchors, + // 2040-01-01T00:00:00Z, after the fixtures' revocation date. + verification_time: Instant::new(2_208_988_800, 0).unwrap(), + max_chain_depth: 3, + check_crls: true, + allowed_extended_key_usages: None, + rsa_keys: xml_sec::policy::RsaKeyPolicy::default(), + dsa_keys: xml_sec::policy::DsaKeyPolicy::default(), + certificate_signature_algorithms: None, + }, + ) +} + +#[test] +fn openssl_issued_silent_delta_is_accepted_as_openssl_accepts_it() { + assert_eq!( + openssl_pair(include_bytes!("fixtures/x509/delta-crl/silent.crl")), + Ok(()) + ); +} + +#[test] +fn openssl_issued_revoking_delta_is_refused_as_openssl_refuses_it() { + assert_eq!( + openssl_pair(include_bytes!("fixtures/x509/delta-crl/revoking.crl")), + Err(X509ChainError::Revoked(0)) + ); +} + +#[test] +fn delta_applies_at_its_own_path_position() { + // The root's delta revokes the intermediate, not the leaf below it. + let root = root_named("delta root"); + let mut params = CertificateParams::new(Vec::new()).unwrap(); + params + .distinguished_name + .push(rcgen::DnType::CommonName, "delta intermediate"); + params.serial_number = Some(SerialNumber::from(INTERMEDIATE_SERIAL)); + params.is_ca = IsCa::Ca(BasicConstraints::Unconstrained); + params.key_usages = vec![KeyUsagePurpose::KeyCertSign, KeyUsagePurpose::CrlSign]; + params.use_authority_key_identifier_extension = true; + let intermediate: Issuer = + CertifiedIssuer::signed_by(params, KeyPair::generate().unwrap(), &root).unwrap(); + let info = chain( + vec![ + leaf_of(&intermediate), + intermediate.der().to_vec(), + root.der().to_vec(), + ], + vec![ + list(&root, 3, current(), Vec::new()), + delta( + &root, + 4, + 3, + current(), + vec![entry(INTERMEDIATE_SERIAL, None)], + ), + ], + ); + assert_eq!(verify(&info, &root), Err(X509ChainError::Revoked(1))); +} diff --git a/tests/fixtures/x509/delta-crl/ca.der b/tests/fixtures/x509/delta-crl/ca.der new file mode 100644 index 0000000000000000000000000000000000000000..71178d6b57d4c8c4a104f3c0f81c2fcb21f0230a GIT binary patch literal 392 zcmXqLVr(&JV$@o|%*4pVBx08Ojj1A7yzIFC$7*ri2j|&&en%Q`v2kd%d7QIlVP-N= zFqAWpVPg(uVdfF7$j#9$PEA$_Ni8l>a7oQ6NmOulG>{YLH8L|W1VS)~66ZBBMdA`+ zLZpE(8#~xWCPt{$%#7^JP7Expo7PrZd{>Tmugscy!+ibLJC~p9_3fOslOgiL39bUy z@>r$*)5n$nRXms_`8_6Nt&PAF`%t?lnQ`A+c(ta_JP^Ft$-u!t7U+CgJ{B<+5tC=@ zg1i|RWj-amyUzUV-B&%$eXIujAZcMn#{Vp=2FyUpfDa_V4-xr?7`QSi zGJNImEi7NDu>AGorJ`xgo1HBff7JbazxJWfg-1@|oLa}36#UD3Y;QB(nsRSed+O`I XZEv!^c>A4`Ubkd-tfsdYZ>0tR%=v!J literal 0 HcmV?d00001 diff --git a/tests/fixtures/x509/delta-crl/complete.crl b/tests/fixtures/x509/delta-crl/complete.crl new file mode 100644 index 0000000000000000000000000000000000000000..177832d9422021cd5fcb94809450533a7a6d2a26 GIT binary patch literal 219 zcmXqLykXGTz{JRCz{SR))#h=|mW7$gK*3PXK!%Mul!ci`v?4c0w>ULfAtbf9M8PFB zrzBCq*-@O=$jrbH2*DsqoY%w@iMv4GK-WMXs9#o@MZ!R=LB!pmmHOHr#S%cLoC&CIwBKiKhb>dAygt{;9NVL8NN&QP)CuMJWb} wmrB#bqFLTEDWsftxV7n|W%U+nkVGjzkJ#sbE>8&@a-01K2tqyPW_ literal 0 HcmV?d00001 diff --git a/tests/fixtures/x509/delta-crl/leaf.der b/tests/fixtures/x509/delta-crl/leaf.der new file mode 100644 index 0000000000000000000000000000000000000000..1ca79b7b462f64f7575f3f162cd34472f6ab60e6 GIT binary patch literal 475 zcmXqLV!Uq9#8|t4nTe5!NyH(z)sR=QhVj;J9a7oQ6NmOulG>{YLH8L|W1VS)~66ZBBMdBJL z87dgaK}-ixBAwT9gJ za;6#Ag!ieus=hY8K>2KwTf(t_H*P%tx_6QC-H2t8Z?4Ys?XS%CeB+;dHKy)H(c;EQ z295m&vOovO^0A1qh*aLwe7*5_2T3clNEnDUh?qQE7v#;r zDDx@d-F4Wc<&2AO^eD9H4Zi*k@dnAwBDz?DfM@`wLL*3^Ze>-pMt zUlYG0eULfAtbf9M8PFB zrzBCq*-@O=$jrbH2*DsqoY%w@iMzngz{Wrxs9#o@MZ!R=LB! Date: Sat, 31 Oct 2026 07:05:56 +0200 Subject: [PATCH 02/11] perf(x509): read delta CRL numbers only to compare two deltas --- src/xmldsig/pkix.rs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/xmldsig/pkix.rs b/src/xmldsig/pkix.rs index 27f16f8e..035ec5ad 100644 --- a/src/xmldsig/pkix.rs +++ b/src/xmldsig/pkix.rs @@ -440,8 +440,7 @@ fn newest_delta<'b, 'r, 'a>( session: &CertificateSession, ) -> Result)>, X509ChainError> { use structured_pkix::crl::DeltaBinding; - let mut newest = None; - let mut newest_number = &[][..]; + let mut newest: Option<(&AuthenticatedCrl<'r, 'a>, _)> = None; for candidate in lists.iter().filter(|list| list.delta) { let binding = base .proof @@ -451,13 +450,19 @@ fn newest_delta<'b, 'r, 'a>( continue; }; candidate.bound.set(true); - let number = crl_number(candidate, session)?; - if newest.is_none() || crl_number_after(number, newest_number) { - newest = Some((candidate.index, pair)); - newest_number = number; + // Numbers are compared only when a second bound delta exists. + let newer = match &newest { + None => true, + Some((current, _)) => crl_number_after( + crl_number(candidate, session)?, + crl_number(current, session)?, + ), + }; + if newer { + newest = Some((candidate, pair)); } } - Ok(newest) + Ok(newest.map(|(list, pair)| (list.index, pair))) } /// Original `cRLNumber` content octets of a list `bind_delta` has validated. @@ -672,8 +677,11 @@ fn delta_reasons<'a>( }; validate_delta_window(&facts, delta, evaluation.index)?; validate_delta_entry(&facts, delta, utc, evaluation, session)?; - // The pair shares the base's scope; its facts expose reasons only through - // absence, which a not yet effective entry does not establish. + if let Some(absence) = facts.absence() { + return Ok(Some(absence.reasons())); + } + // The facts expose reasons only through absence, which a not yet effective + // entry does not establish; the pair shares the base's scope. match pair .base() .scope(target, execution) From 83be6faecc69756b26e2581cf360d14f786626c6 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 30 Oct 2026 00:30:16 +0200 Subject: [PATCH 03/11] perf(xml): drop the document-order index Every owned-document parse hashed each node into an order table, but the arena stores subtrees contiguously in preorder, so a node's arena index already is its document position. document_order now returns it directly; a debug assertion keeps the invariant checked while the remaining indexes are built. Parse p50 on the saml shape with 16384 units: 39.7 -> 37.0 ms (xmloxide), 30.3 -> 28.6 ms (roxmltree). Part of #340 --- src/document.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/document.rs b/src/document.rs index 3dd000f5..173fe013 100644 --- a/src/document.rs +++ b/src/document.rs @@ -449,7 +449,6 @@ impl ContentReplacementEdit<'_> { #[derive(Debug)] struct DocumentIndexes { - order: Map, default_ids: Map>, attributes_by_value: Map<(String, String), Vec>, } @@ -2120,12 +2119,9 @@ impl<'a> DocumentView<'a> { /// Rejects foreign/stale identities or nodes absent from this generation. pub fn document_order(self, identity: NodeIdentity) -> Result { self.validate_identity(identity)?; - self.parsed - .indexes - .order - .get(&self.resolve_node(identity)?.id()) - .copied() - .ok_or(XmlDocumentError::MissingNode) + // The arena stores every subtree contiguously in preorder, so a node's + // arena index is its document position. + Ok(self.resolve_node(identity)?.id().get_usize()) } /// Return a total order key for a current tree node. @@ -4173,13 +4169,16 @@ fn validate_wrappers( impl DocumentIndexes { fn build(document: &Document<'_>) -> Self { - let mut order = Map::new(); let mut default_ids = Map::new(); let mut duplicate_ids = Set::new(); let mut attributes_by_value: Map<(String, String), Vec> = Map::new(); for (position, node) in document.descendants().enumerate() { - order.insert(node.id(), position); + debug_assert_eq!( + node.id().get_usize(), + position, + "document order must equal the preorder arena index" + ); if !node.is_element() { continue; } @@ -4206,7 +4205,6 @@ impl DocumentIndexes { } } Self { - order, default_ids, attributes_by_value, } From 6cf08d8ae3f40b991ed0954e24e5a31e25691ead Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 07:18:48 +0200 Subject: [PATCH 04/11] perf(xml): drop per-parse indexes and c14n hash maps Profiling verify on the SAML shape with 16384 units showed 14% of the time in SipHash and 19% in the allocator. Every parse copied and hashed each attribute name and value into an index that only node_for_id reads; that lookup now walks the document once. Canonicalization built and copied a hash map per element for a handful of prefixes. The map now holds up to six bindings inline and moves to the keyed table beyond that, so a document declaring many prefixes keeps constant-time lookups. Verify p50 on that shape, macOS arm64: about 59 -> 50 ms from serialized input, 21.5 -> 14.4 ms on a parsed document. Part of #340 --- src/c14n/scratch.rs | 100 ++++++++++++++++++----- src/c14n/scratch/namespace_tests.rs | 42 ++++++++++ src/collections.rs | 8 +- src/document.rs | 121 ++++++++-------------------- src/xml.rs | 4 - 5 files changed, 162 insertions(+), 113 deletions(-) diff --git a/src/c14n/scratch.rs b/src/c14n/scratch.rs index 4eb509b2..03bb190d 100644 --- a/src/c14n/scratch.rs +++ b/src/c14n/scratch.rs @@ -209,29 +209,73 @@ impl List for Vec { } } -#[derive(Clone, Default)] -pub(crate) struct NativeNamespaces<'a>(Map, Option<&'a str>>); +/// Bindings held without a heap table. Elements rarely see more prefixes, and +/// each element builds and copies these maps. +const INLINE_NAMESPACES: usize = 6; + +type NamespaceEntry<'a> = (NamespaceKey<'a>, Option<&'a str>); + +const EMPTY_NAMESPACE: NamespaceEntry<'static> = ( + NamespaceKey { + prefix: "", + utilizer: false, + }, + None, +); + +/// Prefix bindings: a short inline list, then the keyed table, so a document +/// declaring many prefixes keeps constant-time lookups. +#[derive(Clone)] +pub(crate) struct NativeNamespaces<'a> { + inline: [NamespaceEntry<'a>; INLINE_NAMESPACES], + used: usize, + table: Option, Option<&'a str>>>, +} + +impl Default for NativeNamespaces<'_> { + fn default() -> Self { + Self { + inline: [EMPTY_NAMESPACE; INLINE_NAMESPACES], + used: 0, + table: None, + } + } +} + +impl NativeNamespaces<'_> { + fn position(&self, prefix: &str, utilizer: bool) -> Option { + self.inline[..self.used] + .iter() + .position(|(key, _)| key.utilizer == utilizer && key.prefix == prefix) + } +} #[cfg(test)] impl<'a> NativeNamespaces<'a> { pub(super) fn into_pairs(self) -> impl Iterator { - self.0.into_iter().filter_map(|(key, value)| { - if key.utilizer { - None - } else { - value.map(|uri| (key.prefix, uri)) - } - }) + let inline = self.inline[..self.used].to_vec(); + inline + .into_iter() + .chain(self.table.into_iter().flatten()) + .filter_map(|(key, value)| { + if key.utilizer { + None + } else { + value.map(|uri| (key.prefix, uri)) + } + }) } } impl<'a> NamespaceMap<'a> for NativeNamespaces<'a> { fn get(&self, prefix: &str, utilizer: bool) -> Result, C14nError> { - Ok(self - .0 - .get(&NamespaceKey { prefix, utilizer }) - .copied() - .map_or(NamespaceState::Absent, NamespaceState::binding)) + let entry = match &self.table { + Some(table) => table.get(&NamespaceKey { prefix, utilizer }).copied(), + None => self + .position(prefix, utilizer) + .map(|index| self.inline[index].1), + }; + Ok(entry.map_or(NamespaceState::Absent, NamespaceState::binding)) } fn insert( &mut self, @@ -239,14 +283,32 @@ impl<'a> NamespaceMap<'a> for NativeNamespaces<'a> { utilizer: bool, value: Option<&'a str>, ) -> Result<(), C14nError> { - self.0.insert(NamespaceKey { prefix, utilizer }, value); + let key = NamespaceKey { prefix, utilizer }; + if let Some(table) = &mut self.table { + table.insert(key, value); + } else if let Some(index) = self.position(prefix, utilizer) { + self.inline[index].1 = value; + } else if self.used < INLINE_NAMESPACES { + self.inline[self.used] = (key, value); + self.used += 1; + } else { + let mut table: Map<_, _> = self.inline.iter().copied().collect(); + table.insert(key, value); + self.table = Some(table); + self.used = 0; + } Ok(()) } fn remove(&mut self, prefix: &'a str) -> Result<(), C14nError> { - self.0.remove(&NamespaceKey { - prefix, - utilizer: false, - }); + if let Some(table) = &mut self.table { + table.remove(&NamespaceKey { + prefix, + utilizer: false, + }); + } else if let Some(index) = self.position(prefix, false) { + self.used -= 1; + self.inline[index] = self.inline[self.used]; + } Ok(()) } fn copy(&self) -> Result { diff --git a/src/c14n/scratch/namespace_tests.rs b/src/c14n/scratch/namespace_tests.rs index dc4f4e96..d11693ed 100644 --- a/src/c14n/scratch/namespace_tests.rs +++ b/src/c14n/scratch/namespace_tests.rs @@ -65,6 +65,48 @@ fn native_namespaces_keep_absent_excluded_and_empty_uri_distinct() { check_namespace_states(Native.namespaces()); } +#[test] +fn native_namespaces_keep_every_state_across_the_inline_boundary() { + // The same contract holds with free inline slots, exactly full and spilled, + // and no earlier binding is lost when the bindings move to the table. + const PREFIXES: [&str; 8] = ["a", "b", "c", "d", "e", "f", "g", "h"]; + for filled in 0..=PREFIXES.len() { + let mut namespaces = Native.namespaces(); + for prefix in &PREFIXES[..filled] { + namespaces + .insert(prefix, false, Some(prefix)) + .expect("fill binding"); + } + for prefix in &PREFIXES[..filled] { + assert_eq!( + namespaces.get(prefix, false).expect("filled binding"), + NamespaceState::Bound(prefix), + "{filled} bindings" + ); + } + check_namespace_states(namespaces.clone()); + // A repeated insert replaces, so it must not consume another slot. + for _ in 0..PREFIXES.len() { + namespaces + .insert("repeat", false, Some("urn:repeat")) + .expect("repeat binding"); + } + if let Some(first) = PREFIXES[..filled].first() { + namespaces.remove(first).expect("remove first binding"); + assert_eq!( + namespaces.get(first, false).expect("removed binding"), + NamespaceState::Absent + ); + } + if let Some(last) = PREFIXES[..filled].last().filter(|_| filled > 1) { + assert_eq!( + namespaces.get(last, false).expect("last binding survives"), + NamespaceState::Bound(last) + ); + } + } +} + #[cfg(feature = "xmldsig")] #[test] fn accounted_namespaces_share_states_and_release_retained_capacity() { diff --git a/src/collections.rs b/src/collections.rs index 8eaf6280..98e22ef4 100644 --- a/src/collections.rs +++ b/src/collections.rs @@ -5,12 +5,16 @@ //! so ordered trees take their place. use alloc::collections::TryReserveError; +#[cfg(all(not(feature = "std"), any(feature = "xmldsig", test)))] +pub(crate) use alloc::collections::btree_map::Entry; #[cfg(not(feature = "std"))] -pub(crate) use alloc::collections::{BTreeMap as Map, BTreeSet as Set, btree_map::Entry}; +pub(crate) use alloc::collections::{BTreeMap as Map, BTreeSet as Set}; #[cfg(all(not(feature = "std"), feature = "xmldsig"))] pub(crate) use alloc::collections::{btree_map as map, btree_set as set}; +#[cfg(all(feature = "std", any(feature = "xmldsig", test)))] +pub(crate) use std::collections::hash_map::Entry; #[cfg(feature = "std")] -pub(crate) use std::collections::{HashMap as Map, HashSet as Set, hash_map::Entry}; +pub(crate) use std::collections::{HashMap as Map, HashSet as Set}; #[cfg(all(feature = "std", feature = "xmldsig"))] pub(crate) use std::collections::{hash_map as map, hash_set as set}; diff --git a/src/document.rs b/src/document.rs index 173fe013..103d9e45 100644 --- a/src/document.rs +++ b/src/document.rs @@ -21,7 +21,9 @@ use core::sync::atomic::Ordering; #[cfg(not(target_has_atomic = "64"))] use portable_atomic::AtomicU64; -use crate::collections::{Entry, Map, Set, Table}; +#[cfg(feature = "xmldsig")] +use crate::collections::Map; +use crate::collections::{Set, Table}; use crate::xml::dom::storage::names::{NameStorage, Names}; use crate::xml::dom::storage::{ Native as NativeXmlStorage, Storage as XmlStorage, Values as XmlValues, @@ -447,12 +449,6 @@ impl ContentReplacementEdit<'_> { } } -#[derive(Debug)] -struct DocumentIndexes { - default_ids: Map>, - attributes_by_value: Map<(String, String), Vec>, -} - #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] mod policy_sealed { pub trait Sealed {} @@ -501,7 +497,6 @@ impl_document_policy!(crate::policy::DecryptionPolicy); struct ParsedDocument<'input> { document: Document<'input>, - indexes: DocumentIndexes, node_count: usize, #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] max_depth: usize, @@ -2076,31 +2071,23 @@ impl<'a> DocumentView<'a> { value: &str, registrations: &[IdAttributeRegistration], ) -> Option { - let mut matched = match self.parsed.indexes.default_ids.get(value) { - Some(Some(node)) => Some(*node), - Some(None) => return None, - None => None, - }; - for registration in registrations { - let key = ( - registration.attribute_local_name().to_owned(), - value.to_owned(), - ); - if let Some(nodes) = self.parsed.indexes.attributes_by_value.get(&key) { - for node_id in nodes { - let recognized = self.parsed.document.get_node(*node_id).is_some_and(|node| { - node.attributes().any(|attribute| { - attribute.value() == value && registration.matches(node, attribute) - }) - }); - if !recognized { - continue; - } - if matched.is_some_and(|previous| previous != *node_id) { - return None; - } - matched = Some(*node_id); + // One walk per query: operations resolve through their own ID index, so + // parsing does not pay to copy every attribute for this rare lookup. + let mut matched = None; + for node in self.parsed.document.descendants() { + if !node.is_element() { + continue; + } + for attribute in node.attributes() { + if attribute.value() != value + || !crate::xml::is_id_attribute(node, attribute, registrations) + { + continue; + } + if matched.is_some_and(|previous| previous != node.id()) { + return None; } + matched = Some(node.id()); } } matched.map(|node| self.node_identity_by_id(node)) @@ -2377,10 +2364,15 @@ fn build_semantic_cell( let _ = parse_work; DocumentCell::try_new(xml, |source| { let (document, metrics) = parse_semantic_document(source, settings)?; - let indexes = DocumentIndexes::build(&document); + debug_assert!( + document + .descendants() + .enumerate() + .all(|(position, node)| node.id().get_usize() == position), + "document order must equal the preorder arena index" + ); Ok::<_, XmlDocumentError>(ParsedDocument { document, - indexes, node_count: metrics.node_count, #[cfg(any(feature = "xmldsig", feature = "xmlenc"))] max_depth: metrics.max_depth, @@ -4167,50 +4159,6 @@ fn validate_wrappers( } } -impl DocumentIndexes { - fn build(document: &Document<'_>) -> Self { - let mut default_ids = Map::new(); - let mut duplicate_ids = Set::new(); - let mut attributes_by_value: Map<(String, String), Vec> = Map::new(); - - for (position, node) in document.descendants().enumerate() { - debug_assert_eq!( - node.id().get_usize(), - position, - "document order must equal the preorder arena index" - ); - if !node.is_element() { - continue; - } - for attribute in node.attributes() { - attributes_by_value - .entry((attribute.name().to_owned(), attribute.value().to_owned())) - .or_default() - .push(node.id()); - if !crate::xml::is_id_attribute(node, attribute, &[]) - || duplicate_ids.contains(attribute.value()) - { - continue; - } - match default_ids.entry(attribute.value().to_owned()) { - Entry::Vacant(entry) => { - entry.insert(Some(node.id())); - } - Entry::Occupied(mut entry) if entry.get() != &Some(node.id()) => { - entry.insert(None); - duplicate_ids.insert(attribute.value().to_owned()); - } - Entry::Occupied(_) => {} - } - } - } - Self { - default_ids, - attributes_by_value, - } - } -} - fn element_content_range(element: &str) -> Result<(Range, String, bool), XmlDocumentError> { let start_end = element_opening_end(element)?; let start_tag = &element[..=start_end]; @@ -4684,16 +4632,13 @@ mod tests { let retained = build_semantic_cell(xml.to_owned(), settings, 1, 0) .expect("selected backend semantic projection must parse"); retained.with_dependent(|_, parsed| { - assert!(parsed.indexes.default_ids.contains_key("target")); - assert_eq!( - parsed - .document - .root_element() - .first_element_child() - .expect("fixture must contain the indexed child element") - .text(), - Some("value") - ); + let item = parsed + .document + .root_element() + .first_element_child() + .expect("fixture must contain the identified child element"); + assert_eq!(item.attribute("ID"), Some("target")); + assert_eq!(item.text(), Some("value")); }); } diff --git a/src/xml.rs b/src/xml.rs index 17127897..c17d5375 100644 --- a/src/xml.rs +++ b/src/xml.rs @@ -121,10 +121,6 @@ impl IdAttributeRegistration { && self.matches_node(node) } - pub(crate) fn attribute_local_name(&self) -> &str { - &self.attribute_local_name - } - pub(crate) fn matches_node(&self, node: Node<'_, '_, S>) -> bool { match &self.element_scope { IdAttributeElementScope::AnyElement => true, From 03dcc783783b0f4d0f28ac31d500f718f03dff58 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 07:40:17 +0200 Subject: [PATCH 05/11] perf(xml): share namespace sets and drop child lists --- src/xml/dom/roxmltree.rs | 46 +++++-- src/xml/dom/tree.rs | 218 ++++++++++++++++++--------------- src/xml/dom/tree/comparison.rs | 28 +++-- src/xml/dom/xmloxide.rs | 71 +++++++---- src/xmldsig/extension/tree.rs | 2 +- 5 files changed, 222 insertions(+), 143 deletions(-) diff --git a/src/xml/dom/roxmltree.rs b/src/xml/dom/roxmltree.rs index e0827cd0..850a5ae8 100644 --- a/src/xml/dom/roxmltree.rs +++ b/src/xml/dom/roxmltree.rs @@ -6,7 +6,9 @@ use alloc::string::ToString; use super::{ Document, LexicalPreflight, ParseError, ParsingOptions, XmlBackendImplementation, - tree::{AttributeData, NamespaceData, NodeId, NodeKind, TreeBuilder, range_contains}, + tree::{ + AttributeData, NamespaceData, NamespaceSet, NodeId, NodeKind, TreeBuilder, range_contains, + }, }; pub(super) struct RoxmltreeBackend; @@ -159,14 +161,7 @@ fn project_document( xml_id_error: None, })?; } - let mut namespaces = storage.values(source.namespaces().len())?; - for namespace in source.namespaces() { - storage.charge(1)?; - namespaces.push(NamespaceData { - prefix: storage.optional_text(namespace.name())?, - uri: storage.text(namespace.uri())?, - })?; - } + let namespaces = project_namespaces(target, source, parent)?; NodeKind::Element { name: storage.text(source.tag_name().name())?, namespace: storage.optional_text( @@ -213,6 +208,39 @@ fn project_document( Ok(()) } +/// The element's in-scope set, sharing its parent's when the bindings agree. +fn project_namespaces( + target: &mut TreeBuilder<'_, S>, + source: backend::Node<'_, '_, R>, + parent: Option, +) -> Result { + let storage = target.storage(); + let inherited = target.namespace_set_of(parent); + let bindings = target.namespace_bindings(inherited); + let mut same = bindings.len() == source.namespaces().len(); + if same { + for (binding, namespace) in bindings.iter().zip(source.namespaces()) { + storage.charge(binding.uri.len().saturating_add(1))?; + if binding.prefix.as_deref() != namespace.name() || &*binding.uri != namespace.uri() { + same = false; + break; + } + } + } + if same { + return Ok(inherited); + } + let mut namespaces = storage.values(source.namespaces().len())?; + for namespace in source.namespaces() { + storage.charge(1)?; + namespaces.push(NamespaceData { + prefix: storage.optional_text(namespace.name())?, + uri: storage.text(namespace.uri())?, + })?; + } + target.add_namespace_set(namespaces) +} + fn element_prefix<'input, R: Storage, S: Storage>( node: backend::Node<'_, 'input, R>, storage: S, diff --git a/src/xml/dom/tree.rs b/src/xml/dom/tree.rs index 3bc90ed3..968bec39 100644 --- a/src/xml/dom/tree.rs +++ b/src/xml/dom/tree.rs @@ -52,7 +52,9 @@ pub enum NodeType { pub(super) struct NodeData { pub(super) parent: Option, - pub(super) children: S::Values, + // Subtrees are contiguous in preorder, so children are reached through + // `subtree_end` and need no list of their own. + pub(super) child_count: u32, pub(super) kind: NodeKind, pub(super) range: Range, pub(super) range_actionable: bool, @@ -63,6 +65,20 @@ pub(super) struct NodeData { pub(super) xpath_start: u32, } +/// One in-scope binding list in the document's table. An element without its +/// own declarations shares its parent's list. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) struct NamespaceSet(u32); + +impl NamespaceSet { + /// No bindings; every document's first table entry. + pub(super) const EMPTY: Self = Self(0); + + fn index(self) -> usize { + self.0 as usize + } +} + pub(super) enum NodeKind { Root, Element { @@ -70,7 +86,7 @@ pub(super) enum NodeKind { namespace: Option, prefix: Option, attributes: S::Values>, - namespaces: S::Values>, + namespaces: NamespaceSet, }, Text(S::Text), Comment(S::Text), @@ -182,60 +198,16 @@ impl core::fmt::Debug for NodeKind { .field("namespace", namespace) .field("prefix", prefix) .field("attributes", &&attributes[..]) - .field("namespaces", &&namespaces[..]) + .field("namespaces", namespaces) .finish(), } } } -impl PartialEq for NodeKind { - fn eq(&self, other: &Self) -> bool { - match (self, other) { - (Self::Root, Self::Root) => true, - (Self::Text(a), Self::Text(b)) | (Self::Comment(a), Self::Comment(b)) => { - a.as_ref() == b.as_ref() - } - ( - Self::PI { - target: a, - value: av, - }, - Self::PI { - target: b, - value: bv, - }, - ) => a.as_ref() == b.as_ref() && av.as_deref() == bv.as_deref(), - ( - Self::Element { - name: a, - namespace: an, - prefix: ap, - attributes: aa, - namespaces: ans, - }, - Self::Element { - name: b, - namespace: bn, - prefix: bp, - attributes: ba, - namespaces: bns, - }, - ) => { - a.as_ref() == b.as_ref() - && an.as_deref() == bn.as_deref() - && ap.as_deref() == bp.as_deref() - && aa[..] == ba[..] - && ans[..] == bns[..] - } - _ => false, - } - } -} -impl Eq for NodeKind {} impl core::fmt::Debug for NodeData { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { f.debug_struct("NodeData") .field("parent", &self.parent) - .field("children", &&self.children[..]) + .field("child_count", &self.child_count) .field("kind", &self.kind) .field("range", &self.range) .field("range_actionable", &self.range_actionable) @@ -248,6 +220,7 @@ impl core::fmt::Debug for NodeData { pub(super) struct TreeBuilder<'input, S: Storage = Native> { input: &'input str, nodes: S::Values>, + namespace_sets: S::Values>>, storage: S, } @@ -256,13 +229,40 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { self.storage } pub(super) fn new(input: &'input str, capacity: usize, storage: S) -> Result { + let mut namespace_sets = storage.values(1)?; + namespace_sets.push(storage.values(0)?)?; Ok(Self { input, nodes: storage.values(capacity)?, + namespace_sets, storage, }) } + /// The set of `node`, or the empty one when it is not an element. + pub(super) fn namespace_set_of(&self, node: Option) -> NamespaceSet { + match node.map(|node| &self.nodes[node.index()].kind) { + Some(NodeKind::Element { namespaces, .. }) => *namespaces, + _ => NamespaceSet::EMPTY, + } + } + + pub(super) fn namespace_bindings(&self, set: NamespaceSet) -> &[NamespaceData] { + &self.namespace_sets[set.index()] + } + + pub(super) fn add_namespace_set( + &mut self, + bindings: S::Values>, + ) -> Result { + let set = NamespaceSet( + u32::try_from(self.namespace_sets.len()) + .expect("bounded XML node count must fit into u32"), + ); + self.namespace_sets.push(bindings)?; + Ok(set) + } + #[cfg(feature = "xml-backend-xmloxide")] pub(super) fn push( &mut self, @@ -285,7 +285,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { ); self.nodes.push(NodeData { parent, - children: self.storage.values(0)?, + child_count: 0, kind, range, range_actionable, @@ -294,7 +294,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { xpath_start: 0, })?; if let Some(parent) = parent { - self.nodes[parent.index()].children.push(id)?; + self.nodes[parent.index()].child_count += 1; } Ok(id) } @@ -306,10 +306,11 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { #[cfg(feature = "xml-backend-xmloxide")] pub(super) fn has_text_tail(&self, parent: NodeId) -> bool { - self.nodes[parent.index()] - .children - .last() - .is_some_and(|last| matches!(self.nodes[last.index()].kind, NodeKind::Text(_))) + // In preorder a parent's text tail is the last node pushed: an element + // child, or anything below it, would have been pushed after it. + self.nodes.last().is_some_and(|last| { + last.parent == Some(parent) && matches!(last.kind, NodeKind::Text(_)) + }) } #[cfg(feature = "xml-backend-xmloxide")] @@ -320,8 +321,8 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { range: Range, range_actionable: bool, ) -> Result<(), ParseError> { - if let Some(last) = self.nodes[parent.index()].children.last().copied() - && let node = &mut self.nodes[last.index()] + if let Some(node) = self.nodes.last_mut() + && node.parent == Some(parent) && let NodeKind::Text(existing) = &mut node.kind { self.storage.append(existing, value)?; @@ -347,14 +348,6 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { self.nodes.len() } - #[cfg(feature = "xml-backend-xmloxide")] - pub(super) fn namespaces(&self, node: NodeId) -> Option<&[NamespaceData]> { - match &self.nodes[node.index()].kind { - NodeKind::Element { namespaces, .. } => Some(namespaces), - _ => None, - } - } - pub(super) fn finish( mut self, preflight: &super::LexicalPreflight<'_, P>, @@ -371,7 +364,9 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { attributes, namespaces, .. - } => attributes.len().checked_add(namespaces.len()), + } => attributes + .len() + .checked_add(self.namespace_sets[namespaces.index()].len()), _ => Some(0), }; xpath_nodes = axes @@ -429,6 +424,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { Ok(Document { input: self.input, nodes: self.nodes, + namespace_sets: self.namespace_sets, #[cfg(feature = "xmldsig")] xpath_nodes, }) @@ -447,6 +443,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { pub struct Document<'input, S: Storage = Native> { input: &'input str, nodes: S::Values>, + namespace_sets: S::Values>>, #[cfg(feature = "xmldsig")] xpath_nodes: u32, } @@ -566,6 +563,24 @@ impl<'input, S: Storage> Document<'input, S> { .map(|_| Node { document: self, id }) } + pub(super) fn namespace_bindings(&self, set: NamespaceSet) -> &[NamespaceData] { + &self.namespace_sets[set.index()] + } + + /// The child of `parent` whose subtree ends at `end`. That subtree's last + /// node is `end - 1`, so its ancestor chain reaches the child within the + /// document's depth. + fn last_child_before(&self, parent: NodeId, end: u32) -> Option { + let mut id = NodeId(end.checked_sub(1)?); + loop { + let up = self.nodes.get(id.index())?.parent?; + if up == parent { + return Some(id); + } + id = up; + } + } + /// Number of `XPath` nodes: tree nodes with their attribute and namespace nodes. #[cfg(feature = "xmldsig")] pub(crate) fn xpath_node_count(&self) -> u32 { @@ -726,7 +741,10 @@ impl<'a, 'input, S: Storage> Node<'a, 'input, S> { pub fn children(self) -> Children<'a, 'input, S> { Children { document: self.document, - ids: self.data().children.iter(), + parent: self.id, + front: self.id.0 + 1, + back: self.data().subtree_end, + remaining: self.data().child_count, } } /// Returns the first direct child node. @@ -773,36 +791,20 @@ impl<'a, 'input, S: Storage> Node<'a, 'input, S> { /// Returns the preceding sibling. #[must_use] pub fn prev_sibling(self) -> Option { - let parent = self.parent()?; - let position = parent - .data() - .children - .iter() - .position(|id| *id == self.id)?; - position - .checked_sub(1) - .and_then(|index| self.document.get_node(parent.data().children[index])) - } - /// Returns the following sibling. + let parent = self.data().parent?; + if self.id.0 == parent.0 + 1 { + return None; + } + // The node before this one ends the previous sibling's subtree. + self.document + .last_child_before(parent, self.id.0) + .and_then(|id| self.document.get_node(id)) + } + /// Returns the following sibling. The arena stores each subtree + /// contiguously in preorder, so the node after this subtree is the next + /// sibling exactly when it has the same parent. #[must_use] pub fn next_sibling(self) -> Option { - let parent = self.parent()?; - let position = parent - .data() - .children - .iter() - .position(|id| *id == self.id)?; - parent - .data() - .children - .get(position + 1) - .and_then(|id| self.document.get_node(*id)) - } - /// Returns the following sibling in constant time. The arena stores each - /// subtree contiguously in preorder, so the node after this subtree is the - /// next sibling exactly when it has the same parent. - #[cfg(feature = "xmldsig")] - pub(crate) fn following_sibling(self) -> Option { let parent = self.data().parent?; let next = NodeId(self.data().subtree_end); let data = self.document.nodes.get(next.index())?; @@ -892,7 +894,7 @@ impl<'a, 'input, S: Storage> Node<'a, 'input, S> { #[must_use] pub fn namespaces(self) -> Namespaces<'a, S> { let values: &[NamespaceData] = match &self.data().kind { - NodeKind::Element { namespaces, .. } => &namespaces[..], + NodeKind::Element { namespaces, .. } => self.document.namespace_bindings(*namespaces), _ => &[], }; Namespaces { @@ -1110,23 +1112,37 @@ pub struct PI<'a> { /// Iterator over direct child nodes. pub struct Children<'a, 'input, S: Storage = Native> { document: &'a Document<'input, S>, - ids: core::slice::Iter<'a, NodeId>, + parent: NodeId, + front: u32, + back: u32, + remaining: u32, } impl<'a, 'input, S: Storage> Iterator for Children<'a, 'input, S> { type Item = Node<'a, 'input, S>; fn next(&mut self) -> Option { - self.ids.next().and_then(|id| self.document.get_node(*id)) + if self.remaining == 0 { + return None; + } + self.remaining -= 1; + let node = self.document.get_node(NodeId(self.front))?; + self.front = node.data().subtree_end; + Some(node) } fn size_hint(&self) -> (usize, Option) { - self.ids.size_hint() + let remaining = self.remaining as usize; + (remaining, Some(remaining)) } } impl ExactSizeIterator for Children<'_, '_, S> {} impl DoubleEndedIterator for Children<'_, '_, S> { fn next_back(&mut self) -> Option { - self.ids - .next_back() - .and_then(|id| self.document.get_node(*id)) + if self.remaining == 0 { + return None; + } + self.remaining -= 1; + let id = self.document.last_child_before(self.parent, self.back)?; + self.back = id.0; + self.document.get_node(id) } } diff --git a/src/xml/dom/tree/comparison.rs b/src/xml/dom/tree/comparison.rs index e123620b..bbebe3e6 100644 --- a/src/xml/dom/tree/comparison.rs +++ b/src/xml/dom/tree/comparison.rs @@ -141,7 +141,7 @@ impl Document<'_, L> { ))?)); } for (index, (left, right)) in self.nodes.iter().zip(other.nodes.iter()).enumerate() { - if let Some(field) = difference(left, right, doctype, storage)? { + if let Some(field) = difference(self, other, left, right, doctype, storage)? { let reason = if field == "source range" { storage.formatted(format_args!( "retained semantic node {index} differs in source range ({}..{} versus {}..{})", @@ -180,6 +180,8 @@ fn optional_equal( } fn difference( + left_document: &Document<'_, L>, + right_document: &Document<'_, R>, left: &NodeData, right: &NodeData, doctype: Option<&Range>, @@ -189,14 +191,16 @@ fn difference( if left.parent != right.parent { return Ok(Some("parent identity")); } - if left.children.len() != right.children.len() { + // With every node's parent compared, the child count catches a difference + // in child identities here and keeps its diagnostic. + if left.child_count != right.child_count { return Ok(Some("child identities")); } - storage.charge(left.children.len())?; - if left.children[..] != right.children[..] { - return Ok(Some("child identities")); - } - if let Some(field) = kind_difference(&left.kind, &right.kind, storage)? { + if let Some(field) = kind_difference( + (left_document, &left.kind), + (right_document, &right.kind), + storage, + )? { return Ok(Some(field)); } if !source_ranges_equivalent(&left.range, &right.range, doctype) { @@ -214,8 +218,8 @@ fn difference( } fn kind_difference( - left: &NodeKind, - right: &NodeKind, + (left_document, left): (&Document<'_, L>, &NodeKind), + (right_document, right): (&Document<'_, R>, &NodeKind), storage: S, ) -> Result, ParseError> { match (left, right) { @@ -284,7 +288,11 @@ fn kind_difference( if !attributes_equal(la, ra, storage)? { return Ok(Some("attribute axis")); } - if !namespaces_equal(lx, rx, storage)? { + if !namespaces_equal( + left_document.namespace_bindings(*lx), + right_document.namespace_bindings(*rx), + storage, + )? { return Ok(Some("namespace axis")); } Ok(None) diff --git a/src/xml/dom/xmloxide.rs b/src/xml/dom/xmloxide.rs index 17d87d56..b3c8f0fb 100644 --- a/src/xml/dom/xmloxide.rs +++ b/src/xml/dom/xmloxide.rs @@ -11,7 +11,7 @@ use xmloxide::tree::{NodeId as XmloxNodeId, NodeKind as XmloxNodeKind}; use super::{ Document, LexicalPreflight, ParseError, ParsingOptions, XmlBackendImplementation, preflight::{PositionCursor, SourceKind}, - tree::{AttributeData, NamespaceData, NodeId, NodeKind, TreeBuilder}, + tree::{AttributeData, NamespaceData, NamespaceSet, NodeId, NodeKind, TreeBuilder}, }; pub(super) struct XmloxideBackend; @@ -243,30 +243,11 @@ impl Projector<'_, '_, '_, S, R> { Some(range) => range.clone(), None => self.positions.take(SourceKind::Element)?, }; - let inherited = namespace_owner - .and_then(|owner| self.target.namespaces(owner)) - .unwrap_or_default(); - let mut namespaces = storage.values(inherited.len())?; - for namespace in inherited { - storage.charge(1)?; - namespaces.push(NamespaceData { - prefix: storage.optional_text(namespace.prefix.as_deref())?, - uri: storage.text(namespace.uri.as_ref())?, - })?; - } + let namespaces = self.project_namespaces(attributes, namespace_owner)?; let mut semantic_attributes = storage.values(attributes.len())?; for attribute in &**attributes { storage.charge(1)?; - if attribute.prefix.as_deref() == Some("xmlns") { - set_namespace( - &mut namespaces, - Some(&attribute.name), - &attribute.value, - storage, - )?; - } else if attribute.prefix.is_none() && attribute.name.as_ref() == "xmlns" { - set_namespace(&mut namespaces, None, &attribute.value, storage)?; - } else { + if namespace_declaration(attribute).is_none() { semantic_attributes.push(AttributeData { name: storage.text(&attribute.name)?, namespace: storage.optional_text(attribute.namespace.as_deref())?, @@ -292,6 +273,38 @@ impl Projector<'_, '_, '_, S, R> { ) } + /// The element's in-scope set: its parent's, unless it declares bindings. + fn project_namespaces( + &mut self, + attributes: &[xmloxide::tree::Attribute], + namespace_owner: Option, + ) -> Result { + let storage = self.target.storage(); + let inherited = self.target.namespace_set_of(namespace_owner); + storage.charge(attributes.len())?; + if attributes + .iter() + .all(|attribute| namespace_declaration(attribute).is_none()) + { + return Ok(inherited); + } + let inherited = self.target.namespace_bindings(inherited); + let mut namespaces = storage.values(inherited.len())?; + for namespace in inherited { + storage.charge(1)?; + namespaces.push(NamespaceData { + prefix: storage.optional_text(namespace.prefix.as_deref())?, + uri: storage.text(namespace.uri.as_ref())?, + })?; + } + for attribute in attributes { + if let Some(prefix) = namespace_declaration(attribute) { + set_namespace(&mut namespaces, prefix, &attribute.value, storage)?; + } + } + self.target.add_namespace_set(namespaces) + } + fn project_leaf( &mut self, source: XmloxNodeId, @@ -404,6 +417,20 @@ impl Projector<'_, '_, '_, S, R> { } } +/// The bound prefix when `attribute` declares a namespace: `Some(None)` is the +/// default namespace. +fn namespace_declaration( + attribute: &xmloxide::tree::Attribute, +) -> Option> { + if attribute.prefix.as_deref() == Some("xmlns") { + Some(Some(&attribute.name)) + } else if attribute.prefix.is_none() && attribute.name.as_ref() == "xmlns" { + Some(None) + } else { + None + } +} + fn set_namespace( namespaces: &mut S::Values>, prefix: Option<&str>, diff --git a/src/xmldsig/extension/tree.rs b/src/xmldsig/extension/tree.rs index 9ca1ca16..dff07a43 100644 --- a/src/xmldsig/extension/tree.rs +++ b/src/xmldsig/extension/tree.rs @@ -112,7 +112,7 @@ pub fn next_sibling( return Err(TreeError::NotChild); } Ok(node - .following_sibling() + .next_sibling() .map(|sibling| view.node_identity(sibling))) } From 16545724f6ff8516ad783086e070826830227de3 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 07:46:58 +0200 Subject: [PATCH 06/11] perf(xml): move xmloxide text into the semantic tree --- benches/security.rs | 11 ++-- examples/benchmark_latency.rs | 19 +++--- src/benchmark_support.rs | 5 +- src/xml/dom/benchmark.rs | 6 +- src/xml/dom/tree.rs | 39 ++++++------ src/xml/dom/xmloxide.rs | 115 ++++++++++++++++++++++++---------- 6 files changed, 120 insertions(+), 75 deletions(-) diff --git a/benches/security.rs b/benches/security.rs index de902beb..81edc531 100644 --- a/benches/security.rs +++ b/benches/security.rs @@ -86,13 +86,14 @@ fn large_reference_sign(bencher: divan::Bencher, units: usize) { #[divan::bench(args = specs().filter(|spec| spec.backend != xml_sec::XmlBackend::Differential))] fn projection(bencher: divan::Bencher, spec: Spec) { let fixture = Fixture::new(spec); - let projection = xml_sec::benchmark_support::Projection::new(&fixture.unsigned, spec.backend) - .expect("prepared backend"); bencher .counter(divan::counter::BytesCount::new(fixture.unsigned.len())) - .bench_local(|| { - std::hint::black_box(projection.project().expect("project")); - }); + // Projection consumes its backend DOM, so each sample prepares a new one. + .with_inputs(|| { + xml_sec::benchmark_support::Projection::new(&fixture.unsigned, spec.backend) + .expect("prepared backend") + }) + .bench_local_values(|projection| projection.project().expect("project")); } /// Signed-document shape for node-set measurements: `elements` siblings with diff --git a/examples/benchmark_latency.rs b/examples/benchmark_latency.rs index d5a4080e..b57c055a 100644 --- a/examples/benchmark_latency.rs +++ b/examples/benchmark_latency.rs @@ -25,10 +25,15 @@ fn percentile(sorted: &[u128], percent: usize) -> u128 { fn measure( fixture: &Fixture, operation: &str, - projection: Option<&xml_sec::benchmark_support::Projection<'_>>, + backend: XmlBackend, policy: &xml_sec::policy::VerificationPolicy, ) -> (u128, usize) { // Per-sample preparation is excluded, just as Divan's with_inputs excludes it. + // Projection consumes its backend DOM, so each sample prepares a new one. + let projection = (operation == "projection").then(|| { + xml_sec::benchmark_support::Projection::new(&fixture.unsigned, backend) + .expect("prepared projection") + }); let mut plan = match operation { "plan_compile" | "plan_execute" => Some(xml_sec::benchmark_support::Plan::new( fixture.spec.units.min(64), @@ -332,14 +337,6 @@ fn main() -> Result<(), Box> { } = parse_request(&args)?; let (units, backend) = (spec.units, spec.backend); let fixture = Fixture::new(spec); - let projection = if operation == "projection" { - Some(xml_sec::benchmark_support::Projection::new( - &fixture.unsigned, - backend, - )?) - } else { - None - }; let policy = xml_sec::policy::VerificationPolicy { signature_algorithms: Some( [xml_sec::xmldsig::SignatureAlgorithm::RsaSha256] @@ -349,12 +346,12 @@ fn main() -> Result<(), Box> { ..Default::default() }; for _ in 0..8 { - std::hint::black_box(measure(&fixture, operation, projection.as_ref(), &policy)); + std::hint::black_box(measure(&fixture, operation, backend, &policy)); } let mut timings = Vec::with_capacity(samples); let mut output_bytes = 0; for _ in 0..samples { - let (elapsed, bytes) = measure(&fixture, operation, projection.as_ref(), &policy); + let (elapsed, bytes) = measure(&fixture, operation, backend, &policy); output_bytes = bytes; timings.push(elapsed); } diff --git a/src/benchmark_support.rs b/src/benchmark_support.rs index 2c91da21..c867e00c 100644 --- a/src/benchmark_support.rs +++ b/src/benchmark_support.rs @@ -21,8 +21,9 @@ impl<'a> Projection<'a> { )?)) } - /// Build the real normalized arena, retaining it until the caller drops it. - pub fn project(&self) -> Result, crate::xml::dom::ParseError> { + /// Build the real normalized arena from the prepared tree, which it + /// consumes, retaining the arena until the caller drops it. + pub fn project(self) -> Result, crate::xml::dom::ParseError> { self.0.project() } } diff --git a/src/xml/dom/benchmark.rs b/src/xml/dom/benchmark.rs index a98ce1d7..a496e8e7 100644 --- a/src/xml/dom/benchmark.rs +++ b/src/xml/dom/benchmark.rs @@ -45,10 +45,10 @@ impl<'a> Projection<'a> { }) } - pub(crate) fn project(&self) -> Result, ParseError> { - match &self.parsed { + pub(crate) fn project(self) -> Result, ParseError> { + match self.parsed { Parsed::Roxmltree(parsed) => { - super::roxmltree::project(self.input, parsed, &self.preflight) + super::roxmltree::project(self.input, &parsed, &self.preflight) } Parsed::Xmloxide(parsed) => super::xmloxide::project( self.input, diff --git a/src/xml/dom/tree.rs b/src/xml/dom/tree.rs index 968bec39..59008693 100644 --- a/src/xml/dom/tree.rs +++ b/src/xml/dom/tree.rs @@ -317,7 +317,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { pub(super) fn append_text( &mut self, parent: NodeId, - value: &str, + value: S::Text, range: Range, range_actionable: bool, ) -> Result<(), ParseError> { @@ -325,7 +325,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { && node.parent == Some(parent) && let NodeKind::Text(existing) = &mut node.kind { - self.storage.append(existing, value)?; + self.storage.append(existing, &value)?; // One semantic text node can fold several adjacent lexical tokens // (plain text, CDATA, and references). Its source range must cover // every token so mutation never splices only a semantic prefix. @@ -334,12 +334,7 @@ impl<'input, S: Storage> TreeBuilder<'input, S> { node.range_actionable &= range_actionable; return Ok(()); } - self.push_with_actionability( - Some(parent), - NodeKind::Text(self.storage.text(value)?), - range, - range_actionable, - )?; + self.push_with_actionability(Some(parent), NodeKind::Text(value), range, range_actionable)?; Ok(()) } @@ -1311,15 +1306,17 @@ mod tests { let xml = "leftright"; let preflight = super::super::LexicalPreflight::scan(xml, false).expect("fixed XML preflight"); - let raw = crate::parser_xmloxide::parser::parse_str_with_options( - xml, - &super::super::xmloxide::backend_options(), - ) - .expect("fixed backend XML"); + let raw = || { + crate::parser_xmloxide::parser::parse_str_with_options( + xml, + &super::super::xmloxide::backend_options(), + ) + .expect("fixed backend XML") + }; let memory = Memory::new(std::sync::Arc::new(MemoryAccount::new(1 << 20, 1 << 20))); let document = super::super::xmloxide::project_in( xml, - &raw, + raw(), super::ParsingOptions::default(), &preflight, super::super::storage::Accounted(&memory), @@ -1333,7 +1330,7 @@ mod tests { assert!(matches!( super::super::xmloxide::project_in( xml, - &raw, + raw(), super::ParsingOptions::default(), &preflight, super::super::storage::Accounted(&denied) @@ -1355,16 +1352,16 @@ mod tests { let source = format!("", "x".repeat(65536)); let preflight = super::super::LexicalPreflight::scan(&source, false).expect("native lexical baseline"); - let raw = crate::parser_xmloxide::parser::parse_str_with_options( - &source, - &super::super::xmloxide::backend_options(), - ) - .expect("native raw XML baseline"); for (limit, capacity) in [(0, 0), (1, 4096)] { + let raw = crate::parser_xmloxide::parser::parse_str_with_options( + &source, + &super::super::xmloxide::backend_options(), + ) + .expect("native raw XML baseline"); let memory = Memory::new(std::sync::Arc::new(MemoryAccount::new(capacity, 1 << 24))); let result = super::super::xmloxide::project_in( &source, - &raw, + raw, super::ParsingOptions { nodes_limit: limit, allow_dtd: false, diff --git a/src/xml/dom/xmloxide.rs b/src/xml/dom/xmloxide.rs index b3c8f0fb..fb4b32f1 100644 --- a/src/xml/dom/xmloxide.rs +++ b/src/xml/dom/xmloxide.rs @@ -1,6 +1,6 @@ //! `xmloxide` parser adapter for the shared semantic arena. -use alloc::string::ToString; +use alloc::string::{String, ToString}; use core::ops::Range; use super::storage::names::NameStorage; @@ -41,10 +41,46 @@ impl XmloxideBackend { diagnostics: diagnostics.into_owned(), }), })?; - Ok(project_in(input, &parsed, options, preflight, storage)?) + Ok(project_in(input, parsed, options, preflight, storage)?) } } +/// Takes a backend text into the semantic tree. The two trees share one storage +/// in a parse, so the text moves with its allocation and its charge. +pub(super) trait AdoptText: Storage { + fn adopt(&self, text: R::Text) -> Result; +} + +impl AdoptText for Native { + fn adopt(&self, text: String) -> Result { + Ok(text) + } +} + +#[cfg(feature = "xmldsig")] +impl<'w, W: structured_asn1::memory::Work> AdoptText> + for super::storage::Accounted<'w, W> +{ + fn adopt(&self, text: Self::Text) -> Result { + Ok(text) + } +} + +// Tests project a natively parsed tree into a limited account. +#[cfg(all(test, feature = "xmldsig"))] +impl AdoptText for super::storage::Accounted<'_, W> { + fn adopt(&self, text: String) -> Result { + self.text(&text) + } +} + +fn adopt_optional, R: Storage>( + storage: S, + text: Option, +) -> Result, ParseError> { + text.map(|text| storage.adopt(text)).transpose() +} + impl XmlBackendImplementation for XmloxideBackend { fn parse<'input>( input: &'input str, @@ -60,22 +96,22 @@ impl XmlBackendImplementation for XmloxideBackend { }, }, )?; - project(input, &parsed, options, preflight) + project(input, parsed, options, preflight) } } pub(super) fn project<'input>( input: &'input str, - parsed: &xmloxide::Document, + parsed: xmloxide::Document, options: ParsingOptions, preflight: &LexicalPreflight, ) -> Result, ParseError> { project_in(input, parsed, options, preflight, Native) } -pub(super) fn project_in<'input, S: Storage, P: Storage, R: NameStorage>( +pub(super) fn project_in<'input, S: AdoptText, P: Storage, R: NameStorage>( input: &'input str, - parsed: &xmloxide::Document, + parsed: xmloxide::Document, options: ParsingOptions, preflight: &LexicalPreflight<'_, P>, storage: S, @@ -83,20 +119,21 @@ pub(super) fn project_in<'input, S: Storage, P: Storage, R: NameStorage>( if options.nodes_limit == 0 { return Err(ParseError::NodesLimitReached); } + let parsed_root = parsed.root(); let mut projector = Projector { - source: parsed, target: TreeBuilder::new( input, parsed.node_count().min(options.nodes_limit as usize), storage, )?, + source: parsed, positions: preflight.positions(), nodes_limit: options.nodes_limit, }; let root = projector .target .push(None, NodeKind::Root, 0..input.len())?; - projector.project_children(parsed.root(), root)?; + projector.project_children(parsed_root, root)?; projector.positions.finish()?; projector.target.finish_subtree(root); projector.target.finish(preflight) @@ -118,8 +155,8 @@ pub(super) fn backend_options() -> xmloxide::parser::ParseOptions { .max_entity_expansions(crate::hard_limits::XML_ENTITY_EXPANSION_CEILING) } -struct Projector<'source, 'input, 'positions, S: Storage = Native, R: NameStorage = Native> { - source: &'source xmloxide::Document, +struct Projector<'input, 'positions, S: Storage = Native, R: NameStorage = Native> { + source: xmloxide::Document, target: TreeBuilder<'input, S>, positions: PositionCursor<'positions>, nodes_limit: u32, @@ -136,7 +173,7 @@ enum ProjectionFrame { Exit(NodeId), } -impl Projector<'_, '_, '_, S, R> { +impl, R: NameStorage> Projector<'_, '_, S, R> { fn project_children(&mut self, source: XmloxNodeId, parent: NodeId) -> Result<(), ParseError> { let storage = self.target.storage(); let mut stack = storage.values(0)?; @@ -152,8 +189,12 @@ impl Projector<'_, '_, '_, S, R> { inherited_range, range_actionable, } => { - let source_node = self.source.node(source); - match &source_node.kind { + // Each node is entered once, so its payload moves out. + let kind = core::mem::replace( + &mut self.source.node_mut(source).kind, + XmloxNodeKind::Document, + ); + match kind { XmloxNodeKind::Document | XmloxNodeKind::DocumentType { .. } => { self.push_children( &mut stack, @@ -210,6 +251,7 @@ impl Projector<'_, '_, '_, S, R> { )?; } else if let Some(value) = value { self.admit_text(parent)?; + let value = self.target.storage().adopt(value)?; self.target.append_text(parent, value, range, false)?; } } @@ -222,7 +264,7 @@ impl Projector<'_, '_, '_, S, R> { fn project_element( &mut self, - kind: &XmloxNodeKind, + kind: XmloxNodeKind, parent: NodeId, namespace_owner: Option, inherited_range: Option<&Range>, @@ -243,16 +285,16 @@ impl Projector<'_, '_, '_, S, R> { Some(range) => range.clone(), None => self.positions.take(SourceKind::Element)?, }; - let namespaces = self.project_namespaces(attributes, namespace_owner)?; + let namespaces = self.project_namespaces(&attributes, namespace_owner)?; let mut semantic_attributes = storage.values(attributes.len())?; - for attribute in &**attributes { + for attribute in attributes { storage.charge(1)?; - if namespace_declaration(attribute).is_none() { + if namespace_declaration(&attribute).is_none() { semantic_attributes.push(AttributeData { - name: storage.text(&attribute.name)?, - namespace: storage.optional_text(attribute.namespace.as_deref())?, - prefix: storage.optional_text(attribute.prefix.as_deref())?, - value: storage.text(&attribute.value)?, + name: storage.adopt(attribute.name)?, + namespace: adopt_optional(storage, attribute.namespace)?, + prefix: adopt_optional(storage, attribute.prefix)?, + value: storage.adopt(attribute.value)?, is_id: false, xml_id_error: None, })?; @@ -261,10 +303,9 @@ impl Projector<'_, '_, '_, S, R> { self.target.push_with_actionability( Some(parent), NodeKind::Element { - name: storage.text(name)?, - namespace: storage - .optional_text(namespace.as_deref().filter(|uri| !uri.is_empty()))?, - prefix: storage.optional_text(prefix.as_deref())?, + name: storage.adopt(name)?, + namespace: adopt_optional(storage, namespace.filter(|uri| !uri.is_empty()))?, + prefix: adopt_optional(storage, prefix)?, attributes: semantic_attributes, namespaces, }, @@ -309,7 +350,7 @@ impl Projector<'_, '_, '_, S, R> { &mut self, source: XmloxNodeId, parent: NodeId, - kind: &XmloxNodeKind, + kind: XmloxNodeKind, inherited_range: Option>, range_actionable: bool, ) -> Result<(), ParseError> { @@ -327,8 +368,12 @@ impl Projector<'_, '_, '_, S, R> { Some(range) => range, None => self.positions.take(SourceKind::Text)?, }; - self.target - .append_text(parent, content, range, range_actionable)?; + self.target.append_text( + parent, + storage.adopt(content)?, + range, + range_actionable, + )?; } XmloxNodeKind::CData { content } => { self.admit_text(parent)?; @@ -336,8 +381,12 @@ impl Projector<'_, '_, '_, S, R> { Some(range) => range, None => self.positions.take(SourceKind::CData)?, }; - self.target - .append_text(parent, content, range, range_actionable)?; + self.target.append_text( + parent, + storage.adopt(content)?, + range, + range_actionable, + )?; } XmloxNodeKind::Comment { content } => { self.admit_node()?; @@ -347,7 +396,7 @@ impl Projector<'_, '_, '_, S, R> { }; self.target.push_with_actionability( Some(parent), - NodeKind::Comment(storage.text(content)?), + NodeKind::Comment(storage.adopt(content)?), range, range_actionable, )?; @@ -364,8 +413,8 @@ impl Projector<'_, '_, '_, S, R> { self.target.push_with_actionability( Some(parent), NodeKind::PI { - target: storage.text(pi_target)?, - value: storage.optional_text(data.as_deref())?, + target: storage.adopt(pi_target)?, + value: adopt_optional(storage, data)?, }, range, range_actionable, From a42f306f0e01c7305e58ec7769a13529478e460c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 08:01:41 +0200 Subject: [PATCH 07/11] perf(c14n): canonicalize SignedInfo from its own subtree --- src/c14n/mod.rs | 112 +++++++++++++++++++++++++++++- src/c14n/serialize.rs | 58 ++++++++++------ src/xml/dom/preflight.rs | 4 +- src/xml/dom/storage/lexical.rs | 3 + src/xml/dom/tree.rs | 12 ++++ src/xml/dom/xmloxide.rs | 33 +++++++-- src/xml_input/lexical.rs | 45 +++++++++--- src/xml_input/lexical/storage.rs | 5 ++ src/xmldsig/verify.rs | 9 +-- src/xslt/model/lexical_storage.rs | 3 + 10 files changed, 238 insertions(+), 46 deletions(-) diff --git a/src/c14n/mod.rs b/src/c14n/mod.rs index 0e32adfb..dae50057 100644 --- a/src/c14n/mod.rs +++ b/src/c14n/mod.rs @@ -230,6 +230,11 @@ pub(crate) fn is_output_limit_error(error: &C14nError) -> bool { pub(crate) trait NodeVisibility { fn contains_node(&self, node: Node<'_, '_, D>) -> Result; + /// The element whose whole subtree, and nothing else, this set selects. + fn apex(&self) -> Option { + None + } + fn contains_attribute( &self, owner: Node<'_, '_, D>, @@ -290,6 +295,41 @@ impl NodeVisibility for ClosureVisibility<'_> { } } +/// One element's whole subtree, with every attribute and namespace node on it. +#[cfg(feature = "xmldsig")] +pub(crate) struct SubtreeVisibility<'a, 'input> { + pub(crate) apex: Node<'a, 'input>, +} + +#[cfg(feature = "xmldsig")] +impl NodeVisibility for SubtreeVisibility<'_, '_> { + fn contains_node(&self, node: Node<'_, '_>) -> Result { + Ok(self.apex.subtree_contains_id(node.id())) + } + + fn apex(&self) -> Option { + Some(self.apex.id()) + } + + fn contains_attribute( + &self, + owner: Node<'_, '_>, + _namespace: Option<&str>, + _local_name: &str, + ) -> Result { + self.contains_node(owner) + } + + fn contains_namespace( + &self, + owner: Node<'_, '_>, + _prefix: &str, + _uri: &str, + ) -> Result { + self.contains_node(owner) + } +} + /// Canonicalize an XML document or document subset. /// /// - `doc`: parsed backend-neutral semantic document (read-only DOM). @@ -320,7 +360,7 @@ pub fn canonicalize( /// Canonicalize through the closure visibility API while enforcing both the /// output ceiling and the caller's operation-wide XML Base work budget. -#[cfg(feature = "xmldsig")] +#[cfg(feature = "xmldsig-sign")] pub(crate) fn canonicalize_bounded_with_xml_base_budget( doc: &Document, node_set: Option<&dyn Fn(Node) -> bool>, @@ -778,6 +818,76 @@ pub fn canonicalize_document( mod tests { use super::*; + #[cfg(feature = "xmldsig")] + fn subtree_bytes( + document: &Document, + apex: Node<'_, '_>, + algorithm: &C14nAlgorithm, + ) -> Vec { + let mut output = Vec::new(); + canonicalize_visible_to_writer_bounded( + document, + Some(&SubtreeVisibility { apex }), + algorithm, + usize::MAX, + &xml_base::XmlBaseResolutionBudget::default(), + &mut output, + 0, + ) + .expect("subtree canonicalization"); + output + } + + #[cfg(feature = "xmldsig")] + #[test] + fn walking_from_the_apex_matches_filtering_the_whole_document() { + // SignedInfo starts at its own element. Ancestors still supply in-scope + // namespaces and inherited xml:* attributes, in every mode. + let xml = r#"textt"#; + let document = Document::parse(xml).expect("parse"); + let apex = document.root_element().first_element_child().expect("apex"); + let subtree: Vec<_> = apex.descendants().map(Node::id).collect(); + for mode in [ + C14nMode::Inclusive1_0, + C14nMode::Inclusive1_1, + C14nMode::Exclusive1_0, + ] { + for comments in [false, true] { + let algorithm = C14nAlgorithm::new(mode, comments); + let mut filtered = Vec::new(); + canonicalize( + &document, + Some(&|node| subtree.contains(&node.id())), + &algorithm, + &mut filtered, + ) + .expect("filtered canonicalization"); + assert_eq!(subtree_bytes(&document, apex, &algorithm), filtered); + } + } + } + + #[cfg(feature = "xmldsig")] + #[test] + fn relative_namespace_outside_the_apex_still_fails() { + // Canonical XML 1.0 §2.3 refuses the document, not just the subset, so + // starting at the apex must not skip a binding declared elsewhere. + let document = Document::parse("").expect("parse"); + let apex = document.root_element().first_element_child().expect("apex"); + let algorithm = C14nAlgorithm::new(C14nMode::Exclusive1_0, false); + let mut output = Vec::new(); + let result = canonicalize_visible_to_writer_bounded( + &document, + Some(&SubtreeVisibility { apex }), + &algorithm, + usize::MAX, + &xml_base::XmlBaseResolutionBudget::default(), + &mut output, + 0, + ); + assert!(matches!(result, Err(C14nError::RelativeNamespace))); + } + #[test] fn from_uri_roundtrip() { let uris = [ diff --git a/src/c14n/serialize.rs b/src/c14n/serialize.rs index 89d11f4a..1db00b8d 100644 --- a/src/c14n/serialize.rs +++ b/src/c14n/serialize.rs @@ -361,6 +361,16 @@ pub(super) fn serialize_with_scratch Result<(Option, TrackedPositions), C14nError> { + // Canonical XML 1.0 §2.3 requires operation failure on documents containing + // relative namespace URIs, including outside the selected subset. Elements + // share binding lists, so each list is checked once for the whole document. + // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#DataModel + for uri in doc.namespace_uris() { + scratch.charge(uri.len().saturating_mul(2).saturating_add(1))?; + if !uri.is_empty() && !super::xml_base::has_scheme(uri) { + return Err(C14nError::RelativeNamespace); + } + } let root = doc.root(); let max_len = match options.max_output_bytes { Some(limit) => initial_offset @@ -380,15 +390,32 @@ pub(super) fn serialize_with_scratch( match child.node_type() { NodeType::Element => { - // Canonical XML 1.0 §2.3 requires operation failure on - // documents containing relative namespace URIs. Apply before - // visibility filtering, including Exclusive C14N subsets. - // https://www.w3.org/TR/2001/REC-xml-c14n-20010315#DataModel - for namespace in child.namespaces() { - let uri = namespace.uri(); - output - .scratch - .charge(uri.len().saturating_mul(2).saturating_add(1))?; - if !uri.is_empty() && !super::xml_base::has_scheme(uri) { - return Err(C14nError::RelativeNamespace); - } - } if in_set { serialize_element( child, diff --git a/src/xml/dom/preflight.rs b/src/xml/dom/preflight.rs index b4454c4b..fab6ea91 100644 --- a/src/xml/dom/preflight.rs +++ b/src/xml/dom/preflight.rs @@ -161,10 +161,11 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { &mut nodes, SourceNode { kind: SourceKind::Element, - range: element.range.clone(), + range: element.range, }, )?; elements.push(index)?; + reader.recycle(element.attributes); } Event::Empty(element) => { enforce_depth(elements.len() + 1)?; @@ -175,6 +176,7 @@ impl<'input, S: Storage> LexicalPreflight<'input, S> { range: element.range, }, )?; + reader.recycle(element.attributes); } Event::End { range, .. } => { if let Some(index) = elements.pop() { diff --git a/src/xml/dom/storage/lexical.rs b/src/xml/dom/storage/lexical.rs index bf857837..07b3fcbd 100644 --- a/src/xml/dom/storage/lexical.rs +++ b/src/xml/dom/storage/lexical.rs @@ -47,6 +47,9 @@ impl<'a, W: Work> AttributeBuffer<'a, Accounted<'_, W>> for OwnedVec) -> Result<(), MemoryError> { self.try_push(attribute) } + fn clear(&mut self) { + OwnedVec::clear(self); + } fn sort( &mut self, compare: impl FnMut(&Attribute<'a>, &Attribute<'a>) -> Result, diff --git a/src/xml/dom/tree.rs b/src/xml/dom/tree.rs index 59008693..83efab23 100644 --- a/src/xml/dom/tree.rs +++ b/src/xml/dom/tree.rs @@ -562,6 +562,13 @@ impl<'input, S: Storage> Document<'input, S> { &self.namespace_sets[set.index()] } + /// Every namespace URI any element has in scope, repeats included. + pub(crate) fn namespace_uris(&self) -> impl Iterator { + self.namespace_sets + .iter() + .flat_map(|set| set.iter().map(|namespace| namespace.uri.as_ref())) + } + /// The child of `parent` whose subtree ends at `end`. That subtree's last /// node is `end - 1`, so its ancestor chain reaches the child within the /// document's depth. @@ -765,6 +772,11 @@ impl<'a, 'input, S: Storage> Node<'a, 'input, S> { ids: self.id.0..self.data().subtree_end, } } + /// Whether `other` is this node or one of its descendants. + #[cfg(feature = "xmldsig")] + pub(crate) fn subtree_contains_id(self, other: NodeId) -> bool { + (self.id.0..self.data().subtree_end).contains(&other.0) + } /// Document-order `XPath` index of this tree node. #[cfg(feature = "xmldsig")] pub(crate) fn xpath_index(self) -> u32 { diff --git a/src/xml/dom/xmloxide.rs b/src/xml/dom/xmloxide.rs index fb4b32f1..e3713271 100644 --- a/src/xml/dom/xmloxide.rs +++ b/src/xml/dom/xmloxide.rs @@ -339,8 +339,13 @@ impl, R: NameStorage> Projector<'_, '_, S, R> { })?; } for attribute in attributes { - if let Some(prefix) = namespace_declaration(attribute) { - set_namespace(&mut namespaces, prefix, &attribute.value, storage)?; + if let Some(declared) = namespace_declaration(attribute) { + set_namespace( + &mut namespaces, + declared.prefix(), + &attribute.value, + storage, + )?; } } self.target.add_namespace_set(namespaces) @@ -466,15 +471,29 @@ impl, R: NameStorage> Projector<'_, '_, S, R> { } } -/// The bound prefix when `attribute` declares a namespace: `Some(None)` is the -/// default namespace. +/// What a namespace declaration attribute binds. +#[derive(Clone, Copy)] +enum Declared<'a> { + Default, + Prefix(&'a str), +} + +impl<'a> Declared<'a> { + fn prefix(self) -> Option<&'a str> { + match self { + Self::Default => None, + Self::Prefix(prefix) => Some(prefix), + } + } +} + fn namespace_declaration( attribute: &xmloxide::tree::Attribute, -) -> Option> { +) -> Option> { if attribute.prefix.as_deref() == Some("xmlns") { - Some(Some(&attribute.name)) + Some(Declared::Prefix(&attribute.name)) } else if attribute.prefix.is_none() && attribute.name.as_ref() == "xmlns" { - Some(None) + Some(Declared::Default) } else { None } diff --git a/src/xml_input/lexical.rs b/src/xml_input/lexical.rs index d7e512dd..7e30e55e 100644 --- a/src/xml_input/lexical.rs +++ b/src/xml_input/lexical.rs @@ -216,6 +216,7 @@ pub struct Scanner<'a, S: Storage = Native> { pending_text: Option>, pending_start: Option>, dtd_start: Option<(usize, &'a str)>, + spare_attributes: Option>, storage: S, admitted: bool, } @@ -309,11 +310,18 @@ impl<'a, S: Storage> Scanner<'a, S> { pending_text: None, pending_start: None, dtd_start: None, + spare_attributes: None, storage, admitted: false, } } + /// Return a finished tag's attribute buffer for the next start tag. + pub fn recycle(&mut self, mut attributes: S::Attributes<'a>) { + attributes.clear(); + self.spare_attributes = Some(attributes); + } + /// Return the next event, or `None` at end of input. /// /// # Errors @@ -461,7 +469,10 @@ impl<'a, S: Storage> Scanner<'a, S> { prefix: prefix.as_str(), local: local.as_str(), }, - attributes: self.storage.attributes(), + attributes: self + .spare_attributes + .take() + .unwrap_or_else(|| self.storage.attributes()), start: span.range().start, }); Ok(None) @@ -576,16 +587,28 @@ fn validate_qualified_lexeme(source: &str) -> Result<(), Error> { .strip_prefix("') - }) - .next() - .unwrap_or_default(); - let mut parts = name.split(':'); - let first = parts.next().unwrap_or_default(); - let second = parts.next(); - if first.is_empty() || second.is_some_and(str::is_empty) || parts.next().is_some() { + // Every delimiter is ASCII and UTF-8 continuation bytes never are, so bytes + // decide this without decoding characters. + let mut colon = None; + let mut length = source.len(); + for (index, &byte) in source.as_bytes().iter().enumerate() { + match byte { + b':' if colon.is_some() => { + return Err(Error::Static("invalid qualified XML name")); + } + b':' => colon = Some(index), + b'=' | b'/' | b'>' => { + length = index; + break; + } + byte if byte.is_ascii_whitespace() => { + length = index; + break; + } + _ => {} + } + } + if length == 0 || colon.is_some_and(|colon| colon == 0 || colon + 1 == length) { return Err(Error::Static("invalid qualified XML name")); } Ok(()) diff --git a/src/xml_input/lexical/storage.rs b/src/xml_input/lexical/storage.rs index 4c9b5e15..bc91a38c 100644 --- a/src/xml_input/lexical/storage.rs +++ b/src/xml_input/lexical/storage.rs @@ -29,6 +29,8 @@ pub trait Storage: Copy { pub trait AttributeBuffer<'a, S: Storage>: Deref]> + DerefMut { /// Append one borrowed attribute, admitting actual growth first. fn push(&mut self, attribute: Attribute<'a>) -> Result<(), S::Refusal>; + /// Forget the attributes, keeping the capacity. + fn clear(&mut self); /// In-place ordering without a second allocation. fn sort( &mut self, @@ -72,6 +74,9 @@ impl<'a, S: Storage> AttributeBuffer<'a, S> for Vec, &Attribute<'a>) -> Result, diff --git a/src/xmldsig/verify.rs b/src/xmldsig/verify.rs index cb6cac12..4154b7f0 100644 --- a/src/xmldsig/verify.rs +++ b/src/xmldsig/verify.rs @@ -25,7 +25,6 @@ use alloc::{ use base64::Engine; use core::cell::{Cell, RefCell}; -use crate::c14n::canonicalize_bounded_with_xml_base_budget; use crate::document::{ DocumentParseSettings, DocumentView, NodeIdentity, XmlDocument, XmlDocumentError, }; @@ -2688,20 +2687,22 @@ fn canonicalize_verification_signed_info( >, ) -> Result, DsigError> { let doc = signed_info_node.document(); - let signed_info_subtree: Set<_> = signed_info_node.descendants().map(Node::id).collect(); let mut canonical_signed_info = Vec::new(); let signed_info_limit = operation .budgets() .canonicalized .remaining() .min(operation.budgets().transforms.remaining_c14n_output()); - canonicalize_bounded_with_xml_base_budget( + crate::c14n::canonicalize_visible_to_writer_bounded( doc, - Some(&|node| signed_info_subtree.contains(&node.id())), + Some(&crate::c14n::SubtreeVisibility { + apex: signed_info_node, + }), method, signed_info_limit, operation.budgets().transforms.xml_base_resolution(), &mut canonical_signed_info, + 0, ) .map_err(|error| { if let Some(violation) = map_c14n_resource_policy_violation( diff --git a/src/xslt/model/lexical_storage.rs b/src/xslt/model/lexical_storage.rs index a5766640..e5c8ccee 100644 --- a/src/xslt/model/lexical_storage.rs +++ b/src/xslt/model/lexical_storage.rs @@ -100,6 +100,9 @@ impl Drop for Attributes<'_, '_> { } impl<'a, 's> AttributeBuffer<'a, Storage<'s>> for Attributes<'a, 's> { + fn clear(&mut self) { + self.values.clear(); + } fn push(&mut self, attribute: Attribute<'a>) -> Result<()> { if self.values.len() == self.values.capacity() { let capacity = self From 0ade9cdb50ad3a3b8102becfc1c215ddcfbc6749 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 08:28:01 +0200 Subject: [PATCH 08/11] perf(xml): scan names once and share the SignedInfo subtree walk --- src/c14n/mod.rs | 38 +++-------------------------- src/parser_xmloxide/parser/input.rs | 21 ++++++++++++++-- src/xmldsig/builder.rs | 15 ++++++------ src/xmldsig/sign.rs | 20 +++++++-------- 4 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/c14n/mod.rs b/src/c14n/mod.rs index dae50057..bdf66b44 100644 --- a/src/c14n/mod.rs +++ b/src/c14n/mod.rs @@ -358,51 +358,21 @@ pub fn canonicalize( ) } -/// Canonicalize through the closure visibility API while enforcing both the -/// output ceiling and the caller's operation-wide XML Base work budget. -#[cfg(feature = "xmldsig-sign")] -pub(crate) fn canonicalize_bounded_with_xml_base_budget( - doc: &Document, - node_set: Option<&dyn Fn(Node) -> bool>, - algo: &C14nAlgorithm, - max_output_bytes: usize, - xml_base_resolution: &xml_base::XmlBaseResolutionBudget, - output: &mut Vec, -) -> Result<(), C14nError> { - let visibility = node_set.map(|predicate| ClosureVisibility { predicate }); - let initial_offset = output.len(); - canonicalize_visible_to_writer_bounded( - doc, - visibility - .as_ref() - .map(|visibility| visibility as &dyn NodeVisibility), - algo, - max_output_bytes, - xml_base_resolution, - output, - initial_offset, - )?; - Ok(()) -} - -/// [`canonicalize_bounded_with_xml_base_budget`] that also reports where each -/// tracked element's start tag begins in `output`. +/// Bounded canonicalization that also reports where each tracked element's +/// start tag begins in `output`. #[cfg(feature = "xmldsig-sign")] pub(crate) fn canonicalize_bounded_with_xml_base_budget_and_positions( doc: &Document, - node_set: &dyn Fn(Node) -> bool, + visibility: &dyn NodeVisibility, algo: &C14nAlgorithm, tracked_elements: &[NodeId], max_output_bytes: usize, xml_base_resolution: &xml_base::XmlBaseResolutionBudget, output: &mut Vec, ) -> Result, C14nError> { - let visibility = ClosureVisibility { - predicate: node_set, - }; canonicalize_with_visibility_and_positions_bounded_with_xml_base_budget( doc, - Some(&visibility), + Some(visibility), algo, tracked_elements, max_output_bytes, diff --git a/src/parser_xmloxide/parser/input.rs b/src/parser_xmloxide/parser/input.rs index e5284987..87727965 100644 --- a/src/parser_xmloxide/parser/input.rs +++ b/src/parser_xmloxide/parser/input.rs @@ -182,9 +182,22 @@ fn is_ascii_name_start(b: u8) -> bool { /// /// Covers the ASCII subset of XML 1.0 §2.3 `[4a]`: `[A-Za-z0-9_:.-]`. fn is_ascii_name_char(b: u8) -> bool { - b.is_ascii_alphanumeric() || b == b'_' || b == b':' || b == b'-' || b == b'.' + ASCII_NAME_CHARS[b as usize] } +/// The table behind [`is_ascii_name_char`]: names are the most scanned bytes in +/// a document, and a lookup replaces a chain of range checks per byte. +const ASCII_NAME_CHARS: [bool; 256] = { + let mut table = [false; 256]; + let mut value = 0_u8; + while value < 0x80 { + table[value as usize] = + value.is_ascii_alphanumeric() || matches!(value, b'_' | b':' | b'-' | b'.'); + value += 1; + } + table +}; + /// Matches one of the five XML builtin entity references at the byte level. /// /// Given bytes starting after `&`, returns `(replacement_char, bytes_to_skip)` @@ -1008,7 +1021,11 @@ impl<'a, S: NameStorage> ParserInput<'a, S> { let count = self.scan_bytes(self.input.len() - self.pos, 1, |bytes, i| { !is_ascii_name_char(bytes[i]) })?; - self.advance_counting_lines(count)?; + // Name characters exclude newlines, so no second scan looks for one. + self.storage.charge(count * 2)?; + self.pos += count; + self.column += + u32::try_from(count).expect("XML source windows fit the admitted byte ceiling"); // Check if we stopped at a non-ASCII byte (need slow path) if self.pos >= self.input.len() || self.input[self.pos] < 0x80 { let len = self.pos - start; diff --git a/src/xmldsig/builder.rs b/src/xmldsig/builder.rs index 5c09fcdc..4ee8d15e 100644 --- a/src/xmldsig/builder.rs +++ b/src/xmldsig/builder.rs @@ -1,12 +1,14 @@ //! Builders for deterministic `XMLDSig` signature templates. use crate::xml_input as xml_sec_xml_input; -use std::{collections::HashSet, io::Write}; +use std::io::Write; use base64::Engine; use xml_sec_xml_input::lexical::Writer; -use crate::c14n::{C14nAlgorithm, C14nMode, canonicalize_bounded_with_xml_base_budget}; +use crate::c14n::{ + C14nAlgorithm, C14nMode, SubtreeVisibility, canonicalize_visible_to_writer_bounded, +}; use crate::policy::{PolicyViolation, SigningPolicy}; use crate::xml::{is_xml_1_0_character, is_xml_ncname}; @@ -459,18 +461,15 @@ impl SignatureBuilder { .children() .find(|node| node.has_tag_name((XMLDSIG_NS, "SignedInfo"))) .ok_or(SignatureBuilderError::MissingGeneratedSignedInfo)?; - let signed_info_subtree: HashSet<_> = signed_info - .descendants() - .map(crate::xml::dom::Node::id) - .collect(); let mut canonical_signed_info = Vec::new(); - canonicalize_bounded_with_xml_base_budget( + canonicalize_visible_to_writer_bounded( &document, - Some(&|node| signed_info_subtree.contains(&node.id())), + Some(&SubtreeVisibility { apex: signed_info }), &self.c14n_method, budget.remaining_c14n_output(), budget.xml_base_resolution(), &mut canonical_signed_info, + 0, ) .map_err(|error| { map_c14n_resource_policy_violation( diff --git a/src/xmldsig/sign.rs b/src/xmldsig/sign.rs index 81101de8..b8362ba9 100644 --- a/src/xmldsig/sign.rs +++ b/src/xmldsig/sign.rs @@ -34,7 +34,6 @@ use std::{ }; use zeroize::Zeroizing; -use crate::c14n::canonicalize_bounded_with_xml_base_budget; use crate::operation::{ OperationExecutionContext, OperationNodeId, OperationNodeKind, OperationPlanError, OperationStage, @@ -3890,24 +3889,23 @@ fn canonicalize_signed_info( } .into()); } - let signed_info_subtree: HashSet<_> = signed_info_node - .descendants() - .map(|node: Node<'_, '_>| node.id()) - .collect(); let mut canonical_signed_info = Vec::new(); let canonicalized = if pending_digests.is_empty() { - canonicalize_bounded_with_xml_base_budget( + crate::c14n::canonicalize_visible_to_writer_bounded( doc, - Some(&|node| signed_info_subtree.contains(&node.id())), + Some(&crate::c14n::SubtreeVisibility { + apex: signed_info_node, + }), &signed_info.c14n_method, budgets.transforms.remaining_c14n_output(), budgets.transforms.xml_base_resolution(), &mut canonical_signed_info, + 0, ) } else { canonicalize_with_pending_digests( doc, - &signed_info_subtree, + signed_info_node, &signed_info.c14n_method, budgets, &pending_digests, @@ -3945,7 +3943,7 @@ const SECOND_EDITION_GENERATION_C14N_URI: &str = "http://www.w3.org/2006/12/xml- /// fn canonicalize_with_pending_digests( document: &Document<'_>, - signed_info_subtree: &HashSet, + signed_info: Node<'_, '_>, algorithm: &crate::c14n::C14nAlgorithm, budgets: &SigningOperationBudgets, pending_digests: &[(NodeId, &str)], @@ -3954,7 +3952,7 @@ fn canonicalize_with_pending_digests( let digests = pending_digests .iter() .copied() - .filter(|(node, _)| signed_info_subtree.contains(node)) + .filter(|(node, _)| signed_info.subtree_contains_id(*node)) .collect::>(); let tracked = digests.keys().copied().collect::>(); let inserted = digests.values().map(|digest| digest.len()).sum::(); @@ -3968,7 +3966,7 @@ fn canonicalize_with_pending_digests( let mut unfilled = Vec::new(); let mut positions = crate::c14n::canonicalize_bounded_with_xml_base_budget_and_positions( document, - &|node| signed_info_subtree.contains(&node.id()), + &crate::c14n::SubtreeVisibility { apex: signed_info }, algorithm, &tracked, allowance, From 1567044b038d198f85cf3d54cd774402a0de37d1 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 31 Oct 2026 08:47:46 +0200 Subject: [PATCH 09/11] build(xml): refresh the maintained xmloxide delta --- scripts/patches/xmloxide-0.5.0.patch | 256 +++++++++++++++------------ 1 file changed, 142 insertions(+), 114 deletions(-) diff --git a/scripts/patches/xmloxide-0.5.0.patch b/scripts/patches/xmloxide-0.5.0.patch index 92d51b4c..93c44949 100644 --- a/scripts/patches/xmloxide-0.5.0.patch +++ b/scripts/patches/xmloxide-0.5.0.patch @@ -201,7 +201,31 @@ // ------------------------------------------------------------------------- // Security defaults -@@ -177,26 +266,6 @@ +@@ -93,9 +182,22 @@ + /// + /// Covers the ASCII subset of XML 1.0 §2.3 `[4a]`: `[A-Za-z0-9_:.-]`. + fn is_ascii_name_char(b: u8) -> bool { +- b.is_ascii_alphanumeric() || b == b'_' || b == b':' || b == b'-' || b == b'.' ++ ASCII_NAME_CHARS[b as usize] + } + ++/// The table behind [`is_ascii_name_char`]: names are the most scanned bytes in ++/// a document, and a lookup replaces a chain of range checks per byte. ++const ASCII_NAME_CHARS: [bool; 256] = { ++ let mut table = [false; 256]; ++ let mut value = 0_u8; ++ while value < 0x80 { ++ table[value as usize] = ++ value.is_ascii_alphanumeric() || matches!(value, b'_' | b':' | b'-' | b'.'); ++ value += 1; ++ } ++ table ++}; ++ + /// Matches one of the five XML builtin entity references at the byte level. + /// + /// Given bytes starting after `&`, returns `(replacement_char, bytes_to_skip)` +@@ -177,26 +279,6 @@ } } @@ -228,7 +252,7 @@ /// Validates that a name is a legal `QName` per Namespaces in XML 1.0 §4. /// /// A `QName` has at most one colon, and neither prefix nor local part may be -@@ -228,22 +297,6 @@ +@@ -228,22 +310,6 @@ ) } @@ -251,7 +275,7 @@ // ------------------------------------------------------------------------- // Position checkpointing (for backtracking) // ------------------------------------------------------------------------- -@@ -266,22 +319,19 @@ +@@ -266,22 +332,19 @@ // ParserInput // ------------------------------------------------------------------------- @@ -281,7 +305,7 @@ /// The input bytes (must be valid UTF-8). input: &'a [u8], -@@ -313,16 +363,11 @@ +@@ -313,16 +376,11 @@ recover: bool, /// Accumulated diagnostics (warnings and recoverable errors). @@ -300,7 +324,7 @@ /// Whether the DTD internal subset contained parameter entity references. /// Per XML 1.0 §4.1 WFC: Entity Declared, undeclared entity references -@@ -336,10 +381,6 @@ +@@ -336,10 +394,6 @@ /// external DTD subset that was not read. pub(crate) has_external_dtd: bool, @@ -311,7 +335,7 @@ /// Optional callback for resolving external entities. entity_resolver: Option, } -@@ -347,7 +388,28 @@ +@@ -347,7 +401,28 @@ impl<'a> ParserInput<'a> { /// Creates a new `ParserInput` from a UTF-8 string with default limits. pub fn new(input: &'a str) -> Self { @@ -341,7 +365,7 @@ input: input.as_bytes(), pos: 0, line: 1, -@@ -358,19 +420,12 @@ +@@ -358,19 +433,12 @@ entity_expansions: 0, max_entity_expansions: DEFAULT_MAX_ENTITY_EXPANSIONS, recover: false, @@ -364,7 +388,7 @@ } /// Sets the maximum nesting depth. -@@ -398,13 +453,32 @@ +@@ -398,13 +466,32 @@ self.recover } @@ -399,7 +423,7 @@ "maximum nesting depth exceeded ({})", self.max_depth ))); -@@ -458,7 +532,7 @@ +@@ -458,7 +545,7 @@ /// Returns a slice of the raw input bytes from `start` to `end`. #[allow(dead_code)] #[inline] @@ -408,7 +432,7 @@ &self.input[start..end] } -@@ -528,7 +602,7 @@ +@@ -528,7 +615,7 @@ if remaining.len() < len { return None; } @@ -417,7 +441,7 @@ .ok() .and_then(|s| s.chars().next()) } -@@ -537,9 +611,10 @@ +@@ -537,9 +624,10 @@ /// Advances the position by `count` bytes, updating line/column. #[inline] @@ -429,7 +453,7 @@ if self.pos < self.input.len() { if self.input[self.pos] == b'\n' { self.line += 1; -@@ -549,15 +624,16 @@ +@@ -549,15 +637,16 @@ } self.pos += 1; } @@ -449,7 +473,7 @@ if ch == '\n' { self.line += 1; self.column = 1; -@@ -565,23 +641,25 @@ +@@ -565,23 +654,25 @@ self.column += 1; } self.pos += len; @@ -478,7 +502,7 @@ if self.pos >= self.input.len() { return Err(self.fatal("unexpected end of input")); } -@@ -608,11 +686,11 @@ +@@ -608,11 +699,11 @@ if self.recover { self.push_diagnostic( ErrorSeverity::Error, @@ -493,7 +517,7 @@ ); } } -@@ -623,16 +701,16 @@ +@@ -623,16 +714,16 @@ let ch = self .peek_char() .ok_or_else(|| self.fatal("unexpected end of input"))?; @@ -514,7 +538,7 @@ } } Ok(ch) -@@ -640,31 +718,46 @@ +@@ -640,31 +731,46 @@ // -- Bulk scanning -- @@ -575,7 +599,7 @@ } /// Scans forward to find the next attribute value delimiter: the given -@@ -672,22 +765,16 @@ +@@ -672,22 +778,16 @@ /// Returns the number of safe bytes before the delimiter. /// Avoids the 256-byte lookup table of `scan_until_any`. #[inline] @@ -603,7 +627,7 @@ } /// Scans forward to find the next occurrence of any of the given marker -@@ -696,63 +783,72 @@ +@@ -696,63 +796,72 @@ /// Uses a 256-byte lookup table for O(1) per-byte matching instead of /// linear `contains()` on the marker slice. #[allow(dead_code)] @@ -703,7 +727,7 @@ let end = self.pos + count; let slice = &self.input[self.pos..end]; // Fast path: if no newlines in the slice, just bump column -@@ -766,22 +862,21 @@ +@@ -766,22 +875,21 @@ } } } else { @@ -731,7 +755,7 @@ "expected '{}', found '{}'", expected as char, b as char ))); -@@ -791,12 +886,13 @@ +@@ -791,12 +899,13 @@ /// Consumes bytes and asserts they match the `expected` sequence. #[inline] @@ -747,7 +771,7 @@ } else { // Fall back to per-byte for precise error reporting for &b in expected { -@@ -828,51 +924,30 @@ +@@ -828,51 +937,30 @@ /// Skips whitespace characters. Returns `true` if any were consumed. #[inline] @@ -812,7 +836,7 @@ return Err(self.fatal("whitespace required")); } Ok(()) -@@ -880,23 +955,18 @@ +@@ -880,23 +968,18 @@ // -- Take while -- @@ -845,7 +869,7 @@ } // -- Name parsing (XML 1.0 §2.3) -- -@@ -910,7 +980,16 @@ +@@ -910,7 +993,16 @@ /// Uses an ASCII fast path that scans name characters as bytes, /// avoiding per-character UTF-8 decoding for the common case. #[inline] @@ -863,7 +887,7 @@ let start = self.pos; if self.pos >= self.input.len() { return Err(self.fatal("expected name, found end of input")); -@@ -919,26 +998,31 @@ +@@ -919,26 +1011,35 @@ let first = self.input[self.pos]; // ASCII fast path: most XML names are pure ASCII @@ -882,7 +906,11 @@ + let count = self.scan_bytes(self.input.len() - self.pos, 1, |bytes, i| { + !is_ascii_name_char(bytes[i]) + })?; -+ self.advance_counting_lines(count)?; ++ // Name characters exclude newlines, so no second scan looks for one. ++ self.storage.charge(count * 2)?; ++ self.pos += count; ++ self.column += ++ u32::try_from(count).expect("XML source windows fit the admitted byte ceiling"); // Check if we stopped at a non-ASCII byte (need slow path) if self.pos >= self.input.len() || self.input[self.pos] < 0x80 { let len = self.pos - start; @@ -903,7 +931,7 @@ } // Fall through: hit a non-ASCII continuation byte, continue // with the char-by-char path below. -@@ -948,16 +1032,22 @@ +@@ -948,16 +1049,22 @@ let ch = self .peek_char() .ok_or_else(|| self.fatal("expected name"))?; @@ -931,7 +959,7 @@ } else { break; } -@@ -965,90 +1055,32 @@ +@@ -965,90 +1072,32 @@ let len = self.pos - start; if len > self.max_name_length { @@ -1033,7 +1061,7 @@ } /// Parses a name and checks whether it matches the given prefix + local -@@ -1060,111 +1092,46 @@ +@@ -1060,111 +1109,46 @@ &mut self, prefix: Option<&str>, local: &str, @@ -1165,7 +1193,7 @@ /// Parses an entity or character reference (`&...;`). /// /// Handles the five built-in XML entities (`amp`, `lt`, `gt`, `apos`, -@@ -1175,8 +1142,8 @@ +@@ -1175,8 +1159,8 @@ /// Increments the entity expansion counter and returns an error if the /// limit is exceeded. #[cfg(test)] @@ -1176,7 +1204,7 @@ self.parse_reference_into(&mut buf)?; Ok(buf) } -@@ -1190,34 +1157,47 @@ +@@ -1190,34 +1174,47 @@ /// /// Returns the resolved text as a `&str` slice of `buf` (the portion /// that was appended), which callers can use for validation. @@ -1234,7 +1262,7 @@ if dec.is_empty() { return Err(self.fatal("empty decimal character reference")); } -@@ -1226,68 +1206,191 @@ +@@ -1226,68 +1223,191 @@ }; self.expect_byte(b';')?; @@ -1475,7 +1503,7 @@ } } -@@ -1295,196 +1398,128 @@ +@@ -1295,196 +1415,128 @@ /// /// Per XML 1.0 §4.4, when an entity's replacement text is included, /// character references and entity references within it are resolved. @@ -1620,11 +1648,23 @@ - "amp" => { - result.push('&'); - continue; +- } +- "lt" => { +- result.push('<'); +- continue; +- } +- "gt" => { +- result.push('>'); +- continue; +- } +- "apos" => { +- result.push('\''); +- continue; + if i >= bytes.len() { + return Err(self.fatal("incomplete entity reference in entity value")); } -- "lt" => { -- result.push('<'); +- "quot" => { +- result.push('"'); + let name = core::str::from_utf8(&bytes[start..i]) + .map_err(|_| self.fatal("invalid UTF-8 in entity name"))?; + i += 1; // skip ';' @@ -1635,18 +1675,6 @@ + if self.append_builtin_replacement(name, result)? { continue; } -- "gt" => { -- result.push('>'); -- continue; -- } -- "apos" => { -- result.push('\''); -- continue; -- } -- "quot" => { -- result.push('"'); -- continue; -- } - _ => {} - } @@ -1774,7 +1802,7 @@ // -- Attribute value parsing (XML 1.0 §3.3.3) -- /// Parses a quoted attribute value with entity resolution and -@@ -1493,19 +1528,23 @@ +@@ -1493,19 +1545,23 @@ /// Uses bulk scanning to find the next `&`, `<`, or quote character, /// then extracts safe chunks with a single `push_str()` instead of /// processing character by character. @@ -1802,7 +1830,7 @@ .map_err(|_| self.fatal("invalid UTF-8 in attribute value"))?; // Fast byte-level pre-check: only validate when bytes suggest // possible invalid chars (0x7F or U+FFFE/U+FFFF sequences). -@@ -1516,30 +1555,18 @@ +@@ -1516,30 +1572,18 @@ if self.recover { self.push_diagnostic( ErrorSeverity::Error, @@ -1840,7 +1868,7 @@ } if self.at_end() { -@@ -1548,57 +1575,45 @@ +@@ -1548,57 +1592,45 @@ let b = self.input[self.pos]; if b == quote { @@ -1913,7 +1941,7 @@ self.expect_byte(quote)?; Ok(value) } -@@ -1606,21 +1621,44 @@ +@@ -1606,21 +1638,44 @@ // -- Error helpers -- /// Creates a fatal `ParseError` at the current location. @@ -1966,7 +1994,7 @@ } } -@@ -1630,100 +1668,111 @@ +@@ -1630,100 +1685,111 @@ /// Manages namespace scope for XML parsers. /// @@ -2136,7 +2164,7 @@ } } -@@ -1736,54 +1785,56 @@ +@@ -1736,54 +1802,56 @@ /// The opening `