bpo-44184: Fix subtype_dealloc() for freed type

[vstinner]

Fix a crash at Python exit when a deallocator function removes the
last strong reference to a heap type.

Don't read type memory after calling basedealloc() since
basedealloc() can deallocate the type and free its memory.

https://bugs.python.org/issue44184

[vstinner]

cc @pablogsal @erlend-aasland

[pablogsal]

This fix makes no sense to me, if what you say in the comment was actually the root cause, we would be seeing this under vallgrind or the address sanitizer and I could not reproduce it

[bedevere-bot]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

I was able to reproduce the crash under ASAN: https://bugs.python.org/msg394085

[pablogsal]

This is enough to crash it:

import ast
import builtins
import sys
import os

tree = ast.parse("pass")
x = [tree]
x.append(x)

# Put the cycle somewhere to survive until the *last* GC collection
def callback(*args):
    pass
callback.a = x
os.register_at_fork(after_in_child=callback)

can we maybe add a test based on this?

[vstinner]

I would prefer to not rely directly on os.register_at_fork() to ensure that the object will survive until the last GC collection. I would prefer an abstraction function in the test.support module. I will try to add a test.

[erlend-aasland]

I can reproduce the bug on macOS using the address sanitiser. The patch also fixes the buildbot problems on #24203 (comment)

[vstinner]

@pablogsal: I added an unit test, would you mind to review the updated PR?

I was too lazy to add an abstraction. Since the test is in test_gc, IMO it's ok. First, I wanted to add the test to test_ast.

[vstinner]

Ah, register_at_fork() is not available on Windows. I modified my test to use codecs.register() instead.

You can test manually that the unit test fails as expected with this change:

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index af99ab79d1..acbedc64e6 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -5,6 +5,7 @@
 #include "pycore_compile.h"       // _Py_Mangle()
 #include "pycore_initconfig.h"
 #include "pycore_moduleobject.h"  // _PyModule_GetDef()
+#include "pycore_pymem.h"  // _PyModule_GetDef()
 #include "pycore_object.h"
 #include "pycore_pyerrors.h"
 #include "pycore_pystate.h"       // _PyThreadState_GET()
@@ -1459,6 +1460,7 @@ subtype_dealloc(PyObject *self)
        our type from a HEAPTYPE to a non-HEAPTYPE, so be careful about
        reference counting. Only decref if the base type is not already a heap
        allocated type. Otherwise, basedealloc should have decref'd it already */
+    assert(!_PyMem_IsPtrFreed(type->tp_name));
     if (type_needs_decref) {
         Py_DECREF(type);
     }

[erlend-aasland]

Looks good to me!

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

[bedevere-bot]

GH-26290 is a backport of this pull request to the 3.10 branch.

[vstinner]

Thanks for reviews @pablogsal and @erlend-aasland!

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-27176 is a backport of this pull request to the 3.9 branch.

[encukou]

For posterity: bpo-42961 reports the same issue and has a nice reproducer.