Fix security and correctness issues from #25 Array API - #40
Merged
Conversation
📊 Benchmark Comparison Results✅ All benchmarks within noise42 benchmarks compared, no significant changes detected. Legend: ✅ Faster (>5%) · |
- Fix Py_BuildValue format specifiers: use 'L' for int64_t and 'K' for uint64_t instead of 'i' which silently truncates 64-bit values - Fix signed integer overflow UB in best_within_dist when max_dist is large: use -1 sentinel instead of max_dist+1 - Replace new[]/delete[] with malloc/free + NULL check to prevent uncaught std::bad_alloc from crashing the process - Fix NEON bug: max_dist > 0 should be max_dist >= 0 so that max_dist=0 correctly delegates to the native path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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 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.
Follow-up to #25
A post-merge review of the Array API introduced in #25 uncovered four bugs — two of which silently corrupt return values on 64-bit systems and one that can crash the interpreter.
Fixes
Py_BuildValueformat truncation —"i"(32-bit C int) was used forint64_t/uint64_treturn values, silently truncating results on arrays with >2³¹ elements or large distances. Changed to"L"/"K"as appropriate.Signed integer overflow in
best_within_dist—best_dist = max_dist + 1is undefined behavior whenmax_distis large. Replaced with a-1sentinel pattern that avoids the overflow entirely.Uncaught
std::bad_allocinall_within_dist—new uint64_t[]on a large allocation would throw an uncaught C++ exception, callingstd::terminate()and killing the process. Replaced withmalloc+NULLcheck that returnsPyErr_NoMemory()gracefully.NEON
max_dist=0edge case —if (max_dist > 0)should beif (max_dist >= 0), causingcheck_bytes_within_dist(..., 0)to take the wrong code path on ARM and return incorrect results.Also includes
#include <cstdlib>formalloc/free