Skip to content

Improve SparseTensors public API input validation as well as sparse utilities#28227

Merged
yuslepukhin merged 7 commits intomainfrom
yuslepukhin/msrc_113760_113215
Apr 28, 2026
Merged

Improve SparseTensors public API input validation as well as sparse utilities#28227
yuslepukhin merged 7 commits intomainfrom
yuslepukhin/msrc_113760_113215

Conversation

@yuslepukhin
Copy link
Copy Markdown
Member

This pull request significantly improves the safety and robustness of sparse tensor handling in ONNX Runtime. The main focus is on adding thorough bounds checking and using safe integer arithmetic to prevent overflows and invalid memory accesses when working with sparse tensor indices. Additionally, the Python bindings for sparse tensors are refactored to ensure correct object lifetimes and memory management when exposing data to NumPy.

Sparse Tensor Index Validation and Safety

  • Added comprehensive bounds checks for COO and CSR sparse tensor indices in both the C API (onnxruntime_c_api.cc) and core conversion utilities, ensuring indices are within valid ranges and, for CSR, that outer indices are non-decreasing and within bounds. [1] [2] [3] [4] [5] [6]
  • Replaced direct arithmetic with SafeInt for all index and size calculations to prevent integer overflows, especially when converting between types or computing dense tensor offsets. [1] [2] [3] [4] [5]
  • Improved error messages for invalid indices, making debugging easier by providing more context about the specific error. [1] [2] [3] [4] [5] [6]

Python Bindings Improvements

  • Refactored the pybind11 bindings for sparse tensor views so that NumPy arrays referencing sparse tensor memory correctly keep the parent Python object alive, preventing potential memory issues when the sparse tensor is on the GPU or managed by Python. [1] [2] [3]

General Code Quality

  • Added missing header include for safeint.h to ensure SafeInt is available where needed.
  • Minor cleanups and improved assertions to clarify intent and ensure correctness.

These changes collectively make sparse tensor support in ONNX Runtime safer, more reliable, and easier to use from both C++ and Python.

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 hardens ONNX Runtime sparse tensor handling by adding stricter index/bounds validation (COO/CSR), switching key size computations to SafeInt, and improving Python sparse tensor views to keep backing memory alive when exposed to NumPy.

Changes:

  • Add COO/CSR index validation in the C API entry points and sparse conversion utilities to reduce invalid access risk.
  • Use SafeInt for index/byte-size calculations in sparse tensor proto unpacking and dense materialization paths.
  • Update pybind sparse tensor/view methods so NumPy arrays keep the owning Python object alive.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
onnxruntime/core/session/onnxruntime_c_api.cc Adds COO/CSR index validation for public C APIs (Fill*/Use*).
onnxruntime/core/framework/sparse_utils.cc Adds additional bounds checks and overflow-safe index math in sparse-to-dense conversions.
onnxruntime/core/framework/tensorprotoutils.cc Uses SafeInt for sparse proto raw-data size and dense buffer sizing.
onnxruntime/python/onnxruntime_pybind_sparse_tensor.cc Fixes NumPy “base object” lifetime management for sparse indices/values views.
onnxruntime/test/shared_lib/test_nontensor_types.cc Adds C API tests for invalid COO/CSR indices (exception builds).
onnxruntime/test/framework/sparse_kernels_test.cc Adds model-loading + sparse_utils conversion tests for negative/out-of-range indices.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/core/framework/sparse_utils.cc Outdated
Comment thread onnxruntime/core/session/onnxruntime_c_api.cc Outdated
Comment thread onnxruntime/core/session/onnxruntime_c_api.cc Outdated
Comment thread onnxruntime/core/session/onnxruntime_c_api.cc Outdated
Comment thread onnxruntime/core/session/onnxruntime_c_api.cc Outdated
Comment thread onnxruntime/core/framework/sparse_utils.cc Outdated
Comment thread onnxruntime/core/framework/sparse_utils.cc Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/core/session/onnxruntime_c_api.cc Outdated
Comment thread onnxruntime/core/session/onnxruntime_c_api.cc Outdated
Comment thread onnxruntime/core/framework/sparse_utils.cc Outdated
@yuslepukhin yuslepukhin requested a review from Copilot April 27, 2026 21:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@vraspar vraspar left a comment

Choose a reason for hiding this comment

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

Nice work on this security fix. The validation is thorough, SafeInt usage is correct, and the pybind11 lifetime fix is a real improvement.

Suggestion (follow-up): The COO index validation in FillSparseTensorCoo and UseCooIndices is nearly identical (~30 lines each), and same for the CSR validation pair. Not suggesting a change on this PR since keeping a security fix self-contained makes sense, but a follow-up to extract shared helpers like ValidateCooIndices()/ValidateCsrIndices() would prevent these from drifting apart over time.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/core/framework/tensorprotoutils.cc
Comment thread onnxruntime/core/framework/tensorprotoutils.cc
Comment thread onnxruntime/core/framework/tensorprotoutils.cc
Comment thread onnxruntime/core/session/onnxruntime_c_api.cc
@yuslepukhin yuslepukhin requested a review from vraspar April 28, 2026 00:04
@yuslepukhin yuslepukhin enabled auto-merge (squash) April 28, 2026 20:33
@yuslepukhin yuslepukhin merged commit 7a795ed into main Apr 28, 2026
93 of 94 checks passed
@yuslepukhin yuslepukhin deleted the yuslepukhin/msrc_113760_113215 branch April 28, 2026 20:33
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.

3 participants