Fix OOB read in InferenceContextImpl::getInputData - #32681
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The new tests exercise unfixed out-of-bounds paths, while the stated omitted-pads regression remains untested.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents crashes when ConvTransposeWithDynamicPads omits its optional Pads input.
Changes:
- Bounds-checks shape-inference input data access.
- Rejects missing dynamic pads during CPU execution.
- Adds mismatched-rank regression tests.
File summaries
| File | Description |
|---|---|
onnxruntime/core/graph/graph.cc |
Guards omitted input access. |
onnxruntime/core/providers/cpu/nn/conv_transpose_attributes.h |
Handles null dynamic pads. |
onnxruntime/test/contrib_ops/conv_transpose_with_dynamic_pads_test.cc |
Adds rank-mismatch tests. |
Review details
Suppressed comments (1)
onnxruntime/test/contrib_ops/conv_transpose_with_dynamic_pads_test.cc:92
- This case also exercises an out-of-bounds access that is not fixed by this PR. With one input spatial dimension and three weight spatial dimensions, shape inference creates a one-element
dilationsvector and then indexes it three times while scalingkernel_shape(contrib_defs.cc:113-117). The expected kernel error is reached only after undefined behavior. Add the same X/W rank-equality validation in shape inference before this loop.
TEST(ContribOpTest, ConvTransposeWithDynamicPads_LongerWeightRank) {
OpTester test("ConvTransposeWithDynamicPads", 1, onnxruntime::kMSDomain);
test.AddInput<float>("X", {1, 1, 2}, std::vector<float>(2, 0.0f));
test.AddInput<float>("W", {1, 1, 3, 3, 3}, std::vector<float>(27, 0.0f));
test.AddInput<int64_t>("Pads", {2}, std::vector<int64_t>(2, 0));
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2bff297 to
cb3f7c5
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The missing-pads test expects a CPU-specific error and will fail under the cuDNN 9 CUDA provider.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
cb3f7c5 to
7c5348f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The cuDNN 8 cached execution path can bypass the new validation on a repeated run.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
efd69c2 to
73a82d7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
CUDA 8 can reuse an algorithm and workspace cached for a different dynamic-padding descriptor, and the repeated-run path lacks coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
bae0a22 to
8071b1c
Compare
Bryan B (bbernhar)
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The memory-safety fix needs a regression test covering an omitted trailing input.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
optional input is omitted 'Pads' on ConvTransposeWithDynamicPads is schema-optional, so a node can validly supply only [X, W]. InferenceContextImpl::getInputData (graph.cc) indexed node_.InputDefs() by the ONNX input index without checking it against InputDefs().size(), so shape inference on such a node read past the end of the vector. Add the same bounds check already used by DataPropagationContextImpl::getInputData.
8071b1c to
522265b
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused bounds check matches existing behavior and the regression test covers the reported failure path.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
|
The failing build is fixed in main branch. |
|
Xavier Dupré (@xadupre) merged |
Description
InferenceContextImpl::getInputData(graph.cc) indexednode_.InputDefs()directly bythe schema input index during shape inference, with no bounds check. Add the same bounds
check already used by
DataPropagationContextImpl::getInputDatain the same file, sogetInputDatareturnsnullptr(treated as "input not available") instead of indexingout of bounds.
Motivation and Context
ONNX schemas can declare trailing inputs as optional (e.g.
ConvTransposeWithDynamicPads'sPads), so a node can validly omit them entirely, shrinkingNode::InputDefs()below theschema's full input count. A
TypeAndShapeInferenceFunctionthat queries such an omittedoptional input's data (e.g. via
ctx.getInputData(index)) on such a node reads past theend of the vector, causing an out-of-bounds read.