From a243a4bb68fbb26751808ff9fd6d8ab9b815e539 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 5 Feb 2026 16:07:04 -0500 Subject: [PATCH] gh-144446: Fix thread-safety of FrameLocalsProxy on executing frames Use stop-the-world to synchronize access to fast locals when the frame is executing on a different thread. --- Lib/test/test_free_threading/test_frame.py | 56 +++++++++ Objects/frameobject.c | 137 +++++++++++++++++++-- 2 files changed, 180 insertions(+), 13 deletions(-) diff --git a/Lib/test/test_free_threading/test_frame.py b/Lib/test/test_free_threading/test_frame.py index bea49df557aa2c..e5fdbd30970817 100644 --- a/Lib/test/test_free_threading/test_frame.py +++ b/Lib/test/test_free_threading/test_frame.py @@ -122,6 +122,62 @@ 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_concurrent_frame_clear(self): # Test race between frame.clear() and attribute reads. def create_frame(): diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 9a7abfc0ec26ab..6e3c405513947e 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -13,6 +13,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_unicodeobject.h" // _PyUnicode_Equal() #include "frameobject.h" // PyFrameLocalsProxyObject @@ -38,6 +40,52 @@ class frame "PyFrameObject *" "&PyFrame_Type" /*[clinic end generated code: output=da39a3ee5e6b4b0d input=2d1dbf2e06cf351f]*/ +#ifdef Py_GIL_DISABLED +// Returns 1 if the frame is currently executing on a different thread. +// Must be called while holding the critical section on the frame object. +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; + } + // TODO(gh-144446): handle FRAME_OWNED_BY_GENERATOR by locking + // the generator's frame state to synchronize with gen_send_ex2. + return 0; +} +#endif + +// Call `call` synchronized with the frame's owning thread and store +// the result in `result`. If the frame is executing on another thread, +// pause all threads (stop-the-world) before calling. +#ifdef Py_GIL_DISABLED +#define FRAMELOCALSPROXY_SYNCHRONIZED(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_SYNCHRONIZED(FRAME, CALL, RESULT) \ + do { \ + RESULT = CALL; \ + } while (0) +#endif + + // Returns new reference or NULL static PyObject * framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i) @@ -184,9 +232,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); @@ -215,6 +262,16 @@ framelocalsproxy_getitem(PyObject *self, PyObject *key) return NULL; } +static PyObject * +framelocalsproxy_getitem(PyObject *self, PyObject *key) +{ + PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + PyObject *result; + FRAMELOCALSPROXY_SYNCHRONIZED(frame, + framelocalsproxy_getitem_lock_held(frame, key), result); + return result; +} + static int add_overwritten_fast_local(PyFrameObject *frame, PyObject *obj) { @@ -243,10 +300,10 @@ add_overwritten_fast_local(PyFrameObject *frame, PyObject *obj) } static int -framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) +framelocalsproxy_setitem_lock_held(PyFrameObject *frame, PyObject *key, + PyObject *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); @@ -319,6 +376,16 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) } } +static int +framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) +{ + PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame; + int result; + FRAMELOCALSPROXY_SYNCHRONIZED(frame, + framelocalsproxy_setitem_lock_held(frame, key, value), result); + return result; +} + static int framelocalsproxy_merge(PyObject* self, PyObject* other) { @@ -369,9 +436,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) { @@ -407,6 +473,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_SYNCHRONIZED(frame, + framelocalsproxy_keys_lock_held(frame), result); + return result; +} + static void framelocalsproxy_dealloc(PyObject *self) { @@ -578,9 +654,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) { @@ -616,9 +691,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_SYNCHRONIZED(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) { @@ -674,10 +758,19 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) return items; } -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_SYNCHRONIZED(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; @@ -694,11 +787,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_SYNCHRONIZED(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; @@ -715,6 +816,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_SYNCHRONIZED(frame, + framelocalsproxy_contains_lock_held(frame, key), result); + return result; +} + static PyObject* framelocalsproxy___contains__(PyObject *self, PyObject *key) { int result = framelocalsproxy_contains(self, key);