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

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

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.

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

Done

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.

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

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

Done

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.

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.

Some nits

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

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-app Bot added the awaiting core review label Apr 11, 2024

bedevere-app Bot 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 thread Objects/weakrefobject.c

Comment thread Objects/abstract.c Outdated

Comment thread Modules/itertoolsmodule.c Outdated

Comment thread Modules/itertoolsmodule.c

Comment thread Modules/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-app Bot 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

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

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

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

                 (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

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

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

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

Repository navigation

GH-59313: Do not allow incomplete tuples, or tuples with NULLs. - #117747

GH-59313: Do not allow incomplete tuples, or tuples with NULLs.#117747markshannon wants to merge 19 commits intopython:mainpython/cpython:mainfrom faster-cpython:safer-tuplesfaster-cpython/cpython:safer-tuplesCopy head branch name to clipboard

Conversation

markshannon commented Apr 11, 2024 • edited by bedevere-app Bot Loading 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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GH-59313: Do not allow incomplete tuples, or tuples with NULLs.#117747
markshannon wants to merge 19 commits into
python:mainfrom
faster-cpython:safer-tuples

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