Guard three graph-optimizer passes against unbounded model-supplied indices - #31670
Open
titaiwangms wants to merge 3 commits into
Open
Guard three graph-optimizer passes against unbounded model-supplied indices#31670titaiwangms wants to merge 3 commits into
titaiwangms wants to merge 3 commits into
Conversation
…ndices - GatherSliceToSplitFusion: reject an axis attribute/value outside the known rank of the shared input before indexing its shape, instead of assuming ONNX shape inference already bounded it. - Transpose optimizer HandleTile: verify a constant 'repeats' initializer has one entry per dimension (matching the preceding Transpose's rank) before using it to reorder values, instead of assuming the model's 'repeats' length always matches. - WhereDummyDq: DequantizeLinear's zero-point input is optional per the ONNX spec; skip inserting a dummy DQ (with a warning) when the DQ node has fewer than 3 inputs instead of assuming a zero-point is present. All three inputs originate from the model graph and were not otherwise range-checked at the point of use. Added regression tests for each in graph_transform_test.cc, transpose_optimizer_test.cc, and qdq_transformer_test.cc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77dcaf1b-748a-4379-94a7-478f7a924d73
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens three model-load-time graph-optimizer passes against out-of-bounds indexing when consuming model-supplied axes/indices (including optional inputs), preventing crashes/asserts on malformed or adversarial models while preserving behavior for valid models.
Changes:
- Add bounds checks in
GatherSliceToSplitFusion(axis vs rank), transpose optimizerHandleTile(repeats length vs rank), andWhereDummyDq(optional DQ zero-point input). - Add regression tests that exercise these previously-unchecked paths using
TestGraphTransformer/direct transformer application. - Minor test wiring to instantiate
TransposeOptimizerdirectly for the new Tile regression test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/core/optimizer/gather_fusion.cc | Reject out-of-range axis before indexing into the shared input shape during Gather/Slice→Split fusion. |
| onnxruntime/core/optimizer/transpose_optimization/onnx_transpose_optimization.cc | Guard Tile handler against repeats.size() != rank before indexing repeats using perm_inv. |
| onnxruntime/core/optimizer/qdq_transformer/where_dummy_dq.cc | Guard DequantizeLinear optional zero-point input before reading InputDefs()[2], skipping the optimization safely. |
| onnxruntime/test/optimizer/graph_transform_test.cc | Add regression test for out-of-range Gather axis reaching the fusion pass. |
| onnxruntime/test/optimizer/transpose_optimizer_test.cc | Add regression test for Tile repeats/rank mismatch reaching transpose optimizer handler without static rank. |
| onnxruntime/test/optimizer/qdq_transformer_test.cc | Add regression test covering a valid 2-input DequantizeLinear feeding Where, ensuring no dummy DQ insertion. |
A DequantizeLinear node without a zero-point input is spec-valid (the input is optional), so this is a routine optimization skip rather than an anomaly worth surfacing at WARNING severity in ordinary logs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77dcaf1b-748a-4379-94a7-478f7a924d73
…fixes - WhereDummyDq: also treat an explicit empty/missing NodeArg placeholder for DequantizeLinear's optional zero-point input as 'not present', not just a shorter input list, so it takes the same VERBOSE skip path instead of falling through to an indexed initializer lookup and a misleading warning. - HandleTile: check the node has its second required input before reading it, matching the same guard already present in the sibling Gather handler, for consistency in build configurations where schema validation is bypassed. - Added a regression test for the empty-placeholder zero-point case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77dcaf1b-748a-4379-94a7-478f7a924d73
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Three graph-optimizer passes that run at model load time index into a vector using a
count/axis/index taken directly from the model graph, without validating it against the
bounds of the vector being indexed:
GatherSliceToSplitFusion(onnxruntime/core/optimizer/gather_fusion.cc): reads anaxisfrom a candidateGather/Sliceconsumer and indexes the shared input's shapewith it (
shape->dim(axis)). ONNX's own shape inference forGather/Sliceonly range-checksaxiswhen it can resolve the shapes of all relevant inputs; if it cannot (e.g. an unresolvableindices/axis input shape), that check is skipped, and this fusion pass previously had no
independent bounds check of its own.
Transpose optimizer's
HandleTile(onnxruntime/core/optimizer/transpose_optimization/onnx_transpose_optimization.cc):assumes a constant
Tilerepeatsinitializer has one entry per dimension of thepreceding
Transpose's rank (taken from that node'spermattribute), and indexes itaccordingly. Similarly, ONNX's own
Tileshape inference only validatesrepeats.size()against the input rank when it can resolve the data input's shape; if it cannot, this
validation is skipped, and there was no independent check before indexing
repeats.WhereDummyDq(onnxruntime/core/optimizer/qdq_transformer/where_dummy_dq.cc):unconditionally reads
DequantizeLinear's 3rd input (x_zero_point, at index 2) tofetch its initializer. Per the ONNX spec,
x_zero_pointis an optional input, so avalid
DequantizeLinearnode can have only 2 inputs (x,x_scale), making this anout-of-bounds read on
InputDefs().Fix
Each pass now validates the model-supplied value against the actual bound before using it
to index, and safely skips the optimization (rather than asserting/crashing) when the value
is out of range:
GatherSliceToSplitFusion: skip fusing this candidate ifaxis < 0 || axis >= rank.HandleTile: returnfalse(leave the node alone) ifrepeats.size() != rank.WhereDummyDq: log a warning and skip inserting a dummy DQ if the DQ node has fewer than3 inputs.
Other execution providers
All three passes are execution-provider-agnostic graph transformations that run before EP
partitioning, so no EP-specific (e.g. CUDA) equivalent exists or needs a separate fix.
Testing
Added regression tests, using
TestGraphTransformer(which applies the transformer andinspects the resulting graph without executing the model, since some of the malformed
inputs used to reach these code paths are not valid inputs to actually run):
graph_transform_test.cc:GatherSliceToSplitFusion_OutOfRangeAxis— aGathernodewith an out-of-range
axis, where the sibling indices input has no static shape soONNX's own inference cannot catch it ahead of time. Verifies the fusion pass leaves the
graph unchanged.
transpose_optimizer_test.cc:TestTileRepeatsRankMismatchNoOpt— aTranspose -> Tile -> Transposepattern whererepeatsis shorter than the rank implied by theTranspose'sperm, with the data input's shape left unresolved. Verifies bothTransposes and the Tile remain untouched.
qdq_transformer_test.cc:WhereDummyDqTest_DqWithoutZeroPoint— aDequantizeLinearwith only 2 inputs (no zero-point) feeding a
Wherenode. Verifies no dummy DQ isinserted and the graph is otherwise unmodified.
All three new tests were confirmed to fail (or crash) when the corresponding fix was
temporarily reverted, and pass cleanly with the fix in place. The full
GraphTransformationTests,TransposeOptimizerTests, andQDQTransformerTestssuiteswere also run locally with no regressions.
Motivation
These three passes run by default during
CreateSession(default optimization level),processing whatever graph structure the model declares. Guarding the indexing operations
against out-of-range model-supplied values makes these passes resilient to malformed or
adversarial models without changing behavior for well-formed ones.