Fix OOB read in ConcatSliceElimination and UnsqueezeElimination#27718
Closed
Fix OOB read in ConcatSliceElimination and UnsqueezeElimination#27718
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes optimizer-time out-of-bounds/UB issues in two graph transforms by aligning Slice/Unsqueeze handling with ONNX/ORT semantics, and adds a regression test to prevent reintroducing the Concat+Slice crash.
Changes:
ConcatSliceElimination: Populate ONNX defaultaxesandstepswhen optional Slice inputs/attributes are omitted, preventing OOB reads in fusion logic.UnsqueezeElimination: Add duplicate-axis detection to avoid iterator OOB when computing the new initializer shape.- Tests: Add a regression test covering ConcatSliceElimination with Slice nodes omitting optional
axes/steps.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| onnxruntime/test/optimizer/graph_transform_test.cc | Adds regression coverage for ConcatSliceElimination when Slice omits optional inputs. |
| onnxruntime/core/optimizer/unsqueeze_elimination.cc | Prevents duplicate-axis cases from triggering invalid shape iteration during elimination. |
| onnxruntime/core/optimizer/concat_slice_elimination.cc | Adds ONNX-default axes/steps population to avoid OOB reads during fusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+145
to
+156
| // Per ONNX Slice spec: if axes not provided, default to [0, 1, ..., len(starts)-1]. | ||
| if (axes.empty()) { | ||
| axes.reserve(starts.size()); | ||
| for (int64_t i = 0, limit = static_cast<int64_t>(starts.size()); i < limit; ++i) { | ||
| axes.push_back(i); | ||
| } | ||
| } | ||
|
|
||
| // Per ONNX Slice spec: if steps not provided, default to all 1s. | ||
| if (steps.empty()) { | ||
| steps.assign(starts.size(), int64_t{1}); | ||
| } |
Contributor
Author
|
Closing the PR in favor of #27638 |
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
GetSliceInfoinConcatSliceEliminationreturns true with empty axes/steps vectors when a Slice node omits optional inputs.The caller
FuseConcatSliceSubgraphthen accesses axes[0] and steps[0] without checking, reading uninitialized InlinedVectorRoot Cause
GetSliceInfodid not populate ONNX spec defaults for omitted optional Slice inputs:Fix
concat_slice_elimination.cc:Populate ONNX spec defaults before returning from GetSliceInfo. axes defaults to [0, 1, ...,len(starts)-1], steps defaults to all 1s. This matches how ORT's Slice kernel handles missing inputs in slice_helper.h.
unsqueeze_elimination.cc(related variant): Added duplicate axis detection to prevent an iterator from reading pasttensor_proto.dims().cend() when axes contain duplicates.
Testing
ConcatSliceEliminationTest_NoAxesStepsregression test