Use a critical section to protect getlist()

[lazerg]

Fixes #9852. Alternative to #9854

getlist() reads the sequence length once, then walks the list with PySequence_Fast_GET_ITEM, which does no bounds checking and no locking. On a free-threaded build, another thread shrinking the list during the walk makes the remaining reads go past the end of the storage, so Image.point() segfaults. Wrapping the walk in a critical section on the argument keeps the length valid for the duration, the same approach used for FontObject in #9498.

[espressolee]

While you're in getlist() — the critical section closes the free-threading race, but there's a second, unrelated bug in the same function that it can't reach, because it isn't a race at all.

n comes from the caller's __len__, and seq is built by iterating, so for a non-list they need not agree:

n = PySequence_Size(arg);          /* -> __len__ */
...
seq = PySequence_Fast(arg, must_be_sequence);   /* iterates; may be shorter */
for (i = 0; i < n; i++) {
    op = PySequence_Fast_GET_ITEM(seq, i);      /* reads past the end when n > len(seq) */

Deterministic, single-threaded, GIL on:

class Lying:
    def __len__(self): return 256          # claims 256
    def __getitem__(self, i):              # yields 8, then stops
        if i >= 8: raise IndexError
        return float(i)

Image.new("L", (4, 4)).point(Lying(), "F")

Image.point passes the sequence straight through for mode "F" (Image.py only materialises it with a list comprehension when the mode differs), so getlist() sees the original object. The same path is reachable via ImageFilter.Kernel, Image.transform and the JPEG qtables.

Clamping the loop to the sequence actually returned fixes it:

Py_ssize_t seq_n = PySequence_Fast_GET_SIZE(seq);
if (n > seq_n) { n = seq_n; }

Not raising this as a security issue — the trigger is an object the caller supplies, so anything that can reach it is already running Python in the process. Flagging it because a critical section around the existing loop would leave it in place, and it seemed worth having in front of you while the function is open.

[lazerg]

Thanks, you're right about point(). On a GIL-enabled 3.14 build, single threaded, your example segfaults for me every time. Image.transform() goes the same way if the object's __getitem__ also handles the data[:8] slice that Image.py takes first.

Two corrections on the other paths. ImageFilter.Kernel does read past the end, but here it surfaced as TypeError: bad argument type for built-in operation rather than a crash, because the slot past the end happened to read as NULL and PyFloat_AsDouble() rejects that. The JPEG qtables path isn't reachable: validate_qtables() does list(array.array("H", table)) on each table, so C only ever sees a real list.

On the fix, I'd rather not clamp. In _point() the caller declared 256 entries and the length check passed on the lie, so clamping hands them a LUT that is zero-filled from index 8 on and no error. Taking the length from seq and running the existing check against that reads better to me:

seq = PySequence_Fast(arg, must_be_sequence);
if (!seq) {
    return NULL;
}

n = PySequence_Fast_GET_SIZE(seq);
if (length && wrong_length && n != *length) {
    PyErr_SetString(PyExc_ValueError, wrong_length);
    Py_DECREF(seq);
    return NULL;
}

The calloc() then moves below this and the PySequence_Size() call goes away. The PySequence_Check() guard above it stays, since PySequence_Fast() on its own would happily accept a set.

I built that locally: your example now raises "wrong number of lut entries", the transform one raises "wrong number of matrix entries", Kernel reaches its own "bad kernel size" check, and the suite is unchanged at 4900 passed on 3.14t.

It is independent of the race, though. The critical section doesn't help here, and this doesn't close the race either, since a real list can still shrink between the size read and the loop. So I'd keep this PR on #9852 and raise the other one separately, unless a maintainer would rather have both here.

[espressolee]

Thanks for checking it properly — and you're right on both corrections.

The qtables path was my error. validate_qtables() does qtables[idx] = list(array.array("H", table)), and array.array iterates, so even a __len__ that lies its way past the len(table) != 64 check produces a real list of the true length before C sees anything. I shouldn't have listed it; it isn't reachable. And thank you for pinning Image.transform() to the case where __getitem__ also serves the data[:8] slice (Image.py:3022) — my note was looser than that.

Your fix is better than the clamp and I'd drop mine. The clamp silently hands back a zero-filled LUT and writes the clamped count through *length, so _point() gets 8 where it asked for 256, with no error anywhere. Taking n from PySequence_Fast_GET_SIZE(seq) runs the existing wrong_length check against reality and makes *length truthful, which is the behaviour the function already documents.

One thing that may sharpen the "independent of the race" point in your favour: the loop calls PyLong_AsLong() / PyFloat_AsDouble() on each item, and for a non-int/non-float those invoke __index__ / __float__ — arbitrary Python, inside the loop. And if that code blocks — I/O, or acquiring another lock — the critical section is suspended and its locks released, then reacquired afterwards (Doc/howto/free-threading-extensions.rst, "How Critical Sections Work"). So a size read once at the top isn't a bound you can rely on even under the section. Snapshotting into a private list, or re-reading the size each iteration, looks like the only thing that closes that, which is a third change again from either of these two.

Agreed on keeping this PR scoped to the race and #9852. Happy for you to raise the size one — you have the tested branch and the better patch, so it should be yours; I'll bring the reproducers over and drop the clamp from #9854 so there's nothing overlapping to review. Say the word if you'd rather I opened it instead.

[lazerg]

Thanks, and yes, please leave the size one to me. I'll raise it separately with the reproducers I already have, so nothing overlaps with #9854.

Your read of the docs is right, a critical section is suspended and its locks released if the thread blocks. In getlist() that only bites when the items are not int or float, because PyLong_AsLong() and PyFloat_AsDouble() then call __index__ or __float__ and that is arbitrary Python. For plain numeric items, which is the aliased-list case in #9852, nothing in the loop can run Python code or block, so the section holds for the whole walk. A snapshot would close the wider case, but as you say that is a third change, so this PR stays on the race.