Skip to content

Assertion errors in Py_DEBUG mode when forgetting to untrack GC objects #111777

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

Closed
Yhg1s opened this issue Nov 6, 2023 · 0 comments
Closed

Assertion errors in Py_DEBUG mode when forgetting to untrack GC objects #111777

Yhg1s opened this issue Nov 6, 2023 · 0 comments
Assignees
Labels
3.11 only security fixes 3.12 only security fixes 3.13 bugs and security fixes

Comments

@Yhg1s
Copy link
Member

Yhg1s commented Nov 6, 2023

This has been distilled from a crash we found at Google while upgrading to Python 3.11. It's a complicated setup and I have only been able to partially reproduce the issue, but I believe my solution is correct.

When extension types set Py_TPFLAGS_HAVE_GC they should call PyObject_GC_Untrack() before starting destruction of the object so that the GC module doesn't see partially destructed objects if it happens to trigger during it. Python 3.11 started warning when types didn't do this correctly:

cpython/Modules/gcmodule.c

Lines 2398 to 2407 in d78c872

if (_PyObject_GC_IS_TRACKED(op)) {
#ifdef Py_DEBUG
if (PyErr_WarnExplicitFormat(PyExc_ResourceWarning, "gc", 0,
"gc", NULL, "Object of type %s is not untracked before destruction",
((PyObject*)op)->ob_type->tp_name)) {
PyErr_WriteUnraisable(NULL);
}
#endif
gc_list_remove(g);
}

There's two subtle issues here: one is that Py_DECREF(), and thus object deallocation, can be called with a pending exception (it's not unreasonable or even uncommon to do so). The PyErr_WarnExplicitFormat call, however, may (and likely will) call other Python code, which will trigger an assertion that no exception is pending, usually this one:

cpython/Objects/typeobject.c

Lines 4784 to 4785 in d78c872

/* We may end up clearing live exceptions below, so make sure it's ours. */
assert(!PyErr_Occurred());

The code should store any pending exception before calling anything that might care about the exception.

The other subtle issue is that the code warns before untracking the object. The warning (which can call arbitrary Python code via warnings.showwarning) can easily trigger a GC run, which then turns up seeing the still-erroneously-tracked object in an invalid state. The act of warning about a potential issue is triggering the issue (but only with Py_DEBUG defined). As far as I can see there's no reason not to untrack before raising the warning, which would avoid the problem. I have a PR that fixes both these issues.

Linked PRs

@Yhg1s Yhg1s added 3.11 only security fixes 3.12 only security fixes 3.13 bugs and security fixes labels Nov 6, 2023
@Yhg1s Yhg1s self-assigned this Nov 6, 2023
Yhg1s added a commit that referenced this issue Nov 12, 2023
…t destruction (#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.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue 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 issue 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]>
@Yhg1s Yhg1s closed this as completed Nov 12, 2023
Yhg1s added a commit that referenced this issue 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 issue 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 issue 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 issue 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
Labels
3.11 only security fixes 3.12 only security fixes 3.13 bugs and security fixes
Projects
None yet
Development

No branches or pull requests

1 participant