gh-109496: Detect Py_DECREF() after dealloc in debug mode (#109539)

On a Python built in debug mode, Py_DECREF() now calls
_Py_NegativeRefcount() if the object is a dangling pointer to
deallocated memory: memory filled with 0xDD "dead byte" by the debug
hook on memory allocators. The fix is to check the reference count
*before* checking for _Py_IsImmortal().

Add test_decref_freed_object() to test_capi.test_misc.

diff --git a/Include/object.h b/Include/object.h
index 5bcb3937b8c8965808f369de2c41e6cf95790a4f..9058558e3cd4d9659cfdf68ea499e1462fb6af62 100644 (file)
--- a/Include/object.h
+++ b/Include/object.h
@@ -660,17 +660,15 @@ static inline void Py_DECREF(PyObject *op) {
 #elif defined(Py_REF_DEBUG)
 static inline void Py_DECREF(const char *filename, int lineno, PyObject *op)
 {
+    if (op->ob_refcnt <= 0) {
+        _Py_NegativeRefcount(filename, lineno, op);
+    }
     if (_Py_IsImmortal(op)) {
         return;
     }
     _Py_DECREF_STAT_INC();
     _Py_DECREF_DecRefTotal();
-    if (--op->ob_refcnt != 0) {
-        if (op->ob_refcnt < 0) {
-            _Py_NegativeRefcount(filename, lineno, op);
-        }
-    }
-    else {
+    if (--op->ob_refcnt == 0) {
         _Py_Dealloc(op);
     }
 }

diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py
index c9cc76a4c80ae8c265ee561d90654a843d64f949..57edacbae35634cc3dcd933270a6b24b72f1cee9 100644 (file)
--- a/Lib/test/test_capi/test_misc.py
+++ b/Lib/test/test_capi/test_misc.py
@@ -301,24 +301,40 @@ class CAPITest(unittest.TestCase):
     def test_buildvalue_N(self):
         _testcapi.test_buildvalue_N()
 
-    @unittest.skipUnless(hasattr(_testcapi, 'negative_refcount'),
-                         'need _testcapi.negative_refcount')
-    def test_negative_refcount(self):
+    def check_negative_refcount(self, code):
         # bpo-35059: Check that Py_DECREF() reports the correct filename
         # when calling _Py_NegativeRefcount() to abort Python.
-        code = textwrap.dedent("""
-            import _testcapi
-            from test import support
-
-            with support.SuppressCrashReport():
-                _testcapi.negative_refcount()
-        """)
+        code = textwrap.dedent(code)
         rc, out, err = assert_python_failure('-c', code)
         self.assertRegex(err,
                          br'_testcapimodule\.c:[0-9]+: '
                          br'_Py_NegativeRefcount: Assertion failed: '
                          br'object has negative ref count')
 
+    @unittest.skipUnless(hasattr(_testcapi, 'negative_refcount'),
+                         'need _testcapi.negative_refcount()')
+    def test_negative_refcount(self):
+        code = """
+            import _testcapi
+            from test import support
+
+            with support.SuppressCrashReport():
+                _testcapi.negative_refcount()
+        """
+        self.check_negative_refcount(code)
+
+    @unittest.skipUnless(hasattr(_testcapi, 'decref_freed_object'),
+                         'need _testcapi.decref_freed_object()')
+    def test_decref_freed_object(self):
+        code = """
+            import _testcapi
+            from test import support
+
+            with support.SuppressCrashReport():
+                _testcapi.decref_freed_object()
+        """
+        self.check_negative_refcount(code)
+
     def test_trashcan_subclass(self):
         # bpo-35983: Check that the trashcan mechanism for "list" is NOT
         # activated when its tp_dealloc is being called by a subclass

diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-18-15-35-08.gh-issue-109496.Kleoz3.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-18-15-35-08.gh-issue-109496.Kleoz3.rst
new file mode 100644 (file)
index 0000000..51b2144
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-18-15-35-08.gh-issue-109496.Kleoz3.rst
@@ -0,0 +1,5 @@
+On a Python built in debug mode, :c:func:`Py_DECREF()` now calls
+``_Py_NegativeRefcount()`` if the object is a dangling pointer to
+deallocated memory: memory filled with ``0xDD`` "dead byte" by the debug
+hook on memory allocators. The fix is to check the reference count *before*
+checking for ``_Py_IsImmortal()``. Patch by Victor Stinner.

diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
index 85d8401435e1b5aa4ec16ce0038776e200e33c74..f356fc5a6a016e1b7ced11be398ced71d9cfdabc 100644 (file)
--- a/Modules/_testcapimodule.c
+++ b/Modules/_testcapimodule.c
@@ -2034,6 +2034,26 @@ negative_refcount(PyObject *self, PyObject *Py_UNUSED(args))
 
     Py_RETURN_NONE;
 }
+
+static PyObject *
+decref_freed_object(PyObject *self, PyObject *Py_UNUSED(args))
+{
+    PyObject *obj = PyUnicode_FromString("decref_freed_object");
+    if (obj == NULL) {
+        return NULL;
+    }
+    assert(Py_REFCNT(obj) == 1);
+
+    // Deallocate the memory
+    Py_DECREF(obj);
+    // obj is a now a dangling pointer
+
+    // gh-109496: If Python is built in debug mode, Py_DECREF() must call
+    // _Py_NegativeRefcount() and abort Python.
+    Py_DECREF(obj);
+
+    Py_RETURN_NONE;
+}
 #endif
 
 
@@ -3299,6 +3319,7 @@ static PyMethodDef TestMethods[] = {
     {"bad_get", _PyCFunction_CAST(bad_get), METH_FASTCALL},
 #ifdef Py_REF_DEBUG
     {"negative_refcount", negative_refcount, METH_NOARGS},
+    {"decref_freed_object", decref_freed_object, METH_NOARGS},
 #endif
     {"meth_varargs", meth_varargs, METH_VARARGS},
     {"meth_varargs_keywords", _PyCFunction_CAST(meth_varargs_keywords), METH_VARARGS|METH_KEYWORDS},