gh-107137: Add _PyTupleBuilder API to the internal C API

[vstinner]

Add _PyTupleBuilder structure and functions:

• _PyTupleBuilder_Init()
• _PyTupleBuilder_Alloc()
• _PyTupleBuilder_Append()
• _PyTupleBuilder_AppendUnsafe()
• _PyTupleBuilder_Finish()
• _PyTupleBuilder_Dealloc()

The builder tracks the size of the tuple and resize it in _PyTupleBuilder_Finish() if needed. Don't allocate empty tuple. Allocate an array of 16 objects on the stack to avoid allocating small tuple. _PyTupleBuilder_Append() overallocates the tuple by 25% to reduce the number of _PyTuple_Resize() calls.

Do no track the temporary internal tuple by the GC before _PyTupleBuilder_Finish() creates the final complete and consistent tuple object.

Use _PyTupleBuilder API in itertools batched_traverse(), PySequence_Tuple() and initialize_structseq_dict().

Add also helper functions:

• _PyTuple_ResizeNoTrack()
• _PyTuple_NewNoTrack()

• Issue: C API: Add internal C API to build a tuple: _PyTupleBuilder #107137

[vstinner]

I chose to use unsigned size_t in the structure in _PyTupleBuilder_Alloc() size argument. I'm not sure if it's better or worse than classic signed Py_ssize_t type.

cc @serhiy-storchaka @methane @pablogsal @erlend-aasland @corona10

[vstinner]

HPy API to build a tuples and lists: https://docs.hpyproject.org/en/latest/api-reference/builder.html HPy API seems to only support creating tuple of a size known in advance: it's not possible to extend or shrink the tuple.

[vstinner]

I'm considering to add a "Set" function later, and/or maybe a "GetUnsafe" function. But I prefer to start with a minimum API :-)

[gvanrossum]

-1. This feels unnecessarily elaborate.

[vstinner]

This API is a fix for this old issue: #59313 "Incomplete tuple created by PyTuple_New() and accessed via the GC can trigged a crash" which was closed as "not a bug" in 2021 by @pablogsal.

[vstinner]

@gvanrossum:

-1. This feels unnecessarily elaborate.

Do you mean that capi-workgroup/problems#56 is not an issue, or that this API is too complicated to use?

[gvanrossum]

I believe we should review the problem before jumping to a solution. And if this is the solution, I'm not sure that it's worth fixing the problem. So please hold your horses here.

[vstinner]

The root issue is that PyTuple_New() tracks directly the tuple by the GC. Moreover, _PyTuple_Resize() tracks also the tuple by the GC. My PR adds _PyTuple_NewNoTrack() and _PyTuple_ResizeNoTrack() to avoid this issue.

The _PyTupleBuilder is built on top of it to wrap the memory allocations.

[vstinner]

PR rebased to fix a merge conflict.

[gvanrossum]

All I am asking is that you hold off for now.

[vstinner]

I marked this PR as a draft.

[corona10]

Guido, even if there is a better way to solve this issue, adding the internal private API doesn't look harmful.
If we decide to adopt a better solution in the future, we can replace the API with a better one at any time.

[gvanrossum]

@corona10 I worry that APIs are forever, even internal ones. So I recommend that we have a discussion somewhere so we're all on the same page about the problem we're trying to solve and then we can how to solve it, rather than jumping the gun. (I don't require unanimity, just more people having thought about it and come to roughly the same conclusion than just Victor.)

[corona10]

So I recommend that we have a discussion somewhere so we're all on the same page about the problem we're trying to solve and then we can how to solve it, rather than jumping the gun

Thank you for the answer. CPython seems to be on the brink of change in many topics.
All topics are very controversial right now, and the time looks like some core team members have to stop everything for a while and watch. But I hope that the time is not that long :)

[vstinner]

In the main Python branch, you can still crash PySequence_Tuple() because it creates a temporary tuple with PyTuple_New() which is immediately tracked by the GC:

import gc
TAG = object()

def monitor():
    lst = [x for x in gc.get_referrers(TAG)
           if isinstance(x, tuple)]
    t = lst[0]   # this *is* the result tuple
    print(t)     # full of nulls !
    return t     # Keep it alive for some time

def my_iter():
    yield TAG    # 'tag' gets stored in the result tuple
    t = monitor()
    for x in range(10):
        yield x  # SystemError when the tuple needs to be resized

tuple(my_iter())

code from: #59313 (comment)

Program in gdb:

vstinner@mona$ gdb -args ./python x.py 
(...)

(<object object at 0x7fffea574af0>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>)

Breakpoint 1, _PyTuple_Resize (pv=0x7fffffffb8b0, newsize=25)
    at Objects/tupleobject.c:910
910	        PyErr_BadInternalCall();

(gdb) up
#1  0x00000000004cd05e in PySequence_Tuple (
    v=<generator at remote 0x7fffea585a90>) at Objects/abstract.c:2135
2135	            if (_PyTuple_Resize(&result, n) != 0) {

The crash occurs in _PyTuple_Resize(): that's why I added no only _PyTuple_NewNoTrack(), but also _PyTuple_ResizeNoTrack().

The problem is that there is a second strong reference to the tuple (created by monitor()): (Py_SIZE(v) != 0 && Py_REFCNT(v) != 1) test failed in _PyTuple_Resize().

[vstinner]

In the main Python branch, you can still crash PySequence_Tuple() because it creates a temporary tuple with PyTuple_New() which is immediately tracked by the GC

This PR fix this bug.

With this PR, monitor() cannot fail the tuple anymore, since it's no longer tracked by the GC while the tuple is being filled, and so it's no longer possible to crash Python in PySequence_Tuple().

monitor() fails at:

Traceback (most recent call last):
  File "/home/vstinner/python/main/x.py", line 17, in <module>
    tuple(my_iter())
  File "/home/vstinner/python/main/x.py", line 13, in my_iter
    t = monitor()
        ^^^^^^^^^
  File "/home/vstinner/python/main/x.py", line 7, in monitor
    t = lst[0]   # this *is* the result tuple
        ~~~^^^
IndexError: list index out of range

[gvanrossum]

@vstinner Please stop pushing. We've lived with this forever. It doesn't have to be fixed today.

[vstinner]

I extracted the non-controversial part of this PR, only _PyTuple_NewNoTrack() and _PyTuple_ResizeNoTrack(), in a new PR fixing the PySequence_Tuple() crash: PR #107183.

[rhettinger]

Can tuples be made robust enough to survive being called while partially filled? The tuple_dealloc code already uses Py_XDECREF to support NULL elements. Perhaps the other tuple methods could be similarly fortified.

If not, tools like itertools.batched still have another reasonable defense against the likes of gc.get_referrers(). When PyTuple_New(n) is called, it can be immediately filled with Py_None objects. Then as new data arrives, it can be swapped in. That way the tuple is always in a consistent state even if accessed by GC before completion.

[gvanrossum]

Raymond's idea of filling with None is more viable than it used to be since None is now immortal (as of 3.12) so we won't need to worry about its refcount. Still, I worry that there might be C code that creates a new tuple and somehow relies on the items being NULL.

[vstinner]

It seems like issues solved by the proposed _PyTupleBuilder API have different solutions discussed at PR #107183. This API doesn't seem to be the preferred option, so I prefer to close my PR and investigate other options first.

[vstinner]

Follow-up: I created issue #111489 to make _PyTuple_FromArraySteal() and _PyList_FromArraySteal() functions public.