Skip to content

Not all types have "trashcan" protection and tp_dealloc can overflow stack #124715

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
Open
nascheme opened this issue Sep 27, 2024 · 1 comment
Open

Not all types have "trashcan" protection and tp_dealloc can overflow stack #124715

nascheme opened this issue Sep 27, 2024 · 1 comment
Labels
type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@nascheme
Copy link
Member

nascheme commented Sep 27, 2024 •
edited by bedevere-app bot
Loading

We occasionally fix these kinds of bugs, like GH-102356. However, the fix by adding the "trashcan" macros to the tp_dealloc method only fixes that specific type. It would be better to have a more generic fix. A crash due to the C stack overflow leaves the user mystified as to what the problem is and these kinds of bugs could be hard to reproduce (requiring long chains of objects calling tp_dealloc recursively).

Linked PRs
@nascheme nascheme added the type-crash A hard crash of the interpreter, possibly with a core dump label Sep 27, 2024
@bedevere-app bedevere-app bot mentioned this issue Sep 27, 2024
Closed
@bedevere-app bedevere-app bot mentioned this issue Apr 8, 2025
Merged
markshannon added a commit that referenced this issue Apr 30, 2025
@markshannon
GH-124715: Move trashcan mechanism into Py_Dealloc (GH-132280)
44e4c47
@bedevere-app bedevere-app bot mentioned this issue Apr 30, 2025
Merged
vstinner added a commit that referenced this issue May 1, 2025
@vstinner
gh-124715: Fix method_dealloc(): use PyObject_GC_UnTrack() (#133199)
662dd29 
Replace _PyObject_GC_UNTRACK() with PyObject_GC_UnTrack() to not fail
if the method was already untracked.
@nascheme
Copy link
Member Author

nascheme commented May 1, 2025

Regarding untracking, when I was working on my "integrate trashcan PR", I started wondering if _PyObject_GC_UNTRACK() should just be changed to check if the object is already untracked and not crash. I originally made it not safe to call twice because I wanted the tiny performance benefit (avoid the branch, I wanted GC support to have as little extra overhead as possible). However, in retrospect, I think it was a mistake. Knowing when it is safe to call _PyObject_GC_UNTRACK() in theory is possible but in practice it gets really painful. If you look at typeobject.c, it has to take great care to avoid _PyObject_GC_UNTRACK() from crashing.

Maybe we should profile this and see if it has any performance impact at all? There has been many bugs related to this over the years.

@vstinner vstinner mentioned this issue May 5, 2025
Merged
vstinner added a commit to vstinner/cpython that referenced this issue May 5, 2025
@vstinner
Revert "pythongh-124715: Fix method_dealloc(): use PyObject_GC_UnTrac…
7ce29b0 
…k() (python#133199)"

This reverts commit 662dd29.

The root issue was fixed by the
commit f554237.
@bedevere-app bedevere-app bot mentioned this issue May 5, 2025
Merged
vstinner added a commit that referenced this issue May 5, 2025
@vstinner
Revert "gh-124715: Fix method_dealloc(): use PyObject_GC_UnTrack() (#…
c336f1c 
…133199)" (#133434)

This reverts commit 662dd29.

The root issue was fixed by the
commit f554237.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@nascheme