From bf7f4f357b58d169508f0e26951c77fff01b639e Mon Sep 17 00:00:00 2001 From: "Miss Islington (bot)" <31488909+miss-islington@users.noreply.github.com> Date: Fri, 19 Jul 2024 19:49:34 +0200 Subject: [PATCH] [3.13] gh-120973: Fix thread-safety issues with `threading.local` (GH-121655) (#122042) This is a small refactoring to the current design that allows us to avoid manually iterating over threads. This should also fix gh-118490. (cherry picked from commit e059aa6b01030310125477c3ed1da0491020fe10) Co-authored-by: mpage --- Include/cpython/pystate.h | 8 + Modules/_threadmodule.c | 384 +++++++++++++++++++++++--------------- Python/pystate.c | 3 + 3 files changed, 245 insertions(+), 150 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index bb2af78a376..f005729fff1 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -192,6 +192,14 @@ struct _ts { PyObject *previous_executor; uint64_t dict_global_version; + + /* Used to store/retrieve `threading.local` keys/values for this thread */ + PyObject *threading_local_key; + + /* Used by `threading.local`s to be remove keys/values for dying threads. + The PyThreadObject must hold the only reference to this value. + */ + PyObject *threading_local_sentinel; }; #ifdef Py_DEBUG diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 39d309729d8..d21a37d8866 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1350,33 +1350,44 @@ newlockobject(PyObject *module) Our implementation uses small "localdummy" objects in order to break the reference chain. These trivial objects are hashable (using the default scheme of identity hashing) and weakrefable. - Each thread-state holds a separate localdummy for each local object - (as a /strong reference/), - and each thread-local object holds a dict mapping /weak references/ - of localdummies to local dicts. + + Each thread-state holds two separate localdummy objects: + + - `threading_local_key` is used as a key to retrieve the locals dictionary + for the thread in any `threading.local` object. + - `threading_local_sentinel` is used to signal when a thread is being + destroyed. Consequently, the associated thread-state must hold the only + reference. + + Each `threading.local` object contains a dict mapping localdummy keys to + locals dicts and a set containing weak references to localdummy + sentinels. Each sentinel weak reference has a callback that removes itself + and the locals dict for the key from the `threading.local` object when + called. Therefore: - - only the thread-state dict holds a strong reference to the dummies - - only the thread-local object holds a strong reference to the local dicts - - only outside objects (application- or library-level) hold strong - references to the thread-local objects - - as soon as a thread-state dict is destroyed, the weakref callbacks of all - dummies attached to that thread are called, and destroy the corresponding - local dicts from thread-local objects - - as soon as a thread-local object is destroyed, its local dicts are - destroyed and its dummies are manually removed from all thread states - - the GC can do its work correctly when a thread-local object is dangling, - without any interference from the thread-state dicts + - The thread-state only holds strong references to localdummy objects, which + cannot participate in cycles. + - Only outside objects (application- or library-level) hold strong + references to the thread-local objects. + - As soon as thread-state's sentinel dummy is destroyed the callbacks for + all weakrefs attached to the sentinel are called, and destroy the + corresponding local dicts from thread-local objects. + - As soon as a thread-local object is destroyed, its local dicts are + destroyed. + - The GC can do its work correctly when a thread-local object is dangling, + without any interference from the thread-state dicts. - As an additional optimization, each localdummy holds a borrowed reference - to the corresponding localdict. This borrowed reference is only used - by the thread-local object which has created the localdummy, which should - guarantee that the localdict still exists when accessed. + This dual key arrangement is necessary to ensure that `threading.local` + values can be retrieved from finalizers. If we were to only keep a mapping + of localdummy weakrefs to locals dicts it's possible that the weakrefs would + be cleared before finalizers were called (GC currently clears weakrefs that + are garbage before invoking finalizers), causing lookups in finalizers to + fail. */ typedef struct { PyObject_HEAD - PyObject *localdict; /* Borrowed reference! */ PyObject *weakreflist; /* List of weak references to self */ } localdummyobject; @@ -1413,80 +1424,60 @@ static PyType_Spec local_dummy_type_spec = { typedef struct { PyObject_HEAD - PyObject *key; PyObject *args; PyObject *kw; PyObject *weakreflist; /* List of weak references to self */ - /* A {localdummy weakref -> localdict} dict */ - PyObject *dummies; - /* The callback for weakrefs to localdummies */ - PyObject *wr_callback; + /* A {localdummy -> localdict} dict */ + PyObject *localdicts; + /* A set of weakrefs to thread sentinels localdummies*/ + PyObject *thread_watchdogs; } localobject; /* Forward declaration */ -static PyObject *_ldict(localobject *self, thread_module_state *state); -static PyObject *_localdummy_destroyed(PyObject *meth_self, PyObject *dummyweakref); +static int create_localsdict(localobject *self, thread_module_state *state, + PyObject **localsdict, PyObject **sentinel_wr); +static PyObject *clear_locals(PyObject *meth_self, PyObject *dummyweakref); -/* Create and register the dummy for the current thread. - Returns a borrowed reference of the corresponding local dict */ +/* Create a weakref to the sentinel localdummy for the current thread */ static PyObject * -_local_create_dummy(localobject *self, thread_module_state *state) +create_sentinel_wr(localobject *self) { - PyObject *ldict = NULL, *wr = NULL; - localdummyobject *dummy = NULL; - PyTypeObject *type = state->local_dummy_type; + static PyMethodDef wr_callback_def = { + "clear_locals", (PyCFunction) clear_locals, METH_O + }; - PyObject *tdict = PyThreadState_GetDict(); - if (tdict == NULL) { - PyErr_SetString(PyExc_SystemError, - "Couldn't get thread-state dictionary"); - goto err; + PyThreadState *tstate = PyThreadState_Get(); + + /* We use a weak reference to self in the callback closure + in order to avoid spurious reference cycles */ + PyObject *self_wr = PyWeakref_NewRef((PyObject *) self, NULL); + if (self_wr == NULL) { + return NULL; } - ldict = PyDict_New(); - if (ldict == NULL) { - goto err; + PyObject *args = PyTuple_New(2); + if (args == NULL) { + Py_DECREF(self_wr); + return NULL; } - dummy = (localdummyobject *) type->tp_alloc(type, 0); - if (dummy == NULL) { - goto err; - } - dummy->localdict = ldict; - wr = PyWeakref_NewRef((PyObject *) dummy, self->wr_callback); - if (wr == NULL) { - goto err; + PyTuple_SET_ITEM(args, 0, self_wr); + PyTuple_SET_ITEM(args, 1, Py_NewRef(tstate->threading_local_key)); + + PyObject *cb = PyCFunction_New(&wr_callback_def, args); + Py_DECREF(args); + if (cb == NULL) { + return NULL; } - /* As a side-effect, this will cache the weakref's hash before the - dummy gets deleted */ - int r = PyDict_SetItem(self->dummies, wr, ldict); - if (r < 0) { - goto err; - } - Py_CLEAR(wr); - r = PyDict_SetItem(tdict, self->key, (PyObject *) dummy); - if (r < 0) { - goto err; - } - Py_CLEAR(dummy); + PyObject *wr = PyWeakref_NewRef(tstate->threading_local_sentinel, cb); + Py_DECREF(cb); - Py_DECREF(ldict); - return ldict; - -err: - Py_XDECREF(ldict); - Py_XDECREF(wr); - Py_XDECREF(dummy); - return NULL; + return wr; } static PyObject * local_new(PyTypeObject *type, PyObject *args, PyObject *kw) { - static PyMethodDef wr_callback_def = { - "_localdummy_destroyed", (PyCFunction) _localdummy_destroyed, METH_O - }; - if (type->tp_init == PyBaseObject_Type.tp_init) { int rc = 0; if (args != NULL) @@ -1513,30 +1504,25 @@ local_new(PyTypeObject *type, PyObject *args, PyObject *kw) self->args = Py_XNewRef(args); self->kw = Py_XNewRef(kw); - self->key = PyUnicode_FromFormat("thread.local.%p", self); - if (self->key == NULL) { + + self->localdicts = PyDict_New(); + if (self->localdicts == NULL) { goto err; } - self->dummies = PyDict_New(); - if (self->dummies == NULL) { + self->thread_watchdogs = PySet_New(NULL); + if (self->thread_watchdogs == NULL) { goto err; } - /* We use a weak reference to self in the callback closure - in order to avoid spurious reference cycles */ - PyObject *wr = PyWeakref_NewRef((PyObject *) self, NULL); - if (wr == NULL) { - goto err; - } - self->wr_callback = PyCFunction_NewEx(&wr_callback_def, wr, NULL); - Py_DECREF(wr); - if (self->wr_callback == NULL) { - goto err; - } - if (_local_create_dummy(self, state) == NULL) { + PyObject *localsdict = NULL; + PyObject *sentinel_wr = NULL; + if (create_localsdict(self, state, &localsdict, &sentinel_wr) < 0) { goto err; } + Py_DECREF(localsdict); + Py_DECREF(sentinel_wr); + return (PyObject *)self; err: @@ -1550,7 +1536,8 @@ local_traverse(localobject *self, visitproc visit, void *arg) Py_VISIT(Py_TYPE(self)); Py_VISIT(self->args); Py_VISIT(self->kw); - Py_VISIT(self->dummies); + Py_VISIT(self->localdicts); + Py_VISIT(self->thread_watchdogs); return 0; } @@ -1559,27 +1546,8 @@ local_clear(localobject *self) { Py_CLEAR(self->args); Py_CLEAR(self->kw); - Py_CLEAR(self->dummies); - Py_CLEAR(self->wr_callback); - /* Remove all strong references to dummies from the thread states */ - if (self->key) { - PyInterpreterState *interp = _PyInterpreterState_GET(); - _PyRuntimeState *runtime = &_PyRuntime; - HEAD_LOCK(runtime); - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); - HEAD_UNLOCK(runtime); - while (tstate) { - if (tstate->dict) { - if (PyDict_Pop(tstate->dict, self->key, NULL) < 0) { - // Silently ignore error - PyErr_Clear(); - } - } - HEAD_LOCK(runtime); - tstate = PyThreadState_Next(tstate); - HEAD_UNLOCK(runtime); - } - } + Py_CLEAR(self->localdicts); + Py_CLEAR(self->thread_watchdogs); return 0; } @@ -1595,48 +1563,142 @@ local_dealloc(localobject *self) PyObject_GC_UnTrack(self); local_clear(self); - Py_XDECREF(self->key); PyTypeObject *tp = Py_TYPE(self); tp->tp_free((PyObject*)self); Py_DECREF(tp); } -/* Returns a borrowed reference to the local dict, creating it if necessary */ +/* Create the TLS key and sentinel if they don't exist */ +static int +create_localdummies(thread_module_state *state) +{ + PyThreadState *tstate = _PyThreadState_GET(); + + if (tstate->threading_local_key != NULL) { + return 0; + } + + PyTypeObject *ld_type = state->local_dummy_type; + tstate->threading_local_key = ld_type->tp_alloc(ld_type, 0); + if (tstate->threading_local_key == NULL) { + return -1; + } + + tstate->threading_local_sentinel = ld_type->tp_alloc(ld_type, 0); + if (tstate->threading_local_sentinel == NULL) { + Py_CLEAR(tstate->threading_local_key); + return -1; + } + + return 0; +} + +/* Insert a localsdict and sentinel weakref for the current thread, placing + strong references in localsdict and sentinel_wr, respectively. +*/ +static int +create_localsdict(localobject *self, thread_module_state *state, + PyObject **localsdict, PyObject **sentinel_wr) +{ + PyThreadState *tstate = _PyThreadState_GET(); + PyObject *ldict = NULL; + PyObject *wr = NULL; + + if (create_localdummies(state) < 0) { + goto err; + } + + /* Create and insert the locals dict and sentinel weakref */ + ldict = PyDict_New(); + if (ldict == NULL) { + goto err; + } + + if (PyDict_SetItem(self->localdicts, tstate->threading_local_key, ldict) < + 0) { + goto err; + } + + wr = create_sentinel_wr(self); + if (wr == NULL) { + PyObject *exc = PyErr_GetRaisedException(); + if (PyDict_DelItem(self->localdicts, tstate->threading_local_key) < + 0) { + PyErr_WriteUnraisable((PyObject *)self); + } + PyErr_SetRaisedException(exc); + goto err; + } + + if (PySet_Add(self->thread_watchdogs, wr) < 0) { + PyObject *exc = PyErr_GetRaisedException(); + if (PyDict_DelItem(self->localdicts, tstate->threading_local_key) < + 0) { + PyErr_WriteUnraisable((PyObject *)self); + } + PyErr_SetRaisedException(exc); + goto err; + } + + *localsdict = ldict; + *sentinel_wr = wr; + return 0; + +err: + Py_XDECREF(ldict); + Py_XDECREF(wr); + return -1; +} + +/* Return a strong reference to the locals dict for the current thread, + creating it if necessary. +*/ static PyObject * _ldict(localobject *self, thread_module_state *state) { - PyObject *tdict = PyThreadState_GetDict(); - if (tdict == NULL) { - PyErr_SetString(PyExc_SystemError, - "Couldn't get thread-state dictionary"); + if (create_localdummies(state) < 0) { return NULL; } + /* Check if a localsdict already exists */ PyObject *ldict; - PyObject *dummy = PyDict_GetItemWithError(tdict, self->key); - if (dummy == NULL) { - if (PyErr_Occurred()) { - return NULL; - } - ldict = _local_create_dummy(self, state); - if (ldict == NULL) - return NULL; + PyThreadState *tstate = _PyThreadState_GET(); + if (PyDict_GetItemRef(self->localdicts, tstate->threading_local_key, + &ldict) < 0) { + return NULL; + } + if (ldict != NULL) { + return ldict; + } - if (Py_TYPE(self)->tp_init != PyBaseObject_Type.tp_init && - Py_TYPE(self)->tp_init((PyObject*)self, - self->args, self->kw) < 0) { - /* we need to get rid of ldict from thread so - we create a new one the next time we do an attr - access */ - PyDict_DelItem(tdict, self->key); - return NULL; + /* threading.local hasn't been instantiated for this thread */ + PyObject *wr; + if (create_localsdict(self, state, &ldict, &wr) < 0) { + return NULL; + } + + /* run __init__ if we're a subtype of `threading.local` */ + if (Py_TYPE(self)->tp_init != PyBaseObject_Type.tp_init && + Py_TYPE(self)->tp_init((PyObject *)self, self->args, self->kw) < 0) { + /* we need to get rid of ldict from thread so + we create a new one the next time we do an attr + access */ + PyObject *exc = PyErr_GetRaisedException(); + if (PyDict_DelItem(self->localdicts, tstate->threading_local_key) < + 0) { + PyErr_WriteUnraisable((PyObject *)self); + PyErr_Clear(); } + if (PySet_Discard(self->thread_watchdogs, wr) < 0) { + PyErr_WriteUnraisable((PyObject *)self); + } + PyErr_SetRaisedException(exc); + Py_DECREF(ldict); + Py_DECREF(wr); + return NULL; } - else { - assert(Py_IS_TYPE(dummy, state->local_dummy_type)); - ldict = ((localdummyobject *) dummy)->localdict; - } + Py_DECREF(wr); return ldict; } @@ -1650,21 +1712,28 @@ local_setattro(localobject *self, PyObject *name, PyObject *v) PyObject *ldict = _ldict(self, state); if (ldict == NULL) { - return -1; + goto err; } int r = PyObject_RichCompareBool(name, &_Py_ID(__dict__), Py_EQ); if (r == -1) { - return -1; + goto err; } if (r == 1) { PyErr_Format(PyExc_AttributeError, "'%.100s' object attribute '%U' is read-only", Py_TYPE(self)->tp_name, name); - return -1; + goto err; } - return _PyObject_GenericSetAttrWithDict((PyObject *)self, name, v, ldict); + int st = + _PyObject_GenericSetAttrWithDict((PyObject *)self, name, v, ldict); + Py_DECREF(ldict); + return st; + +err: + Py_XDECREF(ldict); + return -1; } static PyObject *local_getattro(localobject *, PyObject *); @@ -1707,34 +1776,42 @@ local_getattro(localobject *self, PyObject *name) int r = PyObject_RichCompareBool(name, &_Py_ID(__dict__), Py_EQ); if (r == 1) { - return Py_NewRef(ldict); + return ldict; } if (r == -1) { + Py_DECREF(ldict); return NULL; } if (!Py_IS_TYPE(self, state->local_type)) { /* use generic lookup for subtypes */ - return _PyObject_GenericGetAttrWithDict((PyObject *)self, name, - ldict, 0); + PyObject *res = + _PyObject_GenericGetAttrWithDict((PyObject *)self, name, ldict, 0); + Py_DECREF(ldict); + return res; } /* Optimization: just look in dict ourselves */ PyObject *value; if (PyDict_GetItemRef(ldict, name, &value) != 0) { // found or error + Py_DECREF(ldict); return value; } /* Fall back on generic to get __class__ and __dict__ */ - return _PyObject_GenericGetAttrWithDict( - (PyObject *)self, name, ldict, 0); + PyObject *res = + _PyObject_GenericGetAttrWithDict((PyObject *)self, name, ldict, 0); + Py_DECREF(ldict); + return res; } -/* Called when a dummy is destroyed. */ +/* Called when a dummy is destroyed, indicating that the owning thread is being + * cleared. */ static PyObject * -_localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref) +clear_locals(PyObject *locals_and_key, PyObject *dummyweakref) { + PyObject *localweakref = PyTuple_GetItem(locals_and_key, 0); localobject *self = (localobject *)_PyWeakref_GET_REF(localweakref); if (self == NULL) { Py_RETURN_NONE; @@ -1742,11 +1819,18 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref) /* If the thread-local object is still alive and not being cleared, remove the corresponding local dict */ - if (self->dummies != NULL) { - if (PyDict_Pop(self->dummies, dummyweakref, NULL) < 0) { + if (self->localdicts != NULL) { + PyObject *key = PyTuple_GetItem(locals_and_key, 1); + if (PyDict_Pop(self->localdicts, key, NULL) < 0) { PyErr_WriteUnraisable((PyObject*)self); } } + if (self->thread_watchdogs != NULL) { + if (PySet_Discard(self->thread_watchdogs, dummyweakref) < 0) { + PyErr_WriteUnraisable((PyObject *)self); + } + } + Py_DECREF(self); Py_RETURN_NONE; } diff --git a/Python/pystate.c b/Python/pystate.c index 7a272de11ec..f0452aa3cce 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1702,6 +1702,9 @@ PyThreadState_Clear(PyThreadState *tstate) /* Don't clear tstate->pyframe: it is a borrowed reference */ + Py_CLEAR(tstate->threading_local_key); + Py_CLEAR(tstate->threading_local_sentinel); + Py_CLEAR(((_PyThreadStateImpl *)tstate)->asyncio_running_loop); Py_CLEAR(tstate->dict);