FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix OOB read in getlist() and putdata() when __len__ overstates length by lazerg · Pull Request #9893 · python-pillow/Pillow · GitHub

Fix OOB read in getlist() and putdata() when __len__ overstates length - #9893

Open
lazerg wants to merge 4 commits into
python-pillow:mainfrom
lazerg:fix/issue-9892-getlist-putdata-oob-read
Open

Fix OOB read in getlist() and putdata() when __len__ overstates length#9893
lazerg wants to merge 4 commits into
python-pillow:mainfrom
lazerg:fix/issue-9892-getlist-putdata-oob-read

Conversation

lazerg commented Aug 23, 2026

Copy link
Copy Markdown

Fixes #9892.

Changes proposed in this pull request:

  • getlist() and _putdata() read the sequence length before calling PySequence_Fast(). A custom sequence whose __len__ reports more items than __getitem__ actually produces makes the later loop read past the materialized list, which segfaults.
  • Both functions now take the length from the materialized PySequence_Fast result instead, so they never index past what was actually built.

akx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lazerg commented Aug 24, 2026

Copy link
Copy Markdown
Author

@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.

Copy link
Copy Markdown

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.

  1. The pre-check makes caller-reported __len__ authoritative again. A point table that reports 257 or 255 but actually materializes exactly 256 items is rejected with ValueError. A putdata sequence that reports 17 but materializes 2 items is rejected as too large instead of writing those 2 items. Each result reproduced 3/3 on both CPython 3.14.6 (GIL) and 3.14.0rc1t (GIL disabled). At exact core-fix commit da633723, all three cases succeed 3/3 using the materialized size.

  2. PyErr_Clear() suppresses every PySequence_Size() failure, not only the intended unsized-sequence TypeError. With a stateful sequence whose first __len__ raises RuntimeError("len exploded") and whose later length call succeeds, current head silently clears the RuntimeError and completes point/putdata 3/3 on both runtimes. At da633723, point propagates the RuntimeError 3/3 and putdata follows its existing failure path rather than succeeding.

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.

lazerg commented Aug 28, 2026

Copy link
Copy Markdown
Author

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:

case 4e5f09f da63372 head
point, __len__ 257, 256 materialize ValueError ok ValueError
point, __len__ 255, 256 materialize ValueError ok ValueError
putdata, __len__ 17, 2 materialize TypeError ok TypeError

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.

Agreed on #9852/#9853 being separate.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getlist()/_putdata(): OOB read when __len__ overstates materialized items

3 participants


Back | FazBrowse Home | New Git URL