GH-59313: Do not allow incomplete tuples, or tuples with NULLs.#117747
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Lib/test/test_tuple.py Outdated
| forxinrange(10): | ||
| yieldx# SystemError when the tuple needs to be resized | ||
| withself.assertRaises(IndexError): |
There was a problem hiding this comment.
Would it be possible to rewrite the test to return somehow [x for x in gc.get_referrers(TAG) if isinstance(x, tuple)], and make then check that it's empty?
Example:
defmy_iter(): yieldTAG# 'tag' gets stored in the result tupleyield [xforxingc.get_referrers(TAG) ifisinstance(x, tuple)] forxinrange(10): yieldxresult=tuple(my_iter()) self.assertEqual(result, (TAG, [], *range(10)))Objects/tupleobject.c Outdated
| PyObject* | ||
| _PyTuple_FromArraySteal(PyObject*const*src, Py_ssize_tn) | ||
| _PyTuple_FromNonEmptyArraySteal(PyObject*const*src, Py_ssize_tn) |
There was a problem hiding this comment.
I don't see how this change is related to the fix. And I don't see the benefits to reject n=0.
There was a problem hiding this comment.
I've reverted it.
_PyTuple_FromArraySteal is only called from the interpreter so we know that n != 0. I can change it in another PR.
vstinner left a comment
vstinner left a comment There was a problem hiding this comment.
You should also update _PyTuple_Resize():
/* Zero out items added by growing */if (newsize>oldsize) memset(&sv->ob_item[oldsize], 0, sizeof(*sv->ob_item) * (newsize-oldsize));| staticinlinevoid | ||
| PyTuple_SET_ITEM(PyObject*op, Py_ssize_tindex, PyObject*value){ | ||
| PyTupleObject*tuple=_PyTuple_CAST(op); | ||
| assert(value!=NULL); |
There was a problem hiding this comment.
You modified PyTuple_SET_ITEM() but not PyTuple_SetItem(), is it on purpose? I suggest to modify both, since PyTuple_SetItem() doesn't call PyTuple_SET_ITEM().
Doc/whatsnew/3.13.rst Outdated
| (Contributed by Victor Stinner in :gh:`108765`.) | ||
| * The :c:func:`PyTuple_SET_ITEM` inline function may not be passed ``NULL``. | ||
| This has always been the documented behavior, but was not enforced for |
There was a problem hiding this comment.
https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_SET_ITEM and https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_SetItem don't say that the second argument must not be NULL. I suggest to modify the doc to be extra explicit about it.
Objects/abstract.c Outdated
| PySequence_Tuple(PyObject*v) | ||
| { | ||
| PyObject*it; /* iter(v) */ | ||
| Py_ssize_tn; /* guess for result tuple size */ |
There was a problem hiding this comment.
There is a warning: unused variable ‘n’ [-Wunused-variable]
vstinner commented Apr 11, 2024
Overall, the change looks good to me, but I made a second review with more suggestions. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| Like :c:func:`PyTuple_SetItem`, but does no error checking, and should *only* be | ||
| used to fill in brand new tuples. | ||
| used to fill in brand new tuples. Both ``p`` and ``o`` must be non-``NULL``. |
There was a problem hiding this comment.
Can you move this addition to PyTuple_SetItem(), since PyTuple_SET_ITEM() is "like PyTuple_SetItem()"?
Or just copy/paste the sentence in PyTuple_SetItem() doc?
markshannon commented Apr 16, 2024
This change seems to break weakrefs. Closing until I have time to investigate and fix it. |
markshannon commented Dec 11, 2024
It is possible that changing from |
3 participants
Prevents
gc.get_referrersfrom returning invalid tuples, and should make the cycle GC a bit more robust.Not using
NULLallows some efficiency improvements, like changingXDECREFtoDECREF.I've also streamlined
tuple_alloc, since I was fiddling with tuple creation and destruction anyway.PySequence_Tupletakes 0.02% of the runtime of the benchmark suite, so any change to its performance will be undetectable.