GH-59313: Do not allow incomplete tuples, or tuples with NULLs. by markshannon · Pull Request #117747 · python/cpython

markshannon · 2024-04-11T09:15:46Z

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.

Issue: Incomplete tuple created by PyTuple_New() and accessed via the GC can trigged a crash #59313

vstinner

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.

vstinner · 2024-04-11T09:50:11Z

+ for x in range(10):
+ yield x # SystemError when the tuple needs to be resized
+
+ with self.assertRaises(IndexError):


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)))

vstinner · 2024-04-11T10:11:58Z


 PyObject *
-_PyTuple_FromArraySteal(PyObject *const *src, Py_ssize_t n)
+_PyTuple_FromNonEmptyArraySteal(PyObject *const *src, Py_ssize_t n)


I don't see how this change is related to the fix. And I don't see the benefits to reject n=0.

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

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));

vstinner · 2024-04-11T13:28:55Z

 static inline void
 PyTuple_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) {
 PyTupleObject *tuple = _PyTuple_CAST(op);
+ assert(value != NULL);


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().

vstinner · 2024-04-11T13:30:09Z

 (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


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.

vstinner · 2024-04-11T13:33:36Z

@@ -2040,8 +2040,6 @@ PySequence_Tuple(PyObject *v)
 {
 PyObject *it; /* iter(v) */
 Py_ssize_t n; /* guess for result tuple size */


There is a warning: unused variable ‘n’ [-Wunused-variable]

vstinner · 2024-04-11T13:34:43Z

Overall, the change looks good to me, but I made a second review with more suggestions.

erlend-aasland

Some nits

vstinner · 2024-04-15T09:07:51Z


 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``.


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 · 2024-04-16T13:02:42Z

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.

markshannon · 2024-12-11T11:52:57Z

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

markshannon added 4 commits April 10, 2024 18:30

More robust and maybe faster tuples
d4d6f24

Add test
6a1f806

Merge branch 'main' into safer-tuples
9aeb6da

Add news
40a49d6

markshannon requested review from gvanrossum and rhettinger as code owners April 11, 2024 09:15

bedevere-appBot added the awaiting core review label Apr 11, 2024

bedevere-appBot mentioned this pull request Apr 11, 2024
Incomplete tuple created by PyTuple_New() and accessed via the GC can trigged a crash #59313
Closed

markshannon mentioned this pull request Apr 11, 2024
gh-107137: Add _PyTuple_NewNoTrack() internal C API #107183
Closed

Don't leak references
0a06553

vstinner reviewed Apr 11, 2024
View reviewed changes

markshannon added 6 commits April 11, 2024 12:13

Revert changes to _PyTuple_FromArraySteal
207005c

Mark list as empty before transfering ownership of items
03643cf

Document change to PyTuple_SET_ITEM
0d8974e

Redo test
8fad5a4

Recommend using None
e4b4f39

Fix formatting
f4c0a29

vstinner reviewed Apr 11, 2024
View reviewed changes

erlend-aasland reviewed Apr 11, 2024
View reviewed changes

Comment threadObjects/weakrefobject.c
Comment threadObjects/abstract.c Outdated
Comment threadModules/itertoolsmodule.c Outdated
Comment threadModules/itertoolsmodule.c
Comment threadModules/itertoolsmodule.c

markshannon added 4 commits April 11, 2024 15:33

Improve thread safety of _PyList_AsTupleTakeItems
bf69fc5

Minor tweaks
4d0ad9e

Doc that the value cannot be NULL.
269832a

Handle the mathematically challenged function _PyTuple_Resize
3a766d8

rhettinger removed their request for review April 14, 2024 03:42

vstinner reviewed Apr 15, 2024
View reviewed changes

markshannon closed this Apr 16, 2024

vstinner mentioned this pull request Jun 10, 2024
Disallow creation of incomplete/inconsistent objects capi-workgroup/problems#56
Open

markshannon deleted the safer-tuples branch August 6, 2024 10:16

markshannon added 2 commits November 8, 2024 12:30

Merge branch 'main' into safer-tuples
003264f

Fix handling of zero and negative sizes
a155802

markshannon reopened this Nov 8, 2024

markshannon requested a review from kumaraditya303 as a code owner November 8, 2024 14:16

markshannon marked this pull request as draft November 8, 2024 14:17

bedevere-appBot removed the awaiting core review label Nov 8, 2024

markshannon added 2 commits November 8, 2024 14:23

Fix up docs
ae437f1

Move news
03ce121

markshannon closed this Dec 11, 2024

Uh oh!

Uh oh!

Conversation

markshannon commented Apr 11, 2024• edited by bedevere-app BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 11, 2024

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon commented Apr 16, 2024

Uh oh!

markshannon commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markshannon commented Apr 11, 2024•
edited by bedevere-app Bot
Loading