Fix OOB read in getlist() and putdata() when __len__ overstates length - #9893
Fix OOB read in getlist() and putdata() when __len__ overstates length#9893lazerg wants to merge 4 commits into
Conversation
akx
left a comment
There was a problem hiding this comment.
I think the bigger issue is using PySequence_Fast things on custom types where __getitem__ can basically do whatever it likes...
I assume we'd see a similar crash for an object that raises IndexError randomly in __getitem__.
codecov/patch was failing at 64.71% because the self-review's cheap PySequence_Size() pre-check (added to reject honest-but-oversized sequences in O(1)) had no dedicated test: the "too many data entries" TypeError path in _putdata() and the "no __len__" fallback path in both getlist() and _putdata() (common for custom point-table-like objects) were completely untested anywhere in the suite. Add test_too_many_entries (putdata) and test_unsized_sequence (point, putdata) to close those gaps; mirrors the existing test_overstated_length convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R51WpQNzUqKreMEWv9TZtd
|
@akx I tested this directly: a
The actual fix in this PR is that If So this looks like the same bug class the PR already fixes, just probabilistic instead of fixed. Keeping this PR scoped to the reported OOB read. If you have a repro that still crashes on this branch, I'd like to see it. |
|
I independently checked exact base I did find two regressions in the later
The focused affected-file suite itself is green on current head: The minimal correction appears to be dropping the pre-checks and keeping the first commit shape: materialize once, derive |
|
Thanks for pinning it to exact commits, that made this quick to check. I rebuilt both and reproduced. Point 2 is a real bug and it's fixed in 5d24501. The pre-check now only clears the error when Point 1 I'd rather keep as is, because head matches
So those three were already rejected before the PR. The pre-check also covers a second thing. Without it an honest oversized sequence is fully copied before it gets rejected: |
Fixes #9892.
Changes proposed in this pull request:
getlist()and_putdata()read the sequence length before callingPySequence_Fast(). A custom sequence whose__len__reports more items than__getitem__actually produces makes the later loop read past the materialized list, which segfaults.PySequence_Fastresult instead, so they never index past what was actually built.