| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I'm considering to add a "Set" function later, and/or maybe a "GetUnsafe" function. But I prefer to start with a minimum API :-) |
Sorry, something went wrong.
|
-1. This feels unnecessarily elaborate. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Do you mean that capi-workgroup/problems#56 is not an issue, or that this API is too complicated to use? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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()
|
PR rebased to fix a merge conflict. |
Sorry, something went wrong.
|
All I am asking is that you hold off for now. |
Sorry, something went wrong.
|
I marked this PR as a draft. |
Sorry, something went wrong.
|
Guido, even if there is a better way to solve this issue, adding the internal private API doesn't look harmful. |
Sorry, something went wrong.
|
@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.) |
Sorry, something went wrong.
Thank you for the answer. CPython seems to be on the brink of change in many topics. |
Sorry, something went wrong.
|
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(). |
Sorry, something went wrong.
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
|
Sorry, something went wrong.
|
@vstinner Please stop pushing. We've lived with this forever. It doesn't have to be fixed today. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Follow-up: I created issue #111489 to make _PyTuple_FromArraySteal() and _PyList_FromArraySteal() functions public. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Add _PyTupleBuilder structure and functions:
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: