Skip to content

Bound NamespaceResolver nesting depth (fixes #977)#979

Merged
dralley merged 3 commits into
tafia:masterfrom
scadastrangelove:fix/namespace-nesting-depth-overflow
Jul 20, 2026
Merged

Bound NamespaceResolver nesting depth (fixes #977)#979
dralley merged 3 commits into
tafia:masterfrom
scadastrangelove:fix/namespace-nesting-depth-overflow

Conversation

@scadastrangelove

Copy link
Copy Markdown
Contributor

Fixes #977.

NamespaceResolver::push incremented its u16 nesting_level with an unguarded += 1. A document nested past u16::MAX overflowed it — a panic under overflow-checks builds, or a silent wrap that corrupts namespace-scope truncation in a stock release build (bindings from closed scopes leak; in-scope bindings vanish). The sibling pop already saturates (saturating_sub); only the increment was unguarded. Reachable from the public NsReader::read_resolved_event* on ~490 KB of deeply nested XML. Still present on master; orthogonal to #970/#972 (which capped declarations per element — this is the depth counter).

Fix

Return a new NamespaceError::TooDeeplyNested at the u16 boundary, matching the existing per-element TooManyDeclarations guard:

self.nesting_level = self.nesting_level
    .checked_add(1)
    .ok_or(NamespaceError::TooDeeplyNested(u16::MAX as usize))?;

A saturating_add (mirroring pop) was considered but is insufficient — it removes the panic but not the scope corruption: at saturation all deep levels collapse to u16::MAX and set_level's rposition(|n| n.level <= level) truncation still over-truncates, so the namespace misresolution persists (verified). The clean error rejects the pathological document instead.

Validation

Alternative considered: widen nesting_level to u32/usize (overflow becomes practically unreachable). I chose the explicit cap + error to mirror max_declarations_per_element and keep the depth bound intentional on untrusted input — happy to switch if you'd prefer no new variant.

Found by the rust-in-peace security pipeline.

`NamespaceResolver::push` incremented a `u16` `nesting_level` with an
unguarded `+= 1`; a document nested past `u16::MAX` overflowed it — a
panic under `overflow-checks`, or a silent wrap that corrupts
namespace-scope truncation in release (bindings from closed scopes leak /
in-scope bindings vanish). `pop` already saturates; the increment did not.

Return a new `NamespaceError::TooDeeplyNested` at the `u16` boundary
instead, matching the existing per-element `TooManyDeclarations` guard. A
`saturating_add` would stop the panic but not the scope corruption (at
saturation deep levels collapse and `set_level` still over-truncates).

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@codecov-commenter

codecov-commenter commented Jul 19, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.91%. Comparing base (e00ae5c) to head (2fdf1aa).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
src/name.rs 80.00% 3 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #979      +/-   ##
==========================================
- Coverage   57.31%   56.91%   -0.40%     
==========================================
  Files          46       47       +1     
  Lines       18197    18521     +324     
==========================================
+ Hits        10429    10541     +112     
- Misses       7768     7980     +212     
Flag Coverage Δ
unittests 56.91% <80.00%> (-0.40%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mingun Mingun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one small pit, and need to fix formatting.

Comment thread Changelog.md Outdated

[#969]: https://github.com/tafia/quick-xml/issues/969
[#970]: https://github.com/tafia/quick-xml/issues/970
[#977]: https://github.com/tafia/quick-xml/issues/977

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be on line 30, in the Unreleased section

Per review: the reference link belonged with its Unreleased entry, not
under the already-released 0.41.0 section.
@dralley

dralley commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

@scadastrangelove Please run cargo fmt, otherwise looks good

@scadastrangelove

Copy link
Copy Markdown
Contributor Author

Done — pushed cargo fmt.

@dralley
dralley merged commit 56ae43f into tafia:master Jul 20, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug namespaces Issues related to namespaces support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unbounded u16 depth counter in NamespaceResolver::push → panic / namespace-scope corruption on deeply nested XML

4 participants