GeezerVison

The World Wide Web, restored to 800 pixels of glory.

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

Conversation

markshannon commented Apr 11, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/cpython/tupleobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_tuple.py Outdated

		for x in range(10):
		yield x # SystemError when the tuple needs to be resized

		with self.assertRaises(IndexError):

vstinner Apr 11, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Apr 11, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/tupleobject.c Outdated


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

vstinner Apr 11, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Apr 11, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/cpython/tupleobject.h

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

vstinner Apr 11, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Apr 11, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

vstinner Apr 11, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/abstract.c Outdated

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

vstinner Apr 11, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner commented Apr 11, 2024

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reviewed

Apr 11, 2024

View reviewed changes

erlend-aasland left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/weakrefobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/abstract.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/itertoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/itertoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/itertoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

Doc/c-api/tuple.rst


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

vstinner Apr 15, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon commented Apr 16, 2024

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Dec 11, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon closed this

Dec 11, 2024

Reviewers

vstinner

vstinner left review comments

erlend-aasland

erlend-aasland left review comments

gvanrossum

Awaiting requested review from gvanrossum

kumaraditya303

Awaiting requested review from kumaraditya303

kumaraditya303 is a code owner

rhettinger

Awaiting requested review from rhettinger

rhettinger will be requested when the pull request is marked ready for review

rhettinger is a code owner

AA-Turner

Awaiting requested review from AA-Turner

AA-Turner will be requested when the pull request is marked ready for review

AA-Turner is a code owner

ZeroIntensity

Awaiting requested review from ZeroIntensity

ZeroIntensity will be requested when the pull request is marked ready for review

ZeroIntensity is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

markshannon commented Apr 11, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.