Raise an exception if reading qtables fails - #9919
Conversation
|
I approve the changes to encode.c, but you will notice that the tests are failing on PyPy. |
|
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
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.
|
You are right, thanks. 222c5db copies both the outer sequence and each table with |
espressolee
left a comment
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:
- 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
ImageGrabfile: 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.
Fixes #9917.
get_qtables_arrays()validatedqtablesagainst the original object but read the items out of the list thatPySequence_Fast()built from it. For anything other than an exact list or tuple that means a second iteration, so asequence 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 andfor an individual table.
Both
PySequence_Fast()calls are now checked, the lengths come from the converted lists, andPyImaging_JpegEncoderNew()bails out when an exception is set instead of building the encoder anyway.