From e7f1faecfb0d20ac48ea28d77e13bf332dcd6c3b Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 6 Apr 2022 11:28:55 +0200 Subject: [PATCH] gh-89373: _Py_Dealloc() checks tp_dealloc exception If Python is built in debug mode, _Py_Dealloc() now ensures that the tp_dealloc function leaves the current exception unchanged. --- Doc/using/configure.rst | 1 + Lib/test/test_capi.py | 14 ++++++-- ...2-04-21-16-15-24.gh-issue-89373.A1jgLx.rst | 2 ++ Objects/object.c | 36 ++++++++++++++++++- 4 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-04-21-16-15-24.gh-issue-89373.A1jgLx.rst diff --git a/Doc/using/configure.rst b/Doc/using/configure.rst index 2e632d822f9bdb..e7efd2bbbc0a53 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 eb0edbf5a3a123..5f5d3512d1a0e3 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 00000000000000..56434f7e796690 --- /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 29880f6003bb1d..1144719c313e2a 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 }