Uh oh!
There was an error while loading. Please reload this page.
This repository was archived by the owner on Aug 20, 2026. It is now read-only.
fix: null-stride guard in canUse32BitIndexMath - #3
Merged
Conversation
DLPack permits DLTensor.strides == NULL for a compact row-major tensor. The 32-bit index-math check dereferenced stride[i] unconditionally, so a null strides array would segfault. A contiguous tensor's largest offset is numel-1, already known < INT32_MAX at that point, so short-circuit to `true` when the stride pointer is null. Defense-in-depth companion to the cpu-lib dispatch normalisation (fastfields-cpu-lib) which supplies contiguous strides. Part of #6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
balbasty added a commit
that referenced
this pull request
Aug 9, 2026
…74) `MeshDist::build_tree` sorts the faces of a BVH node with std::sort(FaceIterator(faces.data, ...) + begin, FaceIterator(faces.data, ...) + end, cmp); `FaceIterator`'s constructor eagerly builds a `Face`, whose pointer constructor loaded `copy` from `face` -- i.e. it dereferenced the iterator. For the past-the-end iterator (`end == nb_faces` at the top level) that reads D `index_t` values one face beyond the faces buffer. `FaceIterator`'s copy constructor rebuilt the `Face` from the raw pointer too, so every copy std::sort made of the end iterator repeated the over-read. This is reachable from the public API: `dt_mesh(..., signed=true, naive=false)` -> `distance_mesh::sdt` -> `build_tree`. AddressSanitizer aborts the existing cpu-lib mesh test on it: ==25759==ERROR: AddressSanitizer: heap-buffer-overflow READ of size 4 at 0x508000000200 #0 PointMixin<3,int,StaticPoint<3,int>>::copy_(...) mesh_utils.h:124 #1 StaticPoint<3,int>::StaticPoint(AnyConstPoint<3,int> const&) #2 MeshDist<3,float,int,int>::Face::Face(int*, int) mesh.h:673 #3 MeshDist<3,float,int,int>::FaceIterator::FaceIterator(int*,int,int) #4 operator+(FaceIterator const&, int) mesh.h:743 #5 MeshDist<3,float,int,int>::build_tree<...> mesh.h:833 ... #11 ff::cpu::dt_mesh(...) distance.cpp:732 0x508000000200 is located 0 bytes after 96-byte region allocated by ff::cpu::distance_mesh::copy_faces<3,int,int> Fix, without touching the sort machinery's semantics for valid iterators: * `Face(index_t*, offset_t)` no longer loads `copy`. Every dereference of a valid iterator goes through `operator*` -> `load_()`, which still loads, so sorted output is unchanged; only the never- dereferenced end iterator stops reading. * `Face(const Face&)` snapshots `other.copy` instead of re-reading through the shared data pointer. For a loaded `other` these are the same values (that is how std::sort's `__val` temporaries already worked); for the end iterator it avoids re-triggering the read. * `FaceIterator(const FaceIterator&)` delegates to `Face`'s copy constructor rather than rebuilding from the raw pointer. * `copy` is value-initialised so an unloaded `Face` never hands out indeterminate indices when copied. * Drive-by: `Face::load()` built `clone`, loaded it, then returned `*this` -- the const `operator*` therefore returned an unloaded Face. Return `clone`. Verification: * `make -C fastfields-cpu-lib test CXX=clang++` -- all 12 suites PASS, with check counts byte-identical to the pre-fix baseline (mesh: 4622 checks / 0 failures), so this is behaviour-preserving. * ASan+UBSan build of `tests/test_distance_mesh.cpp` + `distance.cpp` (g++ -fsanitize=address,undefined): before, it aborted on the heap-buffer-overflow; after, zero heap-buffer-overflow reports and the suite runs to completion (4622 checks, 0 failures). * `nvcc -x cu` compile of fastfields-cuda-lib/distance.cpp is clean -- the touched block is inside `#ifndef __CUDACC__`, so the device build is unaffected. Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z Co-authored-by: Claude <noreply@anthropic.com>
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
What
canUse32BitIndexMathdereferencedstride[i]unconditionally. DLPack permitsDLTensor.strides == NULLfor a compact row-major tensor, so a null strides array would segfault here. Short-circuit totruewhen the stride pointer is null: a contiguous tensor's largest offset isnumel-1, already known< INT32_MAXat that point.Why
Defense-in-depth companion to the cpu-lib dispatch normalisation (
fastfields-cpu-lib#…), which already supplies contiguous strides before this is reached. This makes the shared kernel safe for any caller that passes a raw null-strides descriptor.Verified
Header-only; exercised transitively through the cpu-lib test suites (all green). No behavioural change when strides are non-null.
Part of #6.
🤖 Generated with Claude Code
Generated by Claude Code