| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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__.
Sorry, something went wrong.
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 __getitem__ that raises IndexError at random points doesn't crash after this fix. PySequence_Fast on an object without __iter__ falls back to old-style sequence iteration. That protocol treats IndexError from __getitem__ as the normal end-of-sequence signal, not a propagated exception. It just stops there and materializes whatever came before. test_overstated_length already exercises this, just with a fixed cutoff instead of a random one. The actual fix in this PR is that n now comes from PySequence_Fast_GET_SIZE(seq) after materialization, not from the caller's declared __len__. So it doesn't matter when IndexError fires. n always matches however many items got materialized. I ran 200 trials with __getitem__ raising IndexError at a 30% chance per index and got no crash or OOB read. If __getitem__ raises something other than IndexError, CPython propagates it as a real exception from PySequence_Fast, and getlist()/_putdata() already bail out cleanly through if (!seq) return NULL;. 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. |
Sorry, something went wrong.
|
I independently checked exact base 4e5f09f533dbd0d87d39951f48b62f4d1a421cba and current head 89654d1d68c4264f3d873af8dd3417347de73006. The core materialize-then-PySequence_Fast_GET_SIZE fix works: the original point/putdata/transform crash cases no longer signal, and the kernel path now raises ValueError rather than the old TypeError. I did find two regressions in the later PySequence_Size() fast pre-check at _imaging.c:445-452 and :1642-1648, relative to both #9892 materialized-length semantics and the PR first commit da633723d4f4715c58ba7290ddb98695f6861099.
The focused affected-file suite itself is green on current head: 333 passed on CPython 3.14.6 and 333 passed on CPython 3.14.0rc1t. Random IndexError from __getitem__ was also safe in 200/200 trials, and a non-IndexError from __getitem__ propagated normally. Those tests do not cover the two pre-check behaviours above. The minimal correction appears to be dropping the pre-checks and keeping the first commit shape: materialize once, derive n from the materialized object, then apply the existing exact/maximum-length checks. Honest oversized sequences are still rejected by the post-materialization check. This comment is scoped to #9892; I did not treat the separate concurrent-list race in #9852/#9853 as a regression of this PR. |
Sorry, something went wrong.
|
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 PySequence_Size() fails with TypeError, which is the unsized-sequence case it was meant to cover; anything else propagates. Your stateful sequence raises RuntimeError again from both point() and putdata(). Tests are in Tests/test_image_point.py::test_raising_length and Tests/test_image_putdata.py::test_raising_length. Both fail on 89654d1 and pass now. Point 1 I'd rather keep as is, because head matches main here instead of diverging from it. On base 4e5f09f:
So those three were already rejected before the PR. da633723 started accepting them as a side effect of moving the length check after materialization, and the pre-check put the old behaviour back. The pre-check also covers a second thing. Without it an honest oversized sequence is fully copied before it gets rejected: Image.new("L", (16, 16)).putdata(range(50_000_000)) measures 0.000s and 19MB peak RSS with the pre-check, against 0.682s and 2025MB at da633723. Dropping it trades that memory amplification for a permissiveness change, and #9892 asks for neither. The materialized size is still what the loop bound comes from, which is the part that closes the OOB read. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes #9892.
Changes proposed in this pull request: