-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[ICM CXX] Address Pad kernel shortcomings #26864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…nxruntime into yuslepukhin/pad_rce
This reverts commit 96a6045.
…expected. [ FAILED ] PadOpTest/0.Pad_Constant_DimWithZeroInput, where TypeParam = float [ FAILED ] PadOpTest/1.Pad_Constant_DimWithZeroInput, where TypeParam = double
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses shortcomings in the Pad kernel implementation across CPU and CUDA providers, with a focus on improving robustness, safety, and edge case handling for padding operations.
Key Changes:
- Enhanced integer overflow protection using SafeInt throughout CPU and CUDA Pad implementations
- Added validation for edge cases including zero-extent dimensions and reflect mode constraints
- Introduced new comprehensive test cases for edge conditions, negative padding, and mixed-sign padding scenarios
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/test/testdata/test_pad_rce.py | Python script to generate ONNX model for RCE (Remote Code Execution) testing with dynamic pad inputs |
| onnxruntime/test/testdata/test_pad_rce.onnx | Generated ONNX model artifact for pad RCE testing |
| onnxruntime/test/providers/cpu/tensor/pad_test.cc | Updated test exclusions for DML provider, added extensive new tests for edge cases, negative padding, and various pad modes |
| onnxruntime/core/providers/cuda/tensor/pad.h | Minor update with copyright header duplication issue |
| onnxruntime/core/providers/cuda/tensor/pad.cc | Enhanced type safety (int32_t→size_t), added SafeInt usage, improved edge case handling for zero extents and reflect mode |
| onnxruntime/core/providers/cuda/cuda_utils.cu | Added Fill template specialization for bool type |
| onnxruntime/core/providers/cpu/tensor/padbase.h | Added helper functions for flattening validation and element coverage checking |
| onnxruntime/core/providers/cpu/tensor/pad.cc | Introduced OutputSink debug wrapper, enhanced SafeInt usage, improved validation for zero extents and reflect mode, conditional flattening logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tianleiwu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and comprehensive tests looks good.
OutputSink only checks boundary in debug build. Is there use case that need bounds check in release build?
Some CI pipeline failed. Please take a look.
Description
Address OOB reads and writes issues.
Motivation and Context
#11828
#13332
See also this: onnx/onnx#4294