diff --git a/Include/internal/pycore_pymem.h b/Include/internal/pycore_pymem.h index defe143e9dcc3c..5386d4c5f83031 100644 --- 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 5d0a383de64520..3e8fd91b9a67a0 100644 --- a/Lib/test/test_capi/test_object.py +++ b/Lib/test/test_capi/test_object.py @@ -210,5 +210,18 @@ def test_decref_freed_object(self): """ 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 902984cb3db0ed..c03652259d0e50 100644 --- 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 5e70e06b9e3171..dd6be1490af016 100644 --- 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);