gh-130519: Fix crash in QSBR when destructor reenters QSBR (gh-130553)

The `free_work_item()` function in QSBR may call arbitrary code via
Python object destructors, which may reenter the QSBR code. Reorder
the processing of work items to be robust to reentrancy.

Also fix the TODO for the out of memory situation.

diff --git a/Include/internal/pycore_pymem.h b/Include/internal/pycore_pymem.h
index defe143e9dcc3c7ac34d05a2fc8f9bad2b9ab348..5386d4c5f830311a94de433c4725533987955bd8 100644 (file)
--- a/Include/internal/pycore_pymem.h
+++ b/Include/internal/pycore_pymem.h
@@ -121,7 +121,7 @@ extern void _PyMem_FreeDelayed(void *ptr);
 
 // Enqueue an object to be freed possibly after some delay
 #ifdef Py_GIL_DISABLED
-extern void _PyObject_XDecRefDelayed(PyObject *obj);
+PyAPI_FUNC(void) _PyObject_XDecRefDelayed(PyObject *obj);
 #else
 static inline void _PyObject_XDecRefDelayed(PyObject *obj)
 {

diff --git a/Lib/test/test_capi/test_object.py b/Lib/test/test_capi/test_object.py
index 5d0a383de64520ebe05659ee2e9fbed06804036b..3e8fd91b9a67a0eaed4cb0228f9f20f7c7e015f9 100644 (file)
--- a/Lib/test/test_capi/test_object.py
+++ b/Lib/test/test_capi/test_object.py
@@ -210,5 +210,18 @@ class CAPITest(unittest.TestCase):
         """
         self.check_negative_refcount(code)
 
+    def test_decref_delayed(self):
+        # gh-130519: Test that _PyObject_XDecRefDelayed() and QSBR code path
+        # handles destructors that are possibly re-entrant or trigger a GC.
+        import gc
+
+        class MyObj:
+            def __del__(self):
+                gc.collect()
+
+        for _ in range(1000):
+            obj = MyObj()
+            _testinternalcapi.incref_decref_delayed(obj)
+
 if __name__ == "__main__":
     unittest.main()

diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c
index 902984cb3db0ed9383be5da1630985685d5953ca..c03652259d0e503d7c7d560b64b02d8261c7fe68 100644 (file)
--- a/Modules/_testinternalcapi.c
+++ b/Modules/_testinternalcapi.c
@@ -1995,6 +1995,13 @@ is_static_immortal(PyObject *self, PyObject *op)
     Py_RETURN_FALSE;
 }
 
+static PyObject *
+incref_decref_delayed(PyObject *self, PyObject *op)
+{
+    _PyObject_XDecRefDelayed(Py_NewRef(op));
+    Py_RETURN_NONE;
+}
+
 static PyMethodDef module_functions[] = {
     {"get_configs", get_configs, METH_NOARGS},
     {"get_recursion_depth", get_recursion_depth, METH_NOARGS},
@@ -2089,6 +2096,7 @@ static PyMethodDef module_functions[] = {
     {"has_deferred_refcount", has_deferred_refcount, METH_O},
     {"get_tracked_heap_size", get_tracked_heap_size, METH_NOARGS},
     {"is_static_immortal", is_static_immortal, METH_O},
+    {"incref_decref_delayed", incref_decref_delayed, METH_O},
     {NULL, NULL} /* sentinel */
 };
 

diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c
index 5e70e06b9e31710ff6c7255ca1de1c4fb903d578..dd6be1490af016fa4990f9292907146ad00c76a8 100644 (file)
--- a/Objects/obmalloc.c
+++ b/Objects/obmalloc.c
@@ -1143,10 +1143,16 @@ struct _mem_work_chunk {
     struct _mem_work_item array[WORK_ITEMS_PER_CHUNK];
 };
 
+static int
+work_item_should_decref(uintptr_t ptr)
+{
+    return ptr & 0x01;
+}
+
 static void
 free_work_item(uintptr_t ptr, delayed_dealloc_cb cb, void *state)
 {
-    if (ptr & 0x01) {
+    if (work_item_should_decref(ptr)) {
         PyObject *obj = (PyObject *)(ptr - 1);
 #ifdef Py_GIL_DISABLED
         if (cb == NULL) {
@@ -1154,7 +1160,7 @@ free_work_item(uintptr_t ptr, delayed_dealloc_cb cb, void *state)
             Py_DECREF(obj);
             return;
         }
-
+        assert(_PyInterpreterState_GET()->stoptheworld.world_stopped);
         Py_ssize_t refcount = _Py_ExplicitMergeRefcount(obj, -1);
         if (refcount == 0) {
             cb(obj, state);
@@ -1180,7 +1186,7 @@ free_delayed(uintptr_t ptr)
     {
         // Free immediately during interpreter shutdown or if the world is
         // stopped.
-        assert(!interp->stoptheworld.world_stopped || !(ptr & 0x01));
+        assert(!interp->stoptheworld.world_stopped || !work_item_should_decref(ptr));
         free_work_item(ptr, NULL, NULL);
         return;
     }
@@ -1207,10 +1213,22 @@ free_delayed(uintptr_t ptr)
 
     if (buf == NULL) {
         // failed to allocate a buffer, free immediately
+        PyObject *to_dealloc = NULL;
         _PyEval_StopTheWorld(tstate->base.interp);
-        // TODO: Fix me
-        free_work_item(ptr, NULL, NULL);
+        if (work_item_should_decref(ptr)) {
+            PyObject *obj = (PyObject *)(ptr - 1);
+            Py_ssize_t refcount = _Py_ExplicitMergeRefcount(obj, -1);
+            if (refcount == 0) {
+                to_dealloc = obj;
+            }
+        }
+        else {
+            PyMem_Free((void *)ptr);
+        }
         _PyEval_StartTheWorld(tstate->base.interp);
+        if (to_dealloc != NULL) {
+            _Py_Dealloc(to_dealloc);
+        }
         return;
     }
 
@@ -1257,14 +1275,16 @@ process_queue(struct llist_node *head, struct _qsbr_thread_state *qsbr,
     while (!llist_empty(head)) {
         struct _mem_work_chunk *buf = work_queue_first(head);
 
-        while (buf->rd_idx < buf->wr_idx) {
+        if (buf->rd_idx < buf->wr_idx) {
             struct _mem_work_item *item = &buf->array[buf->rd_idx];
             if (!_Py_qsbr_poll(qsbr, item->qsbr_goal)) {
                 return;
             }
 
-            free_work_item(item->ptr, cb, state);
             buf->rd_idx++;
+            // NB: free_work_item may re-enter or execute arbitrary code
+            free_work_item(item->ptr, cb, state);
+            continue;
         }
 
         assert(buf->rd_idx == buf->wr_idx);
@@ -1360,12 +1380,14 @@ _PyMem_FiniDelayed(PyInterpreterState *interp)
     while (!llist_empty(head)) {
         struct _mem_work_chunk *buf = work_queue_first(head);
 
-        while (buf->rd_idx < buf->wr_idx) {
+        if (buf->rd_idx < buf->wr_idx) {
             // Free the remaining items immediately. There should be no other
             // threads accessing the memory at this point during shutdown.
             struct _mem_work_item *item = &buf->array[buf->rd_idx];
-            free_work_item(item->ptr, NULL, NULL);
             buf->rd_idx++;
+            // NB: free_work_item may re-enter or execute arbitrary code
+            free_work_item(item->ptr, NULL, NULL);
+            continue;
         }
 
         llist_remove(&buf->node);