From 44c153617b4281dded9be0be1c50f347cb0ec980 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Fri, 28 Aug 2026 00:18:27 +0530 Subject: [PATCH] gh-144446: Fix thread-safety of FrameLocalsProxy on executing frames Synchronize FrameLocalsProxy accesses with the frame's owning thread: accessors run under the frame object's critical section and stop the world when the frame is executing on another thread. Co-authored-by: Sam Gross --- Lib/test/test_free_threading/test_frame.py | 340 +++++++++++++++++- ...-08-28-10-15-00.gh-issue-144446.pQ7wXn.rst | 2 + Objects/frameobject.c | 242 ++++++++++--- 3 files changed, 533 insertions(+), 51 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-08-28-10-15-00.gh-issue-144446.pQ7wXn.rst diff --git a/Lib/test/test_free_threading/test_frame.py b/Lib/test/test_free_threading/test_frame.py index bea49df557aa2c..cbf8d04d9d467a 100644 --- a/Lib/test/test_free_threading/test_frame.py +++ b/Lib/test/test_free_threading/test_frame.py @@ -3,7 +3,7 @@ import threading import unittest -from test.support import threading_helper +from test.support import import_helper, threading_helper threading_helper.requires_working_threading(module=True) @@ -122,6 +122,344 @@ def writer(frame): run_with_frame([reader, writer, reader, writer]) + def test_concurrent_f_locals_read_values(self): + def runner(): + a = 1 + b = "hello" + c = [1, 2, 3] + for i in range(100): + a += i + + def reader(frame): + locals_dict = frame.f_locals + list(locals_dict.keys()) + list(locals_dict.values()) + + run_with_frame(reader, runner=runner) + + def test_concurrent_f_locals_write(self): + def runner(): + x = 0 + for i in range(100): + x += i + + def writer(frame): + frame.f_locals["new_var"] = 42 + + run_with_frame(writer, runner=runner) + + def test_concurrent_f_locals_read_write(self): + def runner(): + a = 1 + b = 2 + for i in range(100): + a += i + + def reader(frame): + _ = frame.f_locals.get("a") + _ = frame.f_locals.get("b") + + def writer(frame): + frame.f_locals["a"] = 42 + + run_with_frame([reader, writer, reader, writer], runner=runner) + + def test_concurrent_f_locals_iteration(self): + def runner(): + a = 1 + b = "hello" + c = [1, 2, 3] + for i in range(100): + a += i + + def iterator(frame): + for key, value in frame.f_locals.items(): + pass + + run_with_frame(iterator, runner=runner) + + def test_gen_f_locals_read_while_running(self): + # gh-144446: reading f_locals of a generator frame while the + # generator is executing on another thread. + for _ in range(5): + def gen_fn(): + x = 0 + obj = None + s = None + yield + for i in range(2000): + obj = [i] * 4 + s = str(i) * 8 + x += i + yield x + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + + def runner(): + barrier.wait() + next(g) + + def reader(): + barrier.wait() + for _ in range(100): + fl = frame.f_locals + list(fl.values()) + fl.get("obj") + fl.get("s") + len(fl) + + threading_helper.run_concurrently([runner, reader, reader]) + g.close() + + def test_gen_f_locals_vs_resume_cycle(self): + # Concurrent f_locals access must not make a concurrent send() + # spuriously fail with "already executing". + for _ in range(5): + def gen_fn(): + x = 0 + while True: + x += 1 + yield x + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + + def runner(): + barrier.wait() + for _ in range(1000): + next(g) + + def reader(): + barrier.wait() + for _ in range(200): + fl = frame.f_locals + fl.get("x") + list(fl.items()) + + threading_helper.run_concurrently([runner, reader, reader]) + g.close() + + def test_gen_f_locals_write_suspended(self): + # Writes through f_locals must be synchronized with resuming. + for _ in range(5): + def gen_fn(): + x = 0 + extra = None + while True: + x += 1 + yield x + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + + def runner(): + barrier.wait() + for _ in range(500): + next(g) + + def writer(): + barrier.wait() + for i in range(200): + frame.f_locals["extra"] = [i] + frame.f_locals["new_var"] = i + + threading_helper.run_concurrently([runner, writer, writer]) + g.close() + + def test_gen_f_locals_inside_running_gen(self): + # f_locals access from inside a running generator happens on the + # executing thread itself and must work without synchronization + # with other threads accessing the same frame. + for _ in range(5): + def gen_fn(): + x = 0 + yield + frame = sys._getframe() + for i in range(500): + x += i + assert frame.f_locals["x"] == x + yield x + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + + def runner(): + barrier.wait() + next(g) + + def reader(): + barrier.wait() + for _ in range(100): + frame.f_locals.get("x") + + threading_helper.run_concurrently([runner, reader, reader]) + g.close() + + def test_gen_f_locals_dying_generator(self): + # Access f_locals while the last reference to the generator is + # dropped and the frame ownership moves to the frame object. + for _ in range(20): + def gen_fn(): + x = 42 + yield x + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + ref = [g] + del g + + def dropper(): + barrier.wait() + ref.clear() + + def reader(): + barrier.wait() + for _ in range(100): + frame.f_locals.get("x") + list(frame.f_locals.values()) + + threading_helper.run_concurrently([dropper, reader, reader]) + + def test_setitem_old_value_destructor_reenters_proxy(self): + # gh-144446: the value displaced by a f_locals store must be + # released outside the synchronized region: its destructor may + # access the proxy again (this would deadlock on the frame's + # critical section, or try to stop the world twice). + deleted = [] + frame = sys._getframe() + + class Old: + def __del__(self): + deleted.append(frame.f_locals.get("marker")) + + marker = 42 + # Not a real local: goes to the frame's extra locals dict. + frame.f_locals["extra_key"] = Old() + frame.f_locals["extra_key"] = None # replace: destructor runs + self.assertEqual(deleted, [42]) + del frame.f_locals["extra_key"] + + def test_gen_setitem_old_value_destructor_stw(self): + # Same as above, but on a suspended generator frame, where the + # store happens under stop-the-world. + deleted = [] + + def gen_fn(): + yield + + g = gen_fn() + next(g) + frame = g.gi_frame + + class Old: + def __del__(self): + # Accessing the suspended generator frame's proxy stops + # the world again; it must run after the world restarts. + deleted.append(len(frame.f_locals)) + + frame.f_locals["extra_key"] = Old() + frame.f_locals["extra_key"] = None + self.assertEqual(len(deleted), 1) + del frame.f_locals["extra_key"] + g.close() + + def test_gen_setitem_cell_old_value_destructor_stw(self): + # The old value displaced from a cell variable must also be + # released after the world restarts. + deleted = [] + + def make_gen(): + x = None + def gen_fn(): + nonlocal x + yield x + return gen_fn() + + g = make_gen() + next(g) + frame = g.gi_frame + + class Old: + def __del__(self): + deleted.append(frame.f_locals.get("x")) + + frame.f_locals["x"] = Old() + frame.f_locals["x"] = "new" # replace cell value: destructor runs + self.assertEqual(deleted, ["new"]) + g.close() + + def test_gen_pop_extra_locals_concurrent(self): + # pop() must be synchronized with the frame's owner like the + # other accessors. + for _ in range(5): + def gen_fn(): + x = 0 + while True: + x += 1 + yield x + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + + def runner(): + barrier.wait() + for _ in range(500): + next(g) + + def writer(): + barrier.wait() + for i in range(200): + frame.f_locals["extra_key"] = [i] + frame.f_locals.pop("extra_key", None) + + threading_helper.run_concurrently([runner, writer, writer]) + g.close() + + def test_gen_getvar_while_running(self): + # PyFrame_GetVar() reads fast locals and must synchronize with + # the frame's owner as well. + _testcapi = import_helper.import_module("_testcapi") + for _ in range(5): + def gen_fn(): + obj = None + yield + for i in range(2000): + obj = [i] * 4 + yield obj + + g = gen_fn() + next(g) + frame = g.gi_frame + barrier = threading.Barrier(3) + + def runner(): + barrier.wait() + next(g) + + def reader(): + barrier.wait() + for _ in range(100): + try: + _testcapi.frame_getvar(frame, "obj") + except NameError: + pass + + threading_helper.run_concurrently([runner, reader, reader]) + g.close() + def test_concurrent_frame_clear(self): # Test race between frame.clear() and attribute reads. def create_frame(): diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-08-28-10-15-00.gh-issue-144446.pQ7wXn.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-08-28-10-15-00.gh-issue-144446.pQ7wXn.rst new file mode 100644 index 00000000000000..2718a4cc7487f0 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-08-28-10-15-00.gh-issue-144446.pQ7wXn.rst @@ -0,0 +1,2 @@ +Fix thread safety of :attr:`frame.f_locals` and :c:func:`PyFrame_GetVar` on +frames executing on another thread in the free-threaded build. diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 5889cdaf2aa165..84d2f76763b0b8 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -14,6 +14,8 @@ #include "pycore_object.h" // _PyObject_GC_UNTRACK() #include "pycore_opcode_metadata.h" // _PyOpcode_Caches #include "pycore_optimizer.h" // _Py_Executors_InvalidateDependency() +#include "pycore_pystate.h" // _PyEval_StopTheWorld() +#include "pycore_tstate.h" // _PyThreadStateImpl #include "pycore_tuple.h" // _PyTuple_FromPair #include "pycore_unicodeobject.h" // _PyUnicode_Equal() #include "pycore_weakref.h" // FT_CLEAR_WEAKREFS() @@ -41,6 +43,53 @@ class frame "PyFrameObject *" "&PyFrame_Type" /*[clinic end generated code: output=da39a3ee5e6b4b0d input=2d1dbf2e06cf351f]*/ +#ifdef Py_GIL_DISABLED +// Returns 1 if the frame may be executing or resumed on another thread. +static int +_frame_is_on_other_thread(PyFrameObject *frame) +{ + _PyInterpreterFrame *iframe = frame->f_frame; + if (iframe->owner == FRAME_OWNED_BY_THREAD) { + PyThreadState *tstate = _PyThreadState_GET(); + int32_t our_tlbc = ((_PyThreadStateImpl *)tstate)->tlbc_index; + return iframe->tlbc_index != our_tlbc; + } + if (iframe->owner == FRAME_OWNED_BY_GENERATOR) { + // Another thread may be running or resume the generator. + return 1; + } + // FRAME_OWNED_BY_FRAME_OBJECT: the frame can no longer run. + return 0; +} +#endif + +// Evaluate CALL under the frame's critical section, or with the world +// stopped if the frame may be running on another thread. +#ifdef Py_GIL_DISABLED +#define FRAMELOCALSPROXY_LOCKED(FRAME, CALL, RESULT) \ + do { \ + int _stw; \ + Py_BEGIN_CRITICAL_SECTION(FRAME); \ + _stw = _frame_is_on_other_thread(FRAME); \ + if (!_stw) { \ + RESULT = CALL; \ + } \ + Py_END_CRITICAL_SECTION(); \ + if (_stw) { \ + PyInterpreterState *interp = _PyInterpreterState_GET(); \ + _PyEval_StopTheWorld(interp); \ + RESULT = CALL; \ + _PyEval_StartTheWorld(interp); \ + } \ + } while (0) +#else +#define FRAMELOCALSPROXY_LOCKED(FRAME, CALL, RESULT) \ + do { \ + RESULT = CALL; \ + } while (0) +#endif + + // Returns new reference or NULL static PyObject * framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i) @@ -187,9 +236,8 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyO } static PyObject * -framelocalsproxy_getitem(PyObject *self, PyObject *key) +framelocalsproxy_getitem_lock_held(PyFrameObject *frame, PyObject *key) { - PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; PyObject *value = NULL; int i = framelocalsproxy_getkeyindex(frame, key, true, &value); @@ -214,10 +262,23 @@ framelocalsproxy_getitem(PyObject *self, PyObject *key) } } - PyErr_Format(PyExc_KeyError, "local variable '%R' is not defined", key); + // KeyError is raised by the caller, outside the synchronized region. return NULL; } +static PyObject * +framelocalsproxy_getitem(PyObject *self, PyObject *key) +{ + PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + PyObject *result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_getitem_lock_held(frame, key), result); + if (result == NULL && !PyErr_Occurred()) { + PyErr_Format(PyExc_KeyError, "local variable '%R' is not defined", key); + } + return result; +} + static int add_overwritten_fast_local(PyFrameObject *frame, PyObject *obj) { @@ -245,11 +306,13 @@ add_overwritten_fast_local(PyFrameObject *frame, PyObject *obj) return 0; } +// The replaced value is returned in `*old_value` (strong reference or +// NULL) so that its destructor runs outside the synchronized region. static int -framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) +framelocalsproxy_setitem_lock_held(PyFrameObject *frame, PyObject *key, + PyObject *value, PyObject **old_value) { /* Merge locals into fast locals */ - PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; _PyStackRef *fast = _PyFrame_GetLocalsArray(frame->f_frame); PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); @@ -283,7 +346,7 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) } if (cell != NULL) { Py_XINCREF(value); - PyCell_SetTakeRef((PyCellObject *)cell, value); + *old_value = PyCell_SwapTakeRef((PyCellObject *)cell, value); } else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) { PyObject *old_obj = PyStackRef_AsPyObjectBorrow(fast[i]); if (old_obj != NULL && !_Py_IsImmortal(old_obj)) { @@ -316,12 +379,37 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) assert(PyDict_Check(extra)); if (value == NULL) { - return PyDict_DelItem(extra, key); - } else { + int res = PyDict_Pop(extra, key, old_value); + if (res < 0) { + return -1; + } + if (res == 0) { + _PyErr_SetKeyError(key); + return -1; + } + return 0; + } + else { + if (PyDict_GetItemRef(extra, key, old_value) < 0) { + return -1; + } return PyDict_SetItem(extra, key, value); } } +static int +framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) +{ + PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + PyObject *old_value = NULL; + int result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_setitem_lock_held(frame, key, value, &old_value), + result); + Py_XDECREF(old_value); + return result; +} + static int framelocalsproxy_merge(PyObject* self, PyObject* other) { @@ -372,9 +460,8 @@ framelocalsproxy_merge(PyObject* self, PyObject* other) } static PyObject * -framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) +framelocalsproxy_keys_lock_held(PyFrameObject *frame) { - PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); PyObject *names = PyList_New(0); if (names == NULL) { @@ -410,6 +497,16 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) return names; } +static PyObject * +framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + PyObject *result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_keys_lock_held(frame), result); + return result; +} + static void framelocalsproxy_dealloc(PyObject *self) { @@ -581,9 +678,8 @@ framelocalsproxy_inplace_or(PyObject *self, PyObject *other) } static PyObject * -framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) +framelocalsproxy_values_lock_held(PyFrameObject *frame) { - PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); PyObject *values = PyList_New(0); if (values == NULL) { @@ -619,9 +715,18 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) } static PyObject * -framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) +framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) { PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + PyObject *result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_values_lock_held(frame), result); + return result; +} + +static PyObject * +framelocalsproxy_items_lock_held(PyFrameObject *frame) +{ PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); PyObject *items = PyList_New(0); if (items == NULL) { @@ -668,10 +773,19 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } -static Py_ssize_t -framelocalsproxy_length(PyObject *self) +static PyObject * +framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) { PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + PyObject *result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_items_lock_held(frame), result); + return result; +} + +static Py_ssize_t +framelocalsproxy_length_lock_held(PyFrameObject *frame) +{ PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); Py_ssize_t size = 0; @@ -688,11 +802,19 @@ framelocalsproxy_length(PyObject *self) return size; } -static int -framelocalsproxy_contains(PyObject *self, PyObject *key) +static Py_ssize_t +framelocalsproxy_length(PyObject *self) { PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + Py_ssize_t result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_length_lock_held(frame), result); + return result; +} +static int +framelocalsproxy_contains_lock_held(PyFrameObject *frame, PyObject *key) +{ int i = framelocalsproxy_getkeyindex(frame, key, true, NULL); if (i == -2) { return -1; @@ -709,6 +831,16 @@ framelocalsproxy_contains(PyObject *self, PyObject *key) return 0; } +static int +framelocalsproxy_contains(PyObject *self, PyObject *key) +{ + PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + int result; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_contains_lock_held(frame, key), result); + return result; +} + static PyObject* framelocalsproxy___contains__(PyObject *self, PyObject *key) { int result = framelocalsproxy_contains(self, key); @@ -788,6 +920,17 @@ framelocalsproxy_setdefault(PyObject* self, PyObject *const *args, Py_ssize_t na return result; } +// Returns 1 if `key` was popped into `*result`, 0 if not found, -1 on error. +static int +framelocalsproxy_pop_lock_held(PyFrameObject *frame, PyObject *key, + PyObject **result) +{ + if (frame->f_extra_locals == NULL) { + return 0; + } + return PyDict_Pop(frame->f_extra_locals, key, result); +} + static PyObject* framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) { @@ -815,29 +958,19 @@ framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) } PyObject *result = NULL; - - if (frame->f_extra_locals == NULL) { - if (default_value != NULL) { - return Py_XNewRef(default_value); - } else { - _PyErr_SetKeyError(key); - return NULL; - } - } - - if (PyDict_Pop(frame->f_extra_locals, key, &result) < 0) { + int found; + FRAMELOCALSPROXY_LOCKED(frame, + framelocalsproxy_pop_lock_held(frame, key, &result), found); + if (found < 0) { return NULL; } - - if (result == NULL) { + if (found == 0) { if (default_value != NULL) { return Py_XNewRef(default_value); - } else { - _PyErr_SetKeyError(key); - return NULL; } + _PyErr_SetKeyError(key); + return NULL; } - return result; } @@ -2299,15 +2432,11 @@ _PyFrame_GetLocals(_PyInterpreterFrame *frame) } -PyObject * -PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name) +// Returns 1 if `name` is a bound fast local, storing its value in `*pvalue`. +static int +frame_getvar_lock_held(PyFrameObject *frame_obj, PyObject *name, + PyObject **pvalue) { - if (!PyUnicode_Check(name)) { - PyErr_Format(PyExc_TypeError, "name must be str, not %s", - Py_TYPE(name)->tp_name); - return NULL; - } - _PyInterpreterFrame *frame = frame_obj->f_frame; frame_init_get_vars(frame); @@ -2317,17 +2446,30 @@ PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name) if (!_PyUnicode_Equal(var_name, name)) { continue; } - - PyObject *value; - if (!frame_get_var(frame, co, i, &value)) { - break; - } - if (value == NULL) { - break; + if (!frame_get_var(frame, co, i, pvalue)) { + return 0; } - return value; + return *pvalue != NULL; } + return 0; +} +PyObject * +PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name) +{ + if (!PyUnicode_Check(name)) { + PyErr_Format(PyExc_TypeError, "name must be str, not %s", + Py_TYPE(name)->tp_name); + return NULL; + } + + PyObject *value = NULL; + int found; + FRAMELOCALSPROXY_LOCKED(frame_obj, + frame_getvar_lock_held(frame_obj, name, &value), found); + if (found) { + return value; + } PyErr_Format(PyExc_NameError, "variable %R does not exist", name); return NULL; }