fix: non-contiguous array handling and memory safety in C++ bindings#1147
Merged
Conversation
38 tasks
…udge - array_like() copied the input array's strides onto the freshly allocated output array, but callers fill the result linearly in C order from a C-contiguous copy of the input. Fortran-order input gave transposed results and negative strides caused out-of-bounds writes (e.g. IntCategory.index on a reversed array). Allocate a plain C-contiguous output instead. - The std::vector<std::string> caster iterated numpy 'S'/'U' arrays with p += itemsize, ignoring strides, so sliced or reversed string arrays gave silently wrong indices/fills (and reversed views read out of bounds). Take a C-contiguous copy when the input is not contiguous. - edges(..., numpy_upper=true) nudged the last edge with std::nextafter(x, std::numeric_limits<double>::min()), which moves toward ~0: the wrong direction for negative or zero upper edges (Regular(4, -2, -1).to_numpy() reported a last edge above -1). Use std::numeric_limits<double>::lowest() to always nudge downward. Part of #1143 Assisted-by: ClaudeCode:claude-fable-5
A malformed pickle state could write past heap buffers: - weighted_sum/mean/weighted_mean storage load() resized to a.size() / N and then copied a.size() elements; if the serialized buffer size was not a multiple of N this wrote past the allocation. Now throws std::runtime_error on a size mismatch. - multi_cell::serialize swap_ranges'd over size_ * nelem_ elements of the loaded vector without checking its size. Now validated. - tuple_iarchive::operator>>(py::array_t<T>&) reinterpreted an arbitrary tuple element as a typed array without any validation. Now uses py::array_t<T>::ensure (C-contiguous, forcecast) and raises ValueError if the element is not convertible to an array. - The array_wrapper load path only guarded the size match with BOOST_ASSERT, which is compiled out in release builds. Now a real runtime check. Part of #1143 Assisted-by: ClaudeCode:claude-fable-5
- The __deepcopy__ lambdas for axes and histograms did a raw 'new T(self)' and then called copy.deepcopy(metadata), which can throw, leaking the allocation. Construct via std::make_unique and return the unique_ptr (pybind11 accepts holder returns). - axis_cast<int> used py::isinstance<int>(x), which is always false (int is not a registered C++ type), so the integer fast path was dead and every Python int went through a double round-trip. Use py::isinstance<py::int_> with a direct py::cast<int>, letting overflow raise instead of invoking undefined behavior via a double-to-int cast. Part of #1143 Assisted-by: ClaudeCode:claude-fable-5
henryiii
force-pushed
the
fix/cpp-noncontiguous-memsafety
branch
from
7754f3e to
5ff7c02
Compare
Pylint cannot statically resolve the .min attribute on np.finfo. Since np.nextafter only uses the direction of its second argument, -np.inf produces an identical result while reading more clearly. Assisted-by: ClaudeCode:claude-opus-4.8
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🤖 AI text below 🤖
Part of #1143
Fixes a set of non-contiguous-array and memory-safety findings in the C++ bindings:
array_like<T>no longer copies the input array's strides onto the output; callers fill the result linearly in C order, so Fortran-order input toIntCategory.indexreturned transposed results and negative-stride input caused out-of-bounds writes. The output is now plain C-contiguous.std::vector<std::string>caster ignored strides when iterating numpyS/Uarrays, so sliced/reversed string arrays gave wrongStrCategory.index/fillresults (and reversed views read OOB). Non-contiguous inputs now get a C-contiguous copy first.load()now validates that the serialized buffer size is a multiple of the accumulator width before resizing/copying;multi_cell::serializevalidates the loaded buffer length;tuple_iarchivevalidates/converts array elements withpy::array_t<T>::ensureinstead of blindly reinterpreting, and thearray_wrappersize check is a real runtime check instead of a release-mode no-opBOOST_ASSERT.edges(..., numpy_upper=true)usedstd::nextafter(x, std::numeric_limits<double>::min()), which moves toward ~0 — wrong for negative/zero upper edges (Regular(4, -2, -1).to_numpy()reported a last edge above -1). Now nudges towardlowest().py::isinstance<int>(x)is always false, so the int fast path inaxis_cast<int>was dead and large Python ints went through a lossy double round-trip. Now usespy::isinstance<py::int_>with a directpy::cast<int>(overflow raises).__deepcopy__lambdas for axes and histograms leaked the freshlynew-ed object ifcopy.deepcopy(metadata)threw; they now construct viastd::make_uniqueand return the holder.Regression tests added for: Fortran-order / negative-stride / strided
IntCategory.indexandfill; strided / reversed (str and bytes)StrCategory.indexandfill; theto_numpyupper-edge nudge direction for negative, zero, and positive edges; and corrupted pickle states (truncated buffer for weight and mean storages, non-array storage element).🤖 Generated with Claude Code