Skip to content

Limit nested model graph depth - #32344

Merged
Akshay Sonawane (apsonawane) merged 3 commits into
mainfrom
msrc/nested-subgraph-depth-limit
Sep 8, 2026
Merged

Akshay Sonawane (apsonawane) merged 3 commits into
mainfrom
msrc/nested-subgraph-depth-limit

Conversation

@apsonawane

Copy link
Copy Markdown
Contributor

This pull request introduces a validation step to limit the maximum allowed subgraph nesting depth in ONNX models to 32. This prevents excessively deep or recursive subgraph structures that could cause stack overflows or performance issues. The change is enforced during model loading and is covered by new unit tests.

Model validation enhancements:

  • Added a new function ValidateModelSubgraphDepth that checks the nesting depth of subgraphs in a model and returns an error if the depth exceeds kMaxModelSubgraphDepth (32). [1] [2]
  • Integrated ValidateModelSubgraphDepth into the Model constructor to enforce subgraph depth checks during model loading. [1] [2]

Testing improvements:

  • Added unit tests to verify that models with excessive subgraph depth are correctly rejected, and that the depth limit is enforced for both regular subgraphs and those in local functions. [1] [2]
  • Included necessary header for model_helpers.h in test files to support new validation logic.

API and constant definition:

  • Introduced the constant kMaxModelSubgraphDepth in model_helpers.h to define the supported subgraph depth limit.
  • Declared the new validation function in the header for use across the codebase. [1] [2]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a 32-level nesting limit to protect ONNX model loading from deeply nested subgraphs.

Changes:

  • Adds iterative subgraph-depth validation.
  • Enforces validation in model constructors.
  • Adds main-graph and local-function tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
onnxruntime/core/graph/model_helpers.h Declares the limit and validator.
onnxruntime/core/graph/model_helpers.cc Implements depth traversal.
onnxruntime/core/graph/model.cc Enforces validation during construction.
onnxruntime/test/ir/graph_test.cc Tests excessive main-graph depth.
onnxruntime/test/framework/function_test.cc Tests local-function graph depth.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread onnxruntime/core/graph/model_helpers.cc Outdated
@xadupre

Copy link
Copy Markdown
Member

protobuf is already checking that. So this limit only applies when is not going through protobuf or when it is modified. However, could we use the same limit?

@tianleiwu Tianlei Wu (tianleiwu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes for one additional ordering gap: validation currently runs after recursive protobuf copying on the const-reference model construction path, so it cannot protect that entry point. I also rechecked the existing open thread about graph-valued FunctionProto::attribute_proto() defaults; it still applies to this head, so I did not duplicate it.

Comment thread onnxruntime/core/graph/model.cc

@tianleiwu Tianlei Wu (tianleiwu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the current head. The graph-valued local-function default attributes are now included in the iterative depth traversal, and const-reference model protos are validated before recursive protobuf copying. The added boundary and regression tests cover both fixes. No remaining actionable findings.

@apsonawane
Akshay Sonawane (apsonawane) merged commit 79fc8c6 into main Sep 8, 2026
93 of 96 checks passed
@apsonawane
Akshay Sonawane (apsonawane) deleted the msrc/nested-subgraph-depth-limit branch September 8, 2026 23:15
This was referenced Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants