Skip to content

gh-111777: Fix assertion errors on incorrectly still-tracked GC object destruction #111778

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.

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

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

Yhg1s
Copy link
Member

@Yhg1s Yhg1s commented Nov 6, 2023

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that were not properly untracked before starting destruction, take care to untrack the object before warning, to avoid triggering a GC run and causing the problem the code tries to warn about. Also make sure to save and restore any pending exceptions, which the warning would otherwise clobber or trigger an assertion error on.

were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

I would be great to have a test but I understand that's not easy to trigger so this is fine with me. (I am assuming you tested this locally @ Google)

@Yhg1s
Copy link
Member Author

Yhg1s commented Nov 12, 2023

Yeah, it's difficult to reproduce either real-world case. Deliberately setting an exception a custom tp_dealloc before calling PyObject_GC_Del is possible, but it's not really representative of the real case (the error would be set further up the call stack). The other case, with the warning triggering a GC run, I've completely failed to reproduce. It requires quite a complex interplay, apparently. I'm still debugging related issues, though, involving the untracking of GC objects, instances of heap types, subclassing in C, subclassing in Python, and custom tp_dealloc/tp_free, which has some subtle issues that elude me. I may be able to synthesise a good test in the end, but I think this fix is straight-forward enough to do without.

And yes, this fix has solved crashes in two different extension modules with the same problem at Google (in Py_DEBUG mode), without us having to fix the warnings. (We'll still fix the warnings, but at least this unblocks further testing.)

@Yhg1s Yhg1s merged commit ce6a533 into python:main Nov 12, 2023
@miss-islington-app
Copy link

Thanks @Yhg1s for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@Yhg1s Yhg1s deleted the pydebug_gc_del branch November 12, 2023 00:03
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 12, 2023
… object destruction (pythonGH-111778)

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that
were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
(cherry picked from commit ce6a533)

Co-authored-by: T. Wouters <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 12, 2023
… object destruction (pythonGH-111778)

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that
were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
(cherry picked from commit ce6a533)

Co-authored-by: T. Wouters <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 12, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 12, 2023
@bedevere-app
Copy link

bedevere-app bot commented Nov 12, 2023

GH-111990 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Nov 12, 2023
Yhg1s added a commit that referenced this pull request Nov 12, 2023
…C object destruction (GH-111778) (#111989)

gh-111777: Fix assertion errors on incorrectly still-tracked GC object destruction (GH-111778)

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that
were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
(cherry picked from commit ce6a533)

Co-authored-by: T. Wouters <[email protected]>
ambv pushed a commit that referenced this pull request Jan 17, 2024
…C object destruction (GH-111778) (GH-111990)

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that
were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
(cherry picked from commit ce6a533)

Co-authored-by: T. Wouters <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
… object destruction (python#111778)

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that
were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
… object destruction (python#111778)

In PyObject_GC_Del, in Py_DEBUG mode, when warning about GC objects that
were not properly untracked before starting destruction, take care to
untrack the object _before_ warning, to avoid triggering a GC run and
causing the problem the code tries to warn about. Also make sure to save and
restore any pending exceptions, which the warning would otherwise clobber or
trigger an assertion error on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants