GH-59313: Do not allow incomplete tuples, or tuples with NULLs. by markshannon · Pull Request #117747 · python/cpython
Conversation
Prevents gc.get_referrers from returning invalid tuples, and should make the cycle GC a bit more robust.
Not using NULL allows some efficiency improvements, like changing XDECREF to DECREF.
I've also streamlined tuple_alloc, since I was fiddling with tuple creation and destruction anyway.
PySequence_Tuple takes 0.02% of the runtime of the benchmark suite, so any change to its performance will be undetectable.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please restrict changes to the bare minimum fix for gh-59313, to ease backport? It would be better to move other changes (ex: reject NULL) in a separated PR.
| for x in range(10): | ||
| yield x # SystemError when the tuple needs to be resized | ||
|
|
||
| with self.assertRaises(IndexError): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
def my_iter(): yield TAG # 'tag' gets stored in the result tuple yield [x for x in gc.get_referrers(TAG) if isinstance(x, tuple)] for x in range(10): yield x result = tuple(my_iter()) self.assertEqual(result, (TAG, [], *range(10)))
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| PyObject * | ||
| _PyTuple_FromArraySteal(PyObject *const *src, Py_ssize_t n) | ||
| _PyTuple_FromNonEmptyArraySteal(PyObject *const *src, Py_ssize_t n) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this change is related to the fix. And I don't see the benefits to reject n=0.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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));
| static inline void | ||
| PyTuple_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) { | ||
| PyTupleObject *tuple = _PyTuple_CAST(op); | ||
| assert(value != NULL); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| (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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -2040,8 +2040,6 @@ PySequence_Tuple(PyObject *v) | |||
| { | |||
| PyObject *it; /* iter(v) */ | |||
| Py_ssize_t n; /* guess for result tuple size */ | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a warning: unused variable ‘n’ [-Wunused-variable]
Overall, the change looks good to me, but I made a second review with more suggestions.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits
|
|
||
| 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``. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This change seems to break weakrefs.
I suspect that None and NULL are semantically different, so changing NULL to None in the tuples used in the weakref code changes the behavior.
Closing until I have time to investigate and fix it.
It is possible that changing from NULL to None in PyTuple_New will break existing extensions.
The change to PySequence_Tuple has been moved to #127758