Repository navigation
Assertion failure when func_repr is called on an already tp_clear-ed object #91636
Description
Activity
@pitrou and @pablogsal are listed at https://devguide.python.org/experts/ under "gc".
Bisected to here @methane:
3c45240 is the first bad commit
commit 3c45240
Author: INADA Naoki methane@users.noreply.github.com
Date: Wed Jul 4 11:15:50 2018 +0900bpo-33418: Add tp_clear for function object (GH-8058) Without tp_clear, GC can't break cyclic reference. It will cause memory leak when cyclic reference is created intentionally.Hm. this is wider problem. I think many more types are not safe after tp_clear...
This may be quite a widespread problem given how often
tp_clearis used as a proxy totp_dealloc:(Yes. This would be widespread problem. I quickly find two issues:
module_getattroaccessesm->md_dictwhile it would be cleared.cm_repraccessescm->cm_callable.
It's probably worthwhile fixing those
tp_clearmethods to not break invariants, then. Perhaps at least set the cleared fields toNoneso that we don't get crashes when another method such astp_repris invoked?set the cleared fields to
NoneThen we need to also modify
tp_deallocto also drop the reference toNoneI think changing
tp_clearmethods so that objects are usable after calling it is the not correct approach.The issue with the code above is that the weakref created in the
__del__method got excluded from weakrefs being cleared beforedelete_garbage()is called (which callstp_clear). No Python code is supposed to have access to an object that hastp_clearcalled on it.GH-136189 should make it so the example code doesn't crash, even with the old function tp_clear logic. I didn't actually test that but it should be so. For the
test_function_tp_clear_leaves_consistent_state()case intest_gc.py, thefuncglobal is now None. That means that you can't get access to the function after it was cleared, which is how it should be.This flaw in the GC existed for a long time, I think. Probably since weakrefs were first introduced actually.
Reacted by Inada Naoki, Sergey Miryanov and Mikhail EfimovReacted by Gregory P. SmithI believe that #91651 hides the real problem and we should apply solution from @nascheme.
With following change:➜ git diff diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index b4cbfb6d774..842943317e1 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -278,7 +278,7 @@ def __del__(self): latefin = LateFin() def func(): - pass + print('x') cyc = tuple.__new__(Cyclic, (func, latefin)) # 1. Create a reference cycle of `cyc` and `func`. @@ -296,6 +296,7 @@ def func(): # 9. Previously, this would crash because `func_qualname` # had been NULL-ed out by func_clear(). print(f"{func=}") + func() """ # We're mostly just checking that this doesn't crash. rc, stdout, stderr = assert_python_ok("-c", code)
I got segfault on current main:
====================================================================== FAIL: test_function_tp_clear_leaves_consistent_state (test.test_gc.GCTests.test_function_tp_clear_leaves_consistent_state) ---------------------------------------------------------------------- Traceback (most recent call last): File "D:\Sources\_pythonish\cpython\main\Lib\test\test_gc.py", line 302, in test_function_tp_clear_leaves_consistent_state rc, stdout, stderr = assert_python_ok("-c", code) ~~~~~~~~~~~~~~~~^^^^^^^^^^^^ File "D:\Sources\_pythonish\cpython\main\Lib\test\support\script_helper.py", line 182, in assert_python_ok return _assert_python(True, *args, **env_vars) File "D:\Sources\_pythonish\cpython\main\Lib\test\support\script_helper.py", line 167, in _assert_python res.fail(cmd_line) ~~~~~~~~^^^^^^^^^^ File "D:\Sources\_pythonish\cpython\main\Lib\test\support\script_helper.py", line 80, in fail raise AssertionError(f"Process return code is {exitcode}\n" ...<10 lines>... f"---") AssertionError: Process return code is 3221226529 command line: ['D:\\Sources\\_pythonish\\cpython\\main\\PCbuild\\amd64\\python_d.exe', '-X', 'faulthandler', '-c', 'if 1:\n\n import gc\n import weakref\n\n class LateFin:\n __slots__ = (\'ref\',)\n\n def __del__(self):\n\n # 8. Now `latefin`\'s finalizer is called. Here we\n # obtain a reference to `func`, which is currently\n # undergoing `tp_clear`.\n global func\n func = self.ref()\n\n class Cyclic(tuple):\n __slots__ = ()\n\n # 4. The finalizers of all garbage objects are called. In\n # this case this is only us as `func` doesn\'t have a\n # finalizer.\n def __del__(self):\n\n # 5. Create a weakref to `func` now. If we had created\n # it earlier, it would have been cleared by the\n # garbage collector before calling the finalizers.\n self[1].ref = weakref.ref(self[0])\n\n # 6. Drop the global reference to `latefin`. The only\n # remaining reference is the one we have.\n global latefin\n del latefin\n\n # 7. Now `func` is `tp_clear`-ed. This drops the last\n # reference to `Cyclic`, which gets `tp_dealloc`-ed.\n # This drops the last reference to `latefin`.\n\n latefin = LateFin()\n def func():\n print(\'x\')\n cyc = tuple.__new__(Cyclic, (func, latefin))\n\n # 1. Create a reference cycle of `cyc` and `func`.\n func.__module__ = cyc\n\n # 2. Make the cycle unreachable, but keep the global reference\n # to `latefin` so that it isn\'t detected as garbage. This\n # way its finalizer will not be called immediately.\n del func, cyc\n\n # 3. Invoke garbage collection,\n # which will find `cyc` and `func` as garbage.\n gc.collect()\n\n # 9. Previously, this would crash because `func_qualname`\n # had been NULL-ed out by func_clear().\n print(f"{func=}")\n func()\n '] stdout: --- --- stderr: --- Windows fatal exception: access violation Current thread 0x00007058 (most recent call first): File "<string>", line 41 in func File "<string>", line 59 in <module> Current thread's C stack trace (most recent call first): <cannot get C stack on this system> Windows fatal exception: code 0x80000003 Current thread 0x00007058 (most recent call first): File "<string>", line 41 in func File "<string>", line 59 in <module> Current thread's C stack trace (most recent call first): <cannot get C stack on this system> ---
So even if we "resurrect" an object it is not a valid object. Moreover, in general case, we can't even determine how broken it is.ff
Reacted by Mikhail EfimovI believe this can be closed now since #136401 was merged.
8 remaining items
It was not backported to 3.13 yet.
and it needs re-backporting to 3.14
Crash report
It is possible to resurrect a
tp_clear-ed object using pure python. The following piece of code illustrates how:Your environment
Linked PRs