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

Raise an exception if reading qtables fails by lazerg · Pull Request #9919 · python-pillow/Pillow · GitHub

Raise an exception if reading qtables fails - #9919

Open
lazerg wants to merge 4 commits into
python-pillow:mainfrom
lazerg:fix/issue-9917-qtables-null
Open

Raise an exception if reading qtables fails#9919
lazerg wants to merge 4 commits into
python-pillow:mainfrom
lazerg:fix/issue-9917-qtables-null

Conversation

lazerg commented Aug 28, 2026
edited
Loading

Copy link
Copy Markdown

Fixes #9917.

get_qtables_arrays() validated qtables against the original object but read the items out of the list that
PySequence_Fast() built from it. For anything other than an exact list or tuple that means a second iteration, so a
sequence that raises the second time around returned NULL and the unchecked PySequence_Fast_GET_ITEM() segfaulted.
The same mismatch also let a lying __len__ walk off the end of the converted list, both for the outer sequence and
for an individual table.

Both PySequence_Fast() calls are now checked, the lengths come from the converted lists, and
PyImaging_JpegEncoderNew() bails out when an exception is set instead of building the encoder anyway.

Copy link
Copy Markdown
Member

I approve the changes to encode.c, but you will notice that the tests are failing on PyPy.

radarhere added the JPEG label Aug 28, 2026

lazerg commented Aug 28, 2026

Copy link
Copy Markdown
Author

On PyPy, PySequence_Fast does not iterate the list subclass a second time, so test_qtables_iteration_error cannot trigger the crash there. 15a7082 skips it with is_pypy(); I pushed that a couple of minutes after your comment. The three pypy3.11 jobs are green on that commit.

espressolee left a comment

Copy link
Copy Markdown

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 independently built the reported base 20510d4088ee3d42ad3433ff10dc9bf5293287d0 and exact head 15a7082e30181e0c45cddd0dd5819de9c43213bc with CPython 3.14.6 and 3.14.6t on macOS/arm64.

The intended fixes work: on the base, outer iteration failure, inner materialization failure, and an overstated outer length each reproduce as SIGSEGV; the head propagates the original exceptions or uses the materialized length. It also turns the stale SystemError paths for element-conversion failure and short inner materialization into the original RuntimeError/ValueError. The two new tests pass, Tests/test_file_jpeg.py is 104 passed / 1 skipped on both 3.14 and 3.14t, and the runnable local suite excluding the display-dependent ImageGrab file is 4900 passed / 271 skipped / 3 xfailed.

However, there is still a deterministic out-of-bounds read in the same second-read loop (src/encode.c:1139-1150). PySequence_Fast(table, ...) does not make a private snapshot when table is an exact list. PyLong_AS_LONG() may execute an element's __index__; if that hook clears the list, the next PySequence_Fast_GET_ITEM(table_data, j) dereferences beyond the now-empty list.

Minimal shape of the reproducer:

from io import BytesIO
from PIL import Image

class Item:
    def __index__(self):
        self.table.clear()
        return 1

class QTables(list):
    calls = 0
    def __iter__(self):
        self.calls += 1
        if self.calls == 1:  # validation pass
            return super().__iter__()
        item = Item()
        table = [item, *([1] * 63)]
        item.table = table
        return iter([table])

Image.new("RGB", (4, 4)).save(
    BytesIO(), format="JPEG", qtables=QTables([[1] * 64])
)

On the exact head this is SIGSEGV 5/5 with CPython 3.14.6 and 5/5 with 3.14.6t; matched ordinary-table controls are clean 5/5 in both builds. LLDB stops on EXC_BAD_ACCESS at address 0x8 in get_qtables_arrays, in the PySequence_Fast_GET_ITEM / PyLong_AsLong loop. The base also crashes, so this is not a regression introduced by the PR, but it is the same second-read path and means that reading qtables can still crash instead of raising.

A scratch proof that forced private snapshots for both the outer table collection and each inner table closed all 10 hostile cases on 3.14 and 3.14t, while retaining 104 passed / 1 skipped for Tests/test_file_jpeg.py in both builds. I am not prescribing that exact two-line implementation without considering error-message compatibility, but a regression for re-entrant element conversion and a snapshot/strong-reference strategy are needed before this path is complete.

lazerg commented Aug 28, 2026

Copy link
Copy Markdown
Author

You are right, thanks. PyLong_AS_LONG maps to PyLong_AsLong, which calls __index__ on a non-int element. PySequence_Fast returns the same object for an exact list, so a hook that clears the list leaves the loop reading freed storage. Your reproducer segfaults here too on the head commit.

222c5db copies both the outer sequence and each table with PySequence_List, so the loops walk private lists that nothing else can reach. I added test_qtables_cleared_while_reading as a regression test. It segfaults without the change and passes with it. Tests/test_file_jpeg.py is 105 passed and 1 skipped, and the full local suite is 4912 passed.

espressolee left a comment

Copy link
Copy Markdown

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

Re-reviewed exact head 222c5db3ecc89ff90106f93a9787e25209aba567 after the private-snapshot update.

I rebuilt that exact tree independently with CPython 3.14.6 and 3.14.6t on macOS/arm64. The new outer and inner PySequence_List snapshots close the reported re-entrant list-clear path:

  • self-clearing __index__ reproducer: 5/5 clean on 3.14 and 5/5 clean on 3.14t
  • matched ordinary-table controls: 5/5 clean on each runtime
  • separately synchronized mutator-thread clear: 5/5 clean on 3.14 and 5/5 clean on 3.14t
  • broader hostile sequence/error matrix: 10/10 clean on each runtime, with the original exceptions propagated
  • the three focused regression tests: 3/3 on each runtime
  • Tests/test_file_jpeg.py: 105 passed / 1 skipped on each runtime
  • runnable local suite excluding the display-dependent ImageGrab file: 4901 passed / 271 skipped / 3 xfailed
  • git diff --check: clean

I found no blocking issue in the measured scope. The remote matrix had only the docs and pre-commit statuses visible at the time of this review, so this approval is for the exact code and local test evidence above rather than a claim that all remote jobs have completed.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JPEG qtables iteration failure causes a NULL dereference in Image.save()

3 participants


Back | FazBrowse Home | New Git URL