Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-115874: Don't use module state in teedataobject tp_dealloc #116204

Merged
merged 3 commits into from Mar 18, 2024
Merged

gh-115874: Don't use module state in teedataobject tp_dealloc #116204

merged 3 commits into from Mar 18, 2024
+12 −6

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Mar 1, 2024 •
edited by bedevere-app bot

@bedevere-app bedevere-app bot mentioned this pull request Mar 1, 2024
Closed
@erlend-aasland erlend-aasland added the needs backport to 3.12 bug and security fixes label Mar 1, 2024
@erlend-aasland
Copy link
Contributor Author

We could also add your repro as a regression test, @brandtbucher.

@brandtbucher
Copy link
Member

I think adding the test is a good idea. Maybe use one of the helpers that launches a new subprocess?

Honestly, I’m not sure why the safe dealloc function needs a type at all. It’s not possible to have any other type here, right? In fact, it’s basically just reinventing the existing trashcan mechanism.

Either one of two things is true: the type of a linked object can never be anything other than teedataobject_type, and the check is unnecessary (it’s not subclass-able, and the code goes to great lengths to make sure that other classes aren’t used here). Or it is possible to have another type of object here and the current code is incorrect (and we should just use the Py_TRASHCAN macros in the dealloc function).

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Mar 1, 2024 •
edited

Honestly, I’m not sure why the safe dealloc function needs a type at all. It’s not possible to have any other type here, right?

Well, it's not a basetype, so there is no risk of subtypes here. After a second (quick) look, I see no reason to use the module state here at all. I'll be able to have a closer look in a couple of hours. Feel free to continue working on this PR in the mean time if you want, Brandt.

We could just decref the linked list in the dealloc function; the teedataobject_safe_decref abstraction is not needed, and it is not really helping either IMO.

@erlend-aasland
Copy link
Contributor Author

FTR, here's the issue and commit that added the safe-decref helper:

@erlend-aasland
Copy link
Contributor Author

Using the Py_TRASHCAN macros should also be fine AFAIK, but I've never had to deal with them before so I'm on thin ice there :)

serhiy-storchaka
serhiy-storchaka approved these changes Mar 2, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

It is not obvious why the typing module is needed for tests. It may make the test unreliable. Otherwise LGTM.

@erlend-aasland
Copy link
Contributor Author

It is not obvious why the typing module is needed for tests. It may make the test unreliable. Otherwise LGTM.

It is not obvious for me either. @brandtbucher, could you add an explanatory comment?

@brandtbucher
Copy link
Member

brandtbucher commented Mar 2, 2024 •
edited

Away from my laptop for the weekend, but I’ll look when I get back.

Honestly, I think it may just have to do with the order in which cycles are created (and the order in which they, and the items within them, are cleared). typing creates a few cycles between itself and copyreg on import. @JelleZijlstra noted on the original issue that the bug goes away if those cycles aren’t created, even though the cycles themselves aren’t part of the problem.

@erlend-aasland
Copy link
Contributor Author

Perhaps the GitHub reference is enough for now.

erlend-aasland
erlend-aasland commented Mar 12, 2024
View reviewed changes
Lib/test/test_itertools.py
@@ -1699,6 +1699,14 @@ def test_tee(self):
self.pickletest(proto, a, compare=ans)
self.pickletest(proto, b, compare=ans)

def test_tee_dealloc_segfault(self):
# gh-115874: segfaults when accessing module state in tp_dealloc.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandtbucher, how about this:

Suggested change
# gh-115874: segfaults when accessing module state in tp_dealloc.
# gh-115874: Segfault when accessing module state in tp_dealloc.
# The repro depends on multiple ref cycles; by importing
# typing, we implicitly create ref cycles between typing
# and copyreg. Without these cycles, the reproducer does
# not work.

@erlend-aasland
Copy link
Contributor Author

Perhaps the GitHub reference is enough for now.

No-one chimed in, so we assume it is. If someone feels the urge to clarify the comment further, a follow-up PR can be made.

@erlend-aasland erlend-aasland merged commit e2fcaf1 into python:main Mar 18, 2024
34 checks passed
@erlend-aasland erlend-aasland deleted the fix-tp-dealloc-in-teedataobject branch March 18, 2024 12:24
@miss-islington-app
Copy link

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 18, 2024
@erlend-aasland @brandtbucher @miss-islington
pythongh-115874: Don't use module state in teedataobject tp_dealloc (p…
fb64bb0 
…ythonGH-116204)

(cherry picked from commit e2fcaf1)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>
@bedevere-app
Copy link

bedevere-app bot commented Mar 18, 2024

GH-116955 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Mar 18, 2024
erlend-aasland added a commit that referenced this pull request Mar 18, 2024
@miss-islington @erlend-aasland @brandtbucher
[3.12] gh-115874: Don't use module state in teedataobject tp_dealloc (G…
1c0c6c9 
…H-116204) (#116955)

(cherry picked from commit e2fcaf1)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>
@erlend-aasland erlend-aasland mentioned this pull request Mar 19, 2024
Closed
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
@erlend-aasland @brandtbucher @vstinner
pythongh-115874: Don't use module state in teedataobject tp_dealloc (p…
194671b 
…ython#116204)

Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@erlend-aasland @brandtbucher @adorilson
pythongh-115874: Don't use module state in teedataobject tp_dealloc (p…
fa75cdd 
…ython#116204)

Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@erlend-aasland @brandtbucher @diegorusso
pythongh-115874: Don't use module state in teedataobject tp_dealloc (p…
231a673 
…ython#116204)

Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@erlend-aasland @brandtbucher @serhiy-storchaka