Skip to content

Conversation

@TwentyPast4
Copy link
Contributor

@TwentyPast4 TwentyPast4 commented Nov 4, 2025

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

This is not strictly a bug fix, it is just a low-cost method of making sure advanced indexing operations don't go out of bounds. The consequence of this happening could be either memory corruption, illegal memory access, or just undefined behavior. It makes sense to guard against these cases, as it is probably one of the more opaque ways a tensor operation might result in such behavior. Other comparable invalid operations might be caught by the user, but in my experience this one is the most opaque one. An example that would cause this issue:

Tensor a = Tensor::Init<int64_t>({0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
Tensor b = Tensor::Zeros({10}, open3d::core::Float32);
b.IndexAdd_(0, a, Tensor::Ones({a.GetLength()}, open3d::core::Float32));
// one of the operations would end up being (pseudocode) b[10] += 1;

The use cases of IndexSet, IndexGet, IndexAdd that I've come across have been of a computational nature, meaning the index was computed based on some arbitrary input data. Meaning that without proper due diligence, the validity of the indices is dependent on input data.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

Added an assertion check to Indexer, in a function which all index operations call to get input & output pointers.

@update-docs
Copy link

update-docs bot commented Nov 4, 2025

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@ssheorey ssheorey requested review from Copilot and ssheorey and removed request for ssheorey January 5, 2026 22:29
Copy link

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 adds bounds checking assertions to guard against memory corruption in tensor indexing operations. The changes introduce runtime validation to ensure that advanced indexing operations do not access memory outside of valid tensor bounds.

Key Changes:

  • Added bounds checking in Indexer::GetWorkloadDataPtr() to validate computed offsets before dereferencing pointers
  • Introduced total_byte_size_ tracking in TensorRef to properly handle both contiguous and non-contiguous tensor layouts
  • Added comprehensive test coverage for the new assertions on both CPU and CUDA devices

Reviewed changes

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

File Description
cpp/open3d/core/Indexer.h Added total_byte_size_ field to TensorRef and bounds checking assertions in GetWorkloadDataPtr() methods
cpp/open3d/core/AdvancedIndexing.h Added bounds checking assertion for index values in advanced indexing operations
cpp/tests/core/TensorCheck.cpp Added death tests to verify assertion behavior for out-of-bounds indexing on CPU and CUDA devices
CHANGELOG.md Documented the addition of assertion guards for invalid indexing operations

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

Comment on lines +124 to +127
// dimension block This way, we can compute the total buffer size in
// both cases (when tensor is contiguous and when it is not) If it
// is not contiguous, the actual "end" of the buffer may not be
// simply NumElements() * dtype_byte_size_
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

Missing punctuation at the end of sentences in the comment. Lines 123-125 should end with periods for consistency.

Suggested change
// dimension block This way, we can compute the total buffer size in
// both cases (when tensor is contiguous and when it is not) If it
// is not contiguous, the actual "end" of the buffer may not be
// simply NumElements() * dtype_byte_size_
// dimension block. This way, we can compute the total buffer size in
// both cases (when tensor is contiguous and when it is not). If it
// is not contiguous, the actual "end" of the buffer may not be
// simply NumElements() * dtype_byte_size_.

Copilot uses AI. Check for mistakes.
Comment on lines +187 to 190
assert(index >= -indexed_shape_[i] && index < indexed_shape_[i] && "Index out of bounds.");
#else
OPEN3D_ASSERT(index >= -indexed_shape_[i] &&
index < indexed_shape_[i] && "Index out of bounds.");
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The assertion message 'Index out of bounds.' differs from the CPU version's message 'Index out of bounds.' (line 190) in that the CUDA version uses a period. For consistency, both messages should be identical since they represent the same error condition.

Suggested change
assert(index >= -indexed_shape_[i] && index < indexed_shape_[i] && "Index out of bounds.");
#else
OPEN3D_ASSERT(index >= -indexed_shape_[i] &&
index < indexed_shape_[i] && "Index out of bounds.");
assert(index >= -indexed_shape_[i] && index < indexed_shape_[i] && "Index out of bounds");
#else
OPEN3D_ASSERT(index >= -indexed_shape_[i] &&
index < indexed_shape_[i] && "Index out of bounds");

Copilot uses AI. Check for mistakes.
assert(offset >= 0 && offset < input_.total_byte_size_);
#else
OPEN3D_ASSERT(offset >= 0 && offset < input_.total_byte_size_ &&
"TensorIterator operation data pointer is out of range.");
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The error message differs between TensorIterator::GetInputPtr() ('TensorIterator operation...') and Indexer::GetWorkloadDataPtr() ('Index operation...'). Consider using consistent terminology across similar assertions for clarity.

Suggested change
"TensorIterator operation data pointer is out of range.");
"Index operation data pointer is out of range.");

Copilot uses AI. Check for mistakes.
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.

1 participant