diff --git a/Include/internal/pycore_immutability.h b/Include/internal/pycore_immutability.h index b204680f84ecd4..208c8af03f80bd 100644 --- a/Include/internal/pycore_immutability.h +++ b/Include/internal/pycore_immutability.h @@ -26,6 +26,12 @@ PyAPI_FUNC(int) _PyTracingRegion_AttachIgnoreRegionRefs(PyObject* region); * and the validation, with the thread detached, so that other threads can * interfere with it. 0 disables the pause. */ PyAPI_FUNC(void) _PyTracingRegion_SetTestPause(PyTime_t ns); + +/* Tests only: makes every close call `hook` on the closing thread right after + * its trace traversed `traversed`, so that a test can change the object graph + * between two traversals. NULL removes the hook. */ +typedef void (*_PyTracingRegion_TestTraceHook)(PyObject *traversed); +PyAPI_FUNC(void) _PyTracingRegion_SetTestTraceHook(_PyTracingRegion_TestTraceHook hook); #endif /* Returns the region's metadata node, allocating it if this is the first diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index 6c45a304fbbd51..8225eaf40a5411 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -1002,6 +1002,45 @@ class A: self.assertEqual(c.value.local_ref().tag, "local") +_testinternalcapi = import_helper.import_module("_testinternalcapi") + +@unittest.skipUnless(hasattr(_testinternalcapi, "region_set_trace_move"), + "needs the trace hook of debug builds") +class TestGraphChangedDuringTrace(unittest.TestCase): + """The object graph changes between two traversals of a close, as if a + thread holding references into the region changed it concurrently.""" + + def tearDown(self): + _testinternalcapi.region_set_trace_move(None, None, 0) + + def test_moved_references_cannot_hide_incoming_references(self): + # `dst` is only reachable through `src`, so the trace traverses `src` + # first. Right after, the hook moves two items from `src` to `dst` + # without changing any refcount, so the trace counts each item's one + # reference twice. The hook holds a reference to `src` and to `dst`, + # which must keep the region open: two references too few on the + # items must not make up for two too many on the lists. + # + # A close that freezes the region type traces again, which would + # only see the graph after the move, so the type is frozen up front. + deep_freeze(Region) + r = Region() + dst = [] + src = [dst, [], []] + r.src = src + _testinternalcapi.region_set_trace_move(src, dst, 2) + del src, dst + + with self.assertRaises(RuntimeError): + _testinternalcapi.region_close(r) + self.assertFalse(_testinternalcapi.region_is_closed(r)) + + # Once the references are dropped, the region closes. + _testinternalcapi.region_set_trace_move(None, None, 0) + _testinternalcapi.region_close(r) + self.assertTrue(_testinternalcapi.region_is_closed(r)) + + @unittest.skipUnless(support.Py_GIL_DISABLED, "cross-thread object ownership only exists on free threading") class TestCrossThreadClose(unittest.TestCase): diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 5817b543539f04..b38b18f855d407 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -2465,6 +2465,71 @@ region_set_close_pause(PyObject *self, PyObject *seconds) _PyTracingRegion_SetTestPause(ns); Py_RETURN_NONE; } + +/* Strong references, standing in for a thread that took them before a close + * started and holds them throughout. */ +static PyObject *trace_move_src = NULL; +static PyObject *trace_move_dst = NULL; +static Py_ssize_t trace_move_count = 0; + +/* Moves `trace_move_count` items from the end of `trace_move_src` to the end + * of `trace_move_dst`, right after the trace traversed the source. Each item + * ends up with the refcount it started with, as with `dst.append(src.pop())` + * done by a thread that does not take references of its own. Runs once. */ +static void +trace_move_hook(PyObject *traversed) +{ + if (traversed != trace_move_src || trace_move_count == 0) { + return; + } + PyObject *src = trace_move_src; + PyObject *dst = trace_move_dst; + Py_BEGIN_CRITICAL_SECTION2(src, dst); + for (; trace_move_count > 0; trace_move_count--) { + Py_ssize_t size = PyList_GET_SIZE(src); + if (size == 0) { + break; + } + PyObject *item = Py_NewRef(PyList_GET_ITEM(src, size - 1)); + if (PyList_SetSlice(src, size - 1, size, NULL) < 0 + || PyList_Append(dst, item) < 0) + { + PyErr_FormatUnraisable("Exception ignored in the trace move hook"); + } + Py_DECREF(item); + } + Py_END_CRITICAL_SECTION2(); + trace_move_count = 0; +} + +/* region_set_trace_move(src, dst, count): the next close that traverses the + * list `src` moves `count` items from it to the list `dst` right after, while + * this module holds a reference to each list. region_set_trace_move(None, + * None, 0) removes the hook and drops the references. */ +static PyObject * +region_set_trace_move(PyObject *self, PyObject *args) +{ + PyObject *src, *dst; + Py_ssize_t count; + if (!PyArg_ParseTuple(args, "OOn", &src, &dst, &count)) { + return NULL; + } + if (src != Py_None && (!PyList_Check(src) || !PyList_Check(dst))) { + PyErr_SetString(PyExc_TypeError, "expected two lists or two Nones"); + return NULL; + } + _PyTracingRegion_SetTestTraceHook(NULL); + Py_CLEAR(trace_move_src); + Py_CLEAR(trace_move_dst); + trace_move_count = 0; + if (src != Py_None) { + trace_move_src = Py_NewRef(src); + trace_move_dst = Py_NewRef(dst); + trace_move_count = count; + _PyTracingRegion_SetTestTraceHook(trace_move_hook); + } + Py_RETURN_NONE; +} #endif static PyMethodDef module_functions[] = { @@ -2472,6 +2537,7 @@ static PyMethodDef module_functions[] = { {"region_is_closed", region_is_closed, METH_O}, #ifdef Py_DEBUG {"region_set_close_pause", region_set_close_pause, METH_O}, + {"region_set_trace_move", region_set_trace_move, METH_VARARGS}, #endif {"get_configs", get_configs, METH_NOARGS}, {"get_recursion_depth", get_recursion_depth, METH_NOARGS}, diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 7d9988b3e3be21..a250639a5eb8b8 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -777,6 +777,10 @@ typedef struct { // to restart to be valid bool restart; + // Set if some object was reached through more internal references than + // its RC allowed. Only reported if the trace does not restart. + bool overcounted; + // Indicates if the given reference is a strong reference or a weak one. bool strong_ref; @@ -844,6 +848,7 @@ static int region_trace_state_init( state->external_rc = 0; state->bridge_rc = 0; state->restart = false; + state->overcounted = false; // References are strong unless the trace explicitly follows a weak one. state->strong_ref = true; state->has_weak_refs = false; @@ -1664,6 +1669,11 @@ static int _trace_visit(PyObject* obj, region_trace_state_t* state) { visit_info->unaccounted_rc -= 1; dbg(" - Internal reference to %p; LRC -= 1", obj); state->external_rc -= 1; + // More internal edges than the RC recorded at discovery: an edge + // was counted twice, and the excess could hide an external ref. + if (visit_info->unaccounted_rc < 0) { + state->overcounted = true; + } } return 0; } @@ -1733,6 +1743,12 @@ static int _validate_region_closed(TracingRegionObject *region_obj, region_trace } #ifdef Py_DEBUG +static _PyTracingRegion_TestTraceHook region_test_trace_hook = NULL; + +void _PyTracingRegion_SetTestTraceHook(_PyTracingRegion_TestTraceHook hook) { + _Py_atomic_store_ptr_relaxed(®ion_test_trace_hook, (void *)hook); +} + static PyTime_t region_test_pause_ns = 0; void _PyTracingRegion_SetTestPause(PyTime_t ns) { @@ -1810,6 +1826,13 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac state.src = item; dbg(" - traversing %p", item); SUCCEEDS(_PyObject_VisitReachable(item, (visitproc)_trace_visit, (void*)&state)); +#ifdef Py_DEBUG + _PyTracingRegion_TestTraceHook hook = (_PyTracingRegion_TestTraceHook) + _Py_atomic_load_ptr_relaxed(®ion_test_trace_hook); + if (hook != NULL) { + hook(item); + } +#endif if (PyWeakref_Check(item)) { PyWeakReference *wref = (PyWeakReference*)item; @@ -1842,6 +1865,17 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac goto finally; } + // Checked after the restart, since code run by freezing may have moved + // references, which a fresh trace sees correctly. + if (state.overcounted) { +#ifdef _Py_PYRONA_INTERPRETER_SHARING + gc_list_dissolve(®ion->gc_list); +#endif + PyErr_SetString(PyExc_RuntimeError, + "the region cannot be closed since a concurrent thread accessed during tracing"); + goto error; + } + // Report an error, if the region couldn't be closed. if (state.external_rc > 0) { #ifdef _Py_PYRONA_INTERPRETER_SHARING