Bound merge-key expansion depth on borrowed and value deserializers#60
Conversation
The borrowed &Node/&Value Serde deserializers expanded << merge keys without any recursion limit, so a deeply nested caller-built merge chain could exhaust the stack and abort the process. They also disagreed with the owned Node path, which caps merge expansion at the default nesting depth. Value::apply_merge (used by from_value and the owned Value deserializer) likewise had no merge-depth limit. Thread a merge-depth budget through the borrowed ref-merge functions and through Value::apply_merge so every deserialization path enforces the same cap and reports the same depth-exceeded error. Merge chains at the limit still expand normally; chains beyond it now fail consistently instead of overflowing the stack.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 439ecdeafd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // accounting in step with the per-`<<` recursion the borrowed | ||
| // ref-merge paths perform; every other entry restarts its own | ||
| // chain at depth zero. | ||
| push_mapping_values(&mut values, mapping, next_depth + 1); |
There was a problem hiding this comment.
For caller-built Value merge chains, queuing the residual << value at next_depth + 1 makes the next loop iteration charge that same merge edge again before expanding it. This causes Value::apply_merge/from_value to reject valid chains well below the documented 128-link cap (a 65-link chain reaches depth = 128 and then fails on the next charge), while the borrowed &Value/&Node paths continue to count one level per <<. Queue the residual merge at next_depth so the following iteration accounts for only the next merge key.
Useful? React with 👍 / 👎.
1 participant
Summary
The borrowed
&Node/&ValueSerde deserializers were taught to expand<<merge keys but were given a merge engine with no depth limit. A deeply nested caller-built merge chain could:fatal runtime error: stack overflow(via&Node/from_nodeand&Value).Nodepath, which already caps merge expansion at the default nesting depth (MAX_MERGE_DEPTH= 128) inmerge_node_mapping. On the same hand-built structure, the ownedNodedeserializer returned a depth-exceededErrwhile the borrowed paths returnedOk(or crashed).The owned
Value::apply_merge(used byfrom_valueand the ownedValuedeserializer) was iterative — so it never overflowed — but it also had no merge-depth limit, so it disagreed with the capped paths on deep chains.Fix
merged_node_ref_entries/node_ref_merge_entriesandmerged_value_ref_entries/value_ref_merge_entries), enforcing the sameMAX_MERGE_DEPTH(128) as the ownedNodepath and returning the same depth-exceeded error.Value::apply_merge, sofrom_valueand the ownedValuedeserializer cap at the same boundary while preserving the existingserde_yaml-compatible (partial) resolution semantics for shallow merges.After the fix, at the boundary the owned
Node,&Node,from_node,from_value, and&Valuepaths all agree: a chain at the cap resolves, and a chain past it fails with the sameErrorCategory::Limit"maximum YAML nesting depth exceeded" error. No path overflows the stack.Normal-depth merges (untagged
<<, explicit merge tags, merge lists, nested merges, override precedence, and parser-produced configs) are unchanged.Tests
serde_api_caller_built_merge_depth_limit_is_consistent_across_pathsintests/serde_value_api.rs, asserting that a merge chain at depth 128 resolves on all five paths and a chain at depth 160 fails with the same depth-limit error on all five — exercising the cap well before recursion could approach a real stack overflow.serde_yaml-parity corpus (parser_properties),merge_defaults,serde_yaml_swap_harness,schema_modes,yaml11_conformance, anddos_hardening.No public API change (
docs/PUBLIC_API.txtunaffected).