Add recursion-depth limit to serde Deserializer - #982
Conversation
The serde `Deserializer` had no recursion-depth cap on any of the internal recursive re-entry points that process nested structs, Vec<Self>-shaped sequences of structs, or enum variants. Each level of XML nesting added native call-stack frames that nothing bounded, so a sufficiently deeply nested (but otherwise well-formed) untrusted document would overflow the stack and abort the whole process -- a DoS that, unlike an ordinary parse error, cannot be caught as a `Result::Err`. Add `depth`/`max_depth` fields to `Deserializer` (default 128, matching serde_json's recursion limit; configurable via the new `Deserializer::recursion_limit`), and a `DepthGuard` RAII helper that increments on entry and decrements on drop so the counter stays balanced across `?`, panics, and sibling (non-nested) elements. The guard is threaded through the two chokepoints that between them cover every recursive re-entry point: - `ElementMapAccess::new`, the single constructor used by all four call sites that open a struct/map scope (`Deserializer::deserialize_struct`, `ElementDeserializer::deserialize_struct`, and both enum `struct_variant` impls in map.rs/var.rs) -- this closes both the cycle described in the issue body (a `$value` field holding a `Vec` of the same struct) and the `deserialize_seq`-via-`Vec<Self>` cycle found during variant analysis (comment on tafia#978), since both bottom out here. - `Deserializer::deserialize_enum`, re-entered by `VariantAccess::newtype_variant_seed` for a self-referential newtype variant (the third cycle from variant analysis). Note on that third cycle: it turned out not to be a distinct, attacker-controlled-depth DoS the way the other two are. Because `EnumAccess::variant_seed` only peeks the `Start` event and `newtype_variant_seed` re-enters `deserialize_enum` without consuming it either, the reader position never advances across the recursive call -- so `enum E { Leaf(bool), Wrap(Box<E>) }` fails to terminate at *any* depth, including a single `<Wrap></Wrap>` pair (confirmed with `RUST_MIN_STACK=1073741824`, a 1 GiB stack, still overflowing at depth 1). That is the same root cause as the already-open, already-filed tafia#819 (newtype-variant enum recursion), not a new one. The guard is kept anyway since it still converts that non-termination from an uncatchable abort into a clean, catchable error, and it also covers genuinely recursive struct/tuple enum variants via `ElementMapAccess::new`. Regression tests for all three cycles (before/after behavior spot- checked manually against master; in-tree tests check the fixed behavior only, since an abort can't be caught in-process) are in tests/serde-issues.rs under `mod issue978`. Full existing test suite (unit + all integration suites, including the serde-se.rs serialize round-trip tests, with and without the overlapped-lists feature) passes unchanged. Found with the rust-in-peace security pipeline: https://github.com/scadastrangelove/rust-in-peace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #982 +/- ##
==========================================
- Coverage 57.31% 56.96% -0.35%
==========================================
Files 46 47 +1
Lines 18197 18549 +352
==========================================
+ Hits 10429 10567 +138
- Misses 7768 7982 +214
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Mingun
left a comment
There was a problem hiding this comment.
The test-suite needs to be significantly extended. Also, it seems to me that DepthGuard type can be eliminated?
| /// let xml = "<Node>".repeat(1000) + &"</Node>".repeat(1000); | ||
| /// let mut de = Deserializer::from_str(&xml); | ||
| /// de.recursion_limit(100); | ||
| /// | ||
| /// let result = Node::deserialize(&mut de); | ||
| /// assert!(matches!(result, Err(DeError::TooDeeplyNested(100)))); |
There was a problem hiding this comment.
I think, that 10 and 11 are enough for demonstration purposes
| /// let xml = "<Node>".repeat(1000) + &"</Node>".repeat(1000); | |
| /// let mut de = Deserializer::from_str(&xml); | |
| /// de.recursion_limit(100); | |
| /// | |
| /// let result = Node::deserialize(&mut de); | |
| /// assert!(matches!(result, Err(DeError::TooDeeplyNested(100)))); | |
| /// let xml = "<Node>".repeat(11) + &"</Node>".repeat(11); | |
| /// let mut de = Deserializer::from_str(&xml); | |
| /// | |
| /// de.recursion_limit(11); | |
| /// let _ = Node::deserialize(&mut de).unwrap(); | |
| /// | |
| /// de.recursion_limit(10); | |
| /// let result = Node::deserialize(&mut de); | |
| /// assert!(matches!(result, Err(DeError::TooDeeplyNested(10)))); |
| /// the recursion-depth counter stays balanced across early returns (`?`) and | ||
| /// panics unwinding through the guarded recursive call, not just the | ||
| /// successful-return path. | ||
| struct DepthGuard<'a, 'de, R, E> |
There was a problem hiding this comment.
Is that type necessary? It seems to me that ElementMapAccess can fulfill its goals. It is created for each element, so nesting should be possible only via it.
| /// The default recursion limit (128, matching `serde_json`'s default) | ||
| /// applies even without calling `recursion_limit()` explicitly. | ||
| #[test] | ||
| fn default_limit_is_128() { | ||
| #[derive(Debug, Deserialize)] | ||
| #[allow(dead_code)] | ||
| struct Node { | ||
| #[serde(default, rename = "Node")] | ||
| node: Vec<Node>, | ||
| } | ||
|
|
||
| let xml = "<Node>".repeat(128) + &"</Node>".repeat(128); | ||
| let mut de = XmlDeserializer::from_str(&xml); | ||
| assert!(Node::deserialize(&mut de).is_ok()); | ||
|
|
||
| let xml = "<Node>".repeat(1000) + &"</Node>".repeat(1000); | ||
| let mut de = XmlDeserializer::from_str(&xml); | ||
| assert!(matches!( | ||
| Node::deserialize(&mut de), | ||
| Err(DeError::TooDeeplyNested(128)) | ||
| )); | ||
| } |
There was a problem hiding this comment.
It seems to me that this test not needed. Default value is not something that is important, it is just reasonable. Instead, I would like to see the test that demonstrates that sibling elements does not trigger the error.
| // Past the limit: a clean, catchable error -- before this fix, a | ||
| // sufficiently large depth here aborted the process instead | ||
| // (confirmed on master with depth 20_000). |
There was a problem hiding this comment.
I do not think that it is necessary to confirm in the comments that something was once broken in the master. The very presence of this test is already a sufficient argument
| // Past the limit: a clean, catchable error -- before this fix, a | |
| // sufficiently large depth here aborted the process instead | |
| // (confirmed on master with depth 20_000). |
| v: Vec<S>, | ||
| } | ||
|
|
||
| // Within the limit: deserializes successfully, same as before this fix. |
There was a problem hiding this comment.
| // Within the limit: deserializes successfully, same as before this fix. | |
| // Within the limit: deserializes successfully |
| /// Cycle 2 (from the variant-analysis comment): a plain (non-`$value`) | ||
| /// `Vec<Self>` field, which recurses through `deserialize_seq` and | ||
| /// `ElementDeserializer::deserialize_struct` -- a different code path | ||
| /// than cycle 1, so it needs its own guard. |
There was a problem hiding this comment.
| /// Cycle 2 (from the variant-analysis comment): a plain (non-`$value`) | |
| /// `Vec<Self>` field, which recurses through `deserialize_seq` and | |
| /// `ElementDeserializer::deserialize_struct` -- a different code path | |
| /// than cycle 1, so it needs its own guard. |
| /// Cycle 1: a struct whose `$value` field is a `Vec` of itself -- the | ||
| /// "struct-variant" shape that #819 says works correctly at shallow | ||
| /// depth, but which (before this fix) had no depth cap at all. |
There was a problem hiding this comment.
| /// Cycle 1: a struct whose `$value` field is a `Vec` of itself -- the | |
| /// "struct-variant" shape that #819 says works correctly at shallow | |
| /// depth, but which (before this fix) had no depth cap at all. |
| #[allow(dead_code)] | ||
| struct S { | ||
| #[serde(default, rename = "$value")] | ||
| v: Vec<S>, |
There was a problem hiding this comment.
To cover non-seq paths please also add tests with Box instead of Vec.
| /// Variant analysis (see the issue comments) found three independent | ||
| /// re-entry points sharing this root cause: | ||
| /// 1. the struct/map cycle described in the issue body itself -- a | ||
| /// `$value` field holding a `Vec` of the same struct | ||
| /// (`deserialize_struct` -> `ElementMapAccess` -> `next_value_seed`); | ||
| /// 2. `deserialize_seq` for an ordinary (non-`$value`) `Vec<Self>` field | ||
| /// (`MapValueSeqAccess::next_element_seed` -> `ElementDeserializer::deserialize_struct`); | ||
| /// 3. enum `newtype_variant` re-entry | ||
| /// (`VariantAccess::newtype_variant_seed` re-entering `Deserializer::deserialize_enum`). | ||
| /// | ||
| /// All three are exercised here with a small `recursion_limit()` so the | ||
| /// test stays fast; the default limit (128, matching `serde_json`) is | ||
| /// checked separately in `default_limit_is_128`. |
There was a problem hiding this comment.
No need to refer to comments, actually, you need much more tests than the three described here:
- struct field,
Box - struct field,
Vec - struct
$valuefield,Box - struct
$valuefield,Vec - enum newtype variant,
Box - enum newtype variant,
Vec - enum tuple variant,
Box - enum tuple variant,
Vec - enum struct variant, field,
Box - enum struct variant, field,
Vec - enum struct variant,
$valuefield,Box - enum struct variant,
$valuefield,Vec - Test that sibling elements does not trigger the threshold
| impl<'a, 'de, R, E> Deref for DepthGuard<'a, 'de, R, E> | ||
| where | ||
| R: XmlRead<'de>, | ||
| E: EntityResolver, | ||
| { | ||
| type Target = Deserializer<'de, R, E>; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| self.de | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'de, R, E> DerefMut for DepthGuard<'a, 'de, R, E> | ||
| where | ||
| R: XmlRead<'de>, | ||
| E: EntityResolver, | ||
| { | ||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| self.de | ||
| } |
There was a problem hiding this comment.
If DepthGuard will kept, it seems to me, that it would better to use de field directly instead of via traits.
| impl<'a, 'de, R, E> Deref for DepthGuard<'a, 'de, R, E> | |
| where | |
| R: XmlRead<'de>, | |
| E: EntityResolver, | |
| { | |
| type Target = Deserializer<'de, R, E>; | |
| fn deref(&self) -> &Self::Target { | |
| self.de | |
| } | |
| } | |
| impl<'a, 'de, R, E> DerefMut for DepthGuard<'a, 'de, R, E> | |
| where | |
| R: XmlRead<'de>, | |
| E: EntityResolver, | |
| { | |
| fn deref_mut(&mut self) -> &mut Self::Target { | |
| self.de | |
| } |
|
Hi @Mingun, thanks — I went through the matrix empirically rather than assuming my first version was right. I'm not a real developer, more of a fuzzer/security guy, so I apologize for any inaccuracies. The test draft / analysis below was created with help from an LLM-agent, so please double-check. I verified this locally against PR head 6d763c1. The arithmetic in my earlier pass was wrong: I conflated “10 test functions” with your 13 requested cases, but
The remaining four do not seem to be clean “shallow succeeds, deep fails with TooDeeplyNested” regression tests as-is:
On I do not think the guard concept can disappear entirely, though. There is a second independent guarded site in My proposed PR update would be: add the 9 green cases, do not add ignored tests for 6/7/8/9 unless you prefer explicit documentation of those existing enum-shape limitations. Edit(Mingun): removed references to unrelated issues inserted automatically |
If you have experience in fuzzing, then I expect that you understand that writing tests that cover all possible cases are important thing and you understand that integration tests should check software without assuming its internal implementation. This is a subtle point, but still we should consider
Ok,
Or at least store |
|
Fair point on covering everything — I went through the full matrix and tried Small attribution note: I'm more of a fuzzer/security person than a quick-xml Both diffs below apply independently on PR head 6d763c1, each passes full Full matrix — all 13, with an asserted expected result, no ignored tests. Two spots assert pre-existing limitations that are independent of the depth
Flagging those because if the underlying shapes are ever fixed, those On moving the guard into
One observation either way: the recursion actually happens inside full-13-case-matrix.diff (tests/serde-issues.rs, applies on 6d763c1)diff --git a/tests/serde-issues.rs b/tests/serde-issues.rs
index e7c5cba..7ba3e5e 100644
--- a/tests/serde-issues.rs
+++ b/tests/serde-issues.rs
@@ -1019,4 +1019,275 @@ mod issue978 {
Err(DeError::TooDeeplyNested(128))
));
}
+
+ // -------------------------------------------------------------------------
+ // Full self-reference matrix (requested in review). `Box` and `Vec` are
+ // treated as separate paths, and every case has an asserted expected result
+ // -- green where the shape round-trips within the limit and returns a clean
+ // `TooDeeplyNested` past it, and an explicit error assertion where the shape
+ // hits a *pre-existing* enum limitation unrelated to this PR's depth cap.
+ //
+ // # | container | field kind | covered by
+ // ----+----------------+-------------------+------------------------------
+ // 1 | struct | field, Box | struct_field_box (below)
+ // 2 | struct | field, Vec | cycle2_deserialize_seq_vec_self
+ // 3 | struct | $value, Box | struct_value_field_box (below)
+ // 4 | struct | $value, Vec | cycle1_struct_value_vec_self
+ // 5 | enum newtype | Box | cycle3_enum_newtype_variant_self
+ // 6 | enum newtype | Vec | enum_newtype_variant_vec (below)
+ // 7 | enum tuple | Box | enum_tuple_variant_box (below)
+ // 8 | enum tuple | Vec | enum_tuple_variant_vec (below)
+ // 9 | enum struct | named field, Box | enum_struct_variant_field_box (below)
+ // 10 | enum struct | named field, Vec | enum_struct_variant_field_vec (below)
+ // 11 | enum struct | $value, Box | enum_struct_variant_value_box (below)
+ // 12 | enum struct | $value, Vec | enum_struct_variant_value_vec (below)
+ // 13 | siblings must not accumulate depth | siblings_do_not_accumulate_depth
+ // -------------------------------------------------------------------------
+
+ /// #1 struct field, `Box<Self>`: within the limit round-trips, past it a
+ /// clean `TooDeeplyNested`.
+ #[test]
+ fn struct_field_box() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ struct Node {
+ #[serde(default, rename = "Node")]
+ node: Option<Box<Node>>,
+ }
+ let ok = "<Node>".repeat(5) + &"</Node>".repeat(5);
+ let mut de = XmlDeserializer::from_str(&ok);
+ de.recursion_limit(8);
+ assert!(Node::deserialize(&mut de).is_ok());
+
+ let deep = "<Node>".repeat(50) + &"</Node>".repeat(50);
+ let mut de = XmlDeserializer::from_str(&deep);
+ de.recursion_limit(8);
+ assert!(matches!(Node::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+ }
+
+ /// #3 struct `$value` field, `Box<Self>`.
+ #[test]
+ fn struct_value_field_box() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ struct S {
+ #[serde(default, rename = "$value")]
+ v: Option<Box<S>>,
+ }
+ let ok = "<a>".repeat(5) + &"</a>".repeat(5);
+ let mut de = XmlDeserializer::from_str(&ok);
+ de.recursion_limit(8);
+ assert!(S::deserialize(&mut de).is_ok());
+
+ let deep = "<a>".repeat(50) + &"</a>".repeat(50);
+ let mut de = XmlDeserializer::from_str(&deep);
+ de.recursion_limit(8);
+ assert!(matches!(S::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+ }
+
+ /// #6 enum newtype variant, `Vec<Self>`. Like the `Box` newtype variant
+ /// (`cycle3`), a self-referential newtype variant re-enters enum
+ /// deserialization without consuming the variant's `Start` event, so it is
+ /// depth-capped at *any* nesting (that non-termination is #819, not this
+ /// PR). The point this PR fixes is that the cap is now a catchable
+ /// `TooDeeplyNested` instead of a stack overflow.
+ #[test]
+ fn enum_newtype_variant_vec() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum E {
+ Leaf(bool),
+ Wrap(Vec<E>),
+ }
+ // A non-recursive variant is unaffected.
+ let mut de = XmlDeserializer::from_str("<Leaf>true</Leaf>");
+ de.recursion_limit(8);
+ assert!(E::deserialize(&mut de).is_ok());
+
+ // The self-referential variant is capped, not overflowed.
+ let mut de = XmlDeserializer::from_str("<Wrap><Leaf>true</Leaf></Wrap>");
+ de.recursion_limit(8);
+ assert!(matches!(E::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+ }
+
+ /// #7 enum tuple variant, `Box<Self>`. Tuple variants use repeated sibling
+ /// tags (`<Multi>..</Multi><Multi>..</Multi>`). Two sub-shapes:
+ /// - recursive slot first: capped with `TooDeeplyNested` (same enum
+ /// re-entry as the newtype cases), which is what this PR guarantees.
+ /// - recursive slot second: a *pre-existing* shape limitation independent
+ /// of the depth cap -- it fails to parse with `UnexpectedStart` before
+ /// depth is ever reached. Asserted explicitly so the behavior is pinned.
+ #[test]
+ fn enum_tuple_variant_box() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum First {
+ Leaf(bool),
+ Multi(Box<First>, u8),
+ }
+ let mut de = XmlDeserializer::from_str("<Multi>true</Multi><Multi>5</Multi>");
+ de.recursion_limit(8);
+ assert!(matches!(First::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum Second {
+ Leaf(bool),
+ Multi(bool, Box<Second>),
+ }
+ let mut de =
+ XmlDeserializer::from_str("<Multi>true</Multi><Multi><Leaf>true</Leaf></Multi>");
+ de.recursion_limit(8);
+ // Pre-existing: this shape does not parse regardless of the depth cap.
+ assert!(matches!(Second::deserialize(&mut de), Err(DeError::UnexpectedStart(_))));
+ }
+
+ /// #8 enum tuple variant, `Vec<Self>`. Same slot-first/slot-second split as
+ /// #7: recursive slot first is depth-capped; recursive slot second hits the
+ /// same *pre-existing*, non-depth-related `UnexpectedStart` as the `Box`
+ /// case -- confirming this is a tuple-variant limitation shared by both
+ /// container types, not something specific to `Box`.
+ #[test]
+ fn enum_tuple_variant_vec() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum First {
+ Leaf(bool),
+ Multi(Vec<First>, u8),
+ }
+ let mut de = XmlDeserializer::from_str("<Multi>true</Multi><Multi>5</Multi>");
+ de.recursion_limit(8);
+ assert!(matches!(First::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum Second {
+ Leaf(bool),
+ Multi(bool, Vec<Second>),
+ }
+ let mut de =
+ XmlDeserializer::from_str("<Multi>true</Multi><Multi><Leaf>true</Leaf></Multi>");
+ de.recursion_limit(8);
+ // Pre-existing: same shape limitation as the `Box` case, not depth-related.
+ assert!(matches!(Second::deserialize(&mut de), Err(DeError::UnexpectedStart(_))));
+ }
+
+ /// #9 enum struct variant, named `Box<Self>` field. The non-recursive shape
+ /// deserializes; the self-referential shape hits a *pre-existing* issue
+ /// (`unknown variant `$text``) that is independent of the depth cap -- the
+ /// analogous `Vec` (#10) and `$value` (#11) forms work. Asserted explicitly
+ /// so the current behavior is pinned rather than silently skipped.
+ #[test]
+ fn enum_struct_variant_field_box() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum E {
+ Leaf(bool),
+ Named {
+ #[serde(default, rename = "Named")]
+ node: Option<Box<E>>,
+ },
+ }
+ // Non-recursive shape works.
+ let mut de = XmlDeserializer::from_str("<Named><Leaf>true</Leaf></Named>");
+ de.recursion_limit(8);
+ assert!(E::deserialize(&mut de).is_ok());
+
+ // Self-referential shape: pre-existing failure, not the depth cap.
+ let mut de = XmlDeserializer::from_str("<Named><Named></Named></Named>");
+ de.recursion_limit(8);
+ assert!(matches!(E::deserialize(&mut de), Err(DeError::Custom(_))));
+ }
+
+ /// #10 enum struct variant, named `Vec<Self>` field.
+ #[test]
+ fn enum_struct_variant_field_vec() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum E {
+ Leaf(bool),
+ Named {
+ #[serde(default, rename = "Named")]
+ node: Vec<E>,
+ },
+ }
+ let ok = "<Named>".repeat(5) + &"</Named>".repeat(5);
+ let mut de = XmlDeserializer::from_str(&ok);
+ de.recursion_limit(8);
+ assert!(E::deserialize(&mut de).is_ok());
+
+ let deep = "<Named>".repeat(50) + &"</Named>".repeat(50);
+ let mut de = XmlDeserializer::from_str(&deep);
+ de.recursion_limit(8);
+ assert!(matches!(E::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+ }
+
+ /// #11 enum struct variant, `$value` field, `Box<Self>`.
+ #[test]
+ fn enum_struct_variant_value_box() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum E {
+ Leaf(bool),
+ Named {
+ #[serde(default, rename = "$value")]
+ v: Option<Box<E>>,
+ },
+ }
+ let ok = "<Named>".repeat(5) + &"</Named>".repeat(5);
+ let mut de = XmlDeserializer::from_str(&ok);
+ de.recursion_limit(8);
+ assert!(E::deserialize(&mut de).is_ok());
+
+ let deep = "<Named>".repeat(50) + &"</Named>".repeat(50);
+ let mut de = XmlDeserializer::from_str(&deep);
+ de.recursion_limit(8);
+ assert!(matches!(E::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+ }
+
+ /// #12 enum struct variant, `$value` field, `Vec<Self>`.
+ #[test]
+ fn enum_struct_variant_value_vec() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ enum E {
+ Leaf(bool),
+ Named {
+ #[serde(default, rename = "$value")]
+ v: Vec<E>,
+ },
+ }
+ let ok = "<Named>".repeat(5) + &"</Named>".repeat(5);
+ let mut de = XmlDeserializer::from_str(&ok);
+ de.recursion_limit(8);
+ assert!(E::deserialize(&mut de).is_ok());
+
+ let deep = "<Named>".repeat(50) + &"</Named>".repeat(50);
+ let mut de = XmlDeserializer::from_str(&deep);
+ de.recursion_limit(8);
+ assert!(matches!(E::deserialize(&mut de), Err(DeError::TooDeeplyNested(8))));
+ }
+
+ /// #13 sibling (non-nested) elements must not accumulate depth: 200 siblings
+ /// at one level deserialize fine under a small limit, proving the guard
+ /// counts nesting, not elements-seen.
+ #[test]
+ fn siblings_do_not_accumulate_depth() {
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ struct Leaf {
+ #[serde(default)]
+ _x: (),
+ }
+ #[derive(Debug, Deserialize)]
+ #[allow(dead_code)]
+ struct Root {
+ #[serde(default, rename = "Leaf")]
+ leaves: Vec<Leaf>,
+ }
+ let xml = format!("<Root>{}</Root>", "<Leaf/>".repeat(200));
+ let mut de = XmlDeserializer::from_str(&xml);
+ de.recursion_limit(4);
+ assert!(Root::deserialize(&mut de).is_ok());
+ }
}option-b-enumaccess-guard.diff (src/de/{mod,var}.rs, applies on 6d763c1)diff --git a/src/de/mod.rs b/src/de/mod.rs
index 7c39de7..83d7fbf 100644
--- a/src/de/mod.rs
+++ b/src/de/mod.rs
@@ -2593,12 +2593,12 @@ const DEFAULT_RECURSION_LIMIT: usize = 128;
/// the recursion-depth counter stays balanced across early returns (`?`) and
/// panics unwinding through the guarded recursive call, not just the
/// successful-return path.
-struct DepthGuard<'a, 'de, R, E>
+pub(crate) struct DepthGuard<'a, 'de, R, E>
where
R: XmlRead<'de>,
E: EntityResolver,
{
- de: &'a mut Deserializer<'de, R, E>,
+ pub(crate) de: &'a mut Deserializer<'de, R, E>,
}
impl<'a, 'de, R, E> Drop for DepthGuard<'a, 'de, R, E>
@@ -2673,7 +2673,7 @@ where
/// Must be called once per level of struct/sequence/enum nesting that
/// re-enters the deserializer, so that sibling (as opposed to nested)
/// elements do not spuriously accumulate depth.
- fn enter_depth(&mut self) -> Result<DepthGuard<'_, 'de, R, E>, DeError> {
+ pub(crate) fn enter_depth(&mut self) -> Result<DepthGuard<'_, 'de, R, E>, DeError> {
if self.depth >= self.max_depth {
return Err(DeError::TooDeeplyNested(self.max_depth));
}
@@ -3426,8 +3426,7 @@ where
// `VariantAccess::newtype_variant_seed`, adding native call-stack
// frames per level of XML nesting with no other bound.
// See https://github.com/tafia/quick-xml/issues/978
- let mut guard = self.enter_depth()?;
- visitor.visit_enum(var::EnumAccess::new(&mut *guard))
+ visitor.visit_enum(var::EnumAccess::new(self)?)
}
fn deserialize_seq<V>(self, visitor: V) -> Result<V::Value, DeError>
diff --git a/src/de/var.rs b/src/de/var.rs
index b554412..df4334c 100644
--- a/src/de/var.rs
+++ b/src/de/var.rs
@@ -3,7 +3,7 @@ use crate::{
de::map::ElementMapAccess,
de::resolver::EntityResolver,
de::simple_type::SimpleTypeDeserializer,
- de::{DeEvent, Deserializer, XmlRead, TEXT_KEY},
+ de::{DeEvent, DepthGuard, Deserializer, XmlRead, TEXT_KEY},
errors::serialize::DeError,
};
use serde::de::value::BorrowedStrDeserializer;
@@ -15,7 +15,7 @@ where
R: XmlRead<'de>,
E: EntityResolver,
{
- de: &'d mut Deserializer<'de, R, E>,
+ de: DepthGuard<'d, 'de, R, E>,
}
impl<'de, 'd, R, E> EnumAccess<'de, 'd, R, E>
@@ -23,8 +23,8 @@ where
R: XmlRead<'de>,
E: EntityResolver,
{
- pub fn new(de: &'d mut Deserializer<'de, R, E>) -> Self {
- EnumAccess { de }
+ pub fn new(de: &'d mut Deserializer<'de, R, E>) -> Result<Self, DeError> {
+ Ok(EnumAccess { de: de.enter_depth()? })
}
}
@@ -36,7 +36,7 @@ where
type Error = DeError;
type Variant = VariantAccess<'de, 'd, R, E>;
- fn variant_seed<V>(self, seed: V) -> Result<(V::Value, Self::Variant), Self::Error>
+ fn variant_seed<V>(mut self, seed: V) -> Result<(V::Value, Self::Variant), Self::Error>
where
V: DeserializeSeed<'de>,
{
@@ -66,7 +66,7 @@ where
R: XmlRead<'de>,
E: EntityResolver,
{
- de: &'d mut Deserializer<'de, R, E>,
+ de: DepthGuard<'d, 'de, R, E>,
/// `true` if variant should be deserialized from a textual content
/// and `false` if from tag
is_text: bool,
@@ -79,7 +79,7 @@ where
{
type Error = DeError;
- fn unit_variant(self) -> Result<(), Self::Error> {
+ fn unit_variant(mut self) -> Result<(), Self::Error> {
match self.de.next()? {
// Consume subtree
DeEvent::Start(e) => self.de.read_to_end(e.name()),
@@ -91,7 +91,7 @@ where
}
}
- fn newtype_variant_seed<T>(self, seed: T) -> Result<T::Value, Self::Error>
+ fn newtype_variant_seed<T>(mut self, seed: T) -> Result<T::Value, Self::Error>
where
T: DeserializeSeed<'de>,
{
@@ -102,11 +102,11 @@ where
_ => unreachable!("Only `Text` events are possible here"),
}
} else {
- seed.deserialize(self.de)
+ seed.deserialize(&mut *self.de)
}
}
- fn tuple_variant<V>(self, len: usize, visitor: V) -> Result<V::Value, Self::Error>
+ fn tuple_variant<V>(mut self, len: usize, visitor: V) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>,
{
@@ -119,12 +119,12 @@ where
_ => unreachable!("Only `Text` events are possible here"),
}
} else {
- self.de.deserialize_tuple(len, visitor)
+ (&mut *self.de).deserialize_tuple(len, visitor)
}
}
fn struct_variant<V>(
- self,
+ mut self,
fields: &'static [&'static str],
visitor: V,
) -> Result<V::Value, Self::Error>
@@ -132,7 +132,7 @@ where
V: Visitor<'de>,
{
match self.de.next()? {
- DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(self.de, e, fields)?),
+ DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(&mut *self.de, e, fields)?),
DeEvent::Text(e) => {
SimpleTypeDeserializer::from_text_content(e).deserialize_struct("", fields, visitor)
} |
Fixes #978.
The serde
Deserializerhad no recursion-depth cap on any of its re-entrant paths, so a maliciously deeply nested (but otherwise well-formed) untrusted XML document could crash the whole process via native stack overflow — uncatchable as aResult::Err.Correction to the record
The original issue (and my own follow-up comment) described three recursion cycles as independently depth-driven. Re-verifying against current
master, two are: the originaldeserialize_struct→next_value_seedcycle, and adeserialize_seqcycle via a plainVec<Self>field — both abort only past a real depth threshold (fine at depth 3, aborts by depth ~10,000-20,000). The third (enumnewtype_variantre-entry) is not an independent depth-driven DoS — it's issue #819's existing non-terminating-recursion bug re-exhibited:variant_seed/newtype_variant_seednever advance the reader position, so it aborts at depth 1 regardless of any cap. This PR does not fix #819 — recursive newtype enums still can't successfully deserialize, they now just fail cleanly (TooDeeplyNested) instead of aborting, since the same guard sits at the shared re-entry point.Design
Deserializergainsdepth/max_depth: usize(default 128, matchingserde_json) and a publicrecursion_limit()setter. ADepthGuard(RAII, Drop-based, panic/?-safe) is threaded through the fourElementMapAccess::newcall sites (covering the struct, seq, and enum struct/tuple-variant paths) and the top-leveldeserialize_enum. Exceeding the limit returns the newDeError::TooDeeplyNested(usize)instead of aborting.Testing
Before/after via standalone release-mode PoCs (built once against pristine master, once against this fix): both genuine cycles abort (exit 134) pre-fix at attacker-controlled depth and return a clean
Err(TooDeeplyNested(128))post-fix; ordinary shallow documents (depth ≤ 128) are unaffected. Full existing suite (cargo test --features serialize,overlapped-lists, and separately withoutoverlapped-lists): all passing, including the serialize round-trip suite.cargo fmt --check/cargo clippy --all-targetsclean.Discovered by the rust-in-peace security pipeline.