Uh oh!
There was an error while loading. Please reload this page.
Cap UDT expansion depth in arg_value_name to prevent stack overflow on recursive specs - #2679
Cap UDT expansion depth in arg_value_name to prevent stack overflow on recursive specs#2679Galmanus wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds safeguards and regression coverage to prevent stack overflows when rendering CLI argument value names for contracts with recursive UDTs in their spec (issue #2445).
Changes:
- Cap UDT expansion depth in
Spec::arg_value_nameto prevent unbounded recursion on recursive type graphs. - Add spec-tools regression tests for self-recursive and mutually-recursive structs, and ensure non-recursive nesting still expands fully.
- Add a soroban-cli regression test ensuring
build_custom_cmd(...).render_long_help()succeeds for a recursive spec type.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/soroban-cli/src/commands/contract/arg_parsing.rs | Adds a regression test to ensure clap help generation doesn’t crash when a contract spec includes recursive types. |
| cmd/crates/soroban-spec-tools/src/lib.rs | Adds a maximum recursion/expansion depth for UDT value-name rendering and tests for recursive and non-recursive cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // UDTs are the only types resolved by name, so any cycle in a | ||
| // recursive spec passes through here; cap the expansion depth | ||
| // and fall back to the type's name (#2445). | ||
| if depth > Self::MAX_UDT_VALUE_NAME_DEPTH { |
There was a problem hiding this comment.
The > check already matches the doc comment, so I'll keep it. The comment defines the constant as "the maximum nesting depth at which user-defined types are still expanded" (L1155-1156) and says "past this depth the type's name is rendered" (L1161) — "past" meaning strictly greater than the max. So expansion is intended to still happen at depth == MAX_UDT_VALUE_NAME_DEPTH and stop only beyond it, which is exactly what > does (depth 4 expands, depth 5 renders the name).
Switching to >= would stop expansion at depth 4, making the true maximum expansion depth 3 — that contradicts both the constant's name and the "still expanded" wording. In other words >= would introduce the off-by-one rather than remove it.
Fair point that the current tests don't actually pin this boundary — they only assert termination and that the outer value name survives, so they'd pass under either operator. I can add an assertion that fixes the expansion depth to the documented value to make the intended cutoff unambiguous.
…n recursive specs
`Spec::arg_value_name` threads a `depth` counter but never checks it. A
contract whose spec contains a recursive type — e.g.
`struct TreeNode { children: Vec<TreeNode> }`, which the SDK and the
network accept — sent the resolver into unbounded recursion, overflowing
the stack on every `contract invoke` against the contract, including
`-- --help`. Anyone can deploy such a contract, so this crashed the CLI
of anyone who tried to interact with it.
UDTs are the only types resolved by name, so any cycle must pass through
the `ScType::Udt` arm; cap the expansion depth there and render the
type's name past the cap. Returning `None` instead would erase the
whole value name, since `None` propagates through the `?` in every
caller. `example_udts` already bounds example rendering the same way.
Complements stellar#2668, which capped XDR decode depth for deeply-nested
specs; a cycle through UDT name references never nests deeply in the
encoded XDR, so it needs this separate guard.
Fixesstellar#2445af4e67f to
3a1c8fbCompare| if depth > Self::MAX_UDT_VALUE_NAME_DEPTH { | ||
| return Some(name.to_utf8_string_lossy()); |
Fixes#2445.
Spec::arg_value_namethreads adepthcounter but never checks it. A contract whose spec contains a recursive type (e.g.struct TreeNode { children: Vec<TreeNode> }— accepted by the SDK and the network) sends the resolver into unbounded recursionTreeNode → Vec<TreeNode> → TreeNode → …, overflowing the stack on anycontract invokeagainst the contract, including-- --help. Since anyone can deploy such a contract, this is a client-side DoS on anyone who tries to interact with it.Fix
UDTs are the only types resolved by name (
self.find(...)), so any cycle must pass through theScType::Udtarm. This caps the expansion depth there and renders the type's name past the cap — the same approachexample_udtsalready uses for examples (depth > 2→None), except returning the name keeps the outer value name intact: aNonepropagates through the?in every caller and would erase the entire help string.For the reproduction from the issue,
--helpnow renders the argument as:Tests
arg_value_name_terminates_on_self_recursive_struct— the issue'sTreeNodeshape; aborted withfatal runtime error: stack overflowbefore the fix.arg_value_name_terminates_on_mutually_recursive_structs—A ↔ Bcycle.arg_value_name_expands_non_recursive_nested_structs— output unchanged for non-recursive specs.build_custom_cmd_terminates_on_recursive_spec_type(soroban-cli) — end-to-end guard at the clap-command level, the surface the issue's repro crashes.Related
#2668 capped XDR decode depth for deeply-nested specs. A cycle through UDT name references never nests deeply in the encoded XDR, so it needs this separate guard.