| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
I approve the changes to encode.c, but you will notice that the tests are failing on PyPy. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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:
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.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.