From 3715284939f079761e77a65d7a1243003a38e31f Mon Sep 17 00:00:00 2001 From: Tobias Wrigstad Date: Sat, 10 Oct 2026 00:39:36 +0200 Subject: [PATCH] Reject closes that count an object more often than its RC allows The close only checked that the LRC summed over all traced objects was zero. An object counted through more internal references than its recorded RC has a negative balance, which can cancel references from outside on other objects. A thread holding `a` and `b` in a region can move items from `a` to `b` between the two traversals without any refcount operation (e.g. a pop followed by a stealing store): each item is counted twice (-1), cancelling the +1 of each held list, and the region closes while the thread still holds both. Neither the trace flag nor the RC re-read sees this, since no refcount changes. With no object allowed below zero, a zero total means every object balances. The error is raised after the restart check, since code run by freezing during a trace may move references that a fresh trace counts correctly. Adds a debug-only trace hook, called after each traversed object, and _testinternalcapi.region_set_trace_move() to reproduce the move. The test fails without the check on both GIL and free-threaded builds. --- Include/internal/pycore_immutability.h | 6 ++ Lib/test/test_freeze/test_tracing_region.py | 39 ++++++++++++ Modules/_testinternalcapi.c | 66 +++++++++++++++++++++ Objects/tracingregionobject.c | 34 +++++++++++ 4 files changed, 145 insertions(+) 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