[Python-checkins] gh-89373: _Py_Dealloc() checks tp_dealloc exception (#32357)

[vstinner]

https://github.com/python/cpython/commit/364ed9409269fb321dc4eafdea677c09a4bc0d8d
commit: 364ed9409269fb321dc4eafdea677c09a4bc0d8d
branch: main
author: Victor Stinner <vstinner at python.org>
committer: vstinner <vstinner at python.org>
date: 2022-04-21T23:04:01+02:00
summary:

gh-89373: _Py_Dealloc() checks tp_dealloc exception (#32357)

If Python is built in debug mode, _Py_Dealloc() now ensures that the
tp_dealloc function leaves the current exception unchanged.

files:
A Misc/NEWS.d/next/Core and Builtins/2022-04-21-16-15-24.gh-issue-89373.A1jgLx.rst
M Doc/using/configure.rst
M Lib/test/test_capi.py
M Objects/object.c

diff --git a/Doc/using/configure.rst b/Doc/using/configure.rst
index 2e632d822f9bd..e7efd2bbbc0a5 100644
--- a/Doc/using/configure.rst
+++ b/Doc/using/configure.rst
@@ -288,6 +288,7 @@ Effects of a debug build:
     to detect usage of uninitialized objects.
   * Ensure that functions which can clear or replace the current exception are
     not called with an exception raised.
+  * Check that deallocator functions don't change the current exception.
   * The garbage collector (:func:`gc.collect` function) runs some basic checks
     on objects consistency.
   * The :c:macro:`Py_SAFE_DOWNCAST()` macro checks for integer underflow and
diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index eb0edbf5a3a12..5f5d3512d1a0e 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -516,7 +516,12 @@ def __del__(self):
         del subclass_instance
 
         # Test that setting __class__ modified the reference counts of the types
-        self.assertEqual(type_refcnt - 1, B.refcnt_in_del)
+        if Py_DEBUG:
+            # gh-89373: In debug mode, _Py_Dealloc() keeps a strong reference
+            # to the type while calling tp_dealloc()
+            self.assertEqual(type_refcnt, B.refcnt_in_del)
+        else:
+            self.assertEqual(type_refcnt - 1, B.refcnt_in_del)
         self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del)
 
         # Test that the original type already has decreased its refcnt
@@ -581,7 +586,12 @@ def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_o
         del subclass_instance
 
         # Test that setting __class__ modified the reference counts of the types
-        self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del)
+        if Py_DEBUG:
+            # gh-89373: In debug mode, _Py_Dealloc() keeps a strong reference
+            # to the type while calling tp_dealloc()
+            self.assertEqual(type_refcnt, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del)
+        else:
+            self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del)
         self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del)
 
         # Test that the original type already has decreased its refcnt
diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-04-21-16-15-24.gh-issue-89373.A1jgLx.rst b/Misc/NEWS.d/next/Core and Builtins/2022-04-21-16-15-24.gh-issue-89373.A1jgLx.rst
new file mode 100644
index 0000000000000..56434f7e79669
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2022-04-21-16-15-24.gh-issue-89373.A1jgLx.rst	
@@ -0,0 +1,2 @@
+If Python is built in debug mode, Python now ensures that deallocator
+functions leave the current exception unchanged. Patch by Victor Stinner.
diff --git a/Objects/object.c b/Objects/object.c
index 29880f6003bb1..1144719c313e2 100644
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -2354,11 +2354,45 @@ _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg,
 void
 _Py_Dealloc(PyObject *op)
 {
-    destructor dealloc = Py_TYPE(op)->tp_dealloc;
+    PyTypeObject *type = Py_TYPE(op);
+    destructor dealloc = type->tp_dealloc;
+#ifdef Py_DEBUG
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyObject *old_exc_type = tstate->curexc_type;
+    // Keep the old exception type alive to prevent undefined behavior
+    // on (tstate->curexc_type != old_exc_type) below
+    Py_XINCREF(old_exc_type);
+    // Make sure that type->tp_name remains valid
+    Py_INCREF(type);
+#endif
+
 #ifdef Py_TRACE_REFS
     _Py_ForgetReference(op);
 #endif
     (*dealloc)(op);
+
+#ifdef Py_DEBUG
+    // gh-89373: The tp_dealloc function must leave the current exception
+    // unchanged.
+    if (tstate->curexc_type != old_exc_type) {
+        const char *err;
+        if (old_exc_type == NULL) {
+            err = "Deallocator of type '%s' raised an exception";
+        }
+        else if (tstate->curexc_type == NULL) {
+            err = "Deallocator of type '%s' cleared the current exception";
+        }
+        else {
+            // It can happen if dealloc() normalized the current exception.
+            // A deallocator function must not change the current exception,
+            // not even normalize it.
+            err = "Deallocator of type '%s' overrode the current exception";
+        }
+        _Py_FatalErrorFormat(__func__, err, type->tp_name);
+    }
+    Py_XDECREF(old_exc_type);
+    Py_DECREF(type);
+#endif
 }