From 396f8b0b98441344e1d3223a4075e5e342e0c2df Mon Sep 17 00:00:00 2001 From: "Miss Islington (bot)" <31488909+miss-islington@users.noreply.github.com> Date: Mon, 17 Jun 2024 21:12:25 +0200 Subject: [PATCH] [3.13] gh-117657: Fix `__slots__` thread safety in free-threaded build (GH-119368) (#120655) Fix a race in `PyMember_GetOne` and `PyMember_SetOne` for `Py_T_OBJECT_EX`. These functions implement `__slots__` accesses for Python objects. (cherry picked from commit 362cd2680b45a36c3467b9721ff7fc0ceb338452) Co-authored-by: Daniele Parmeggiani <8658291+dpdani@users.noreply.github.com> --- Lib/test/test_descr.py | 2 +- Lib/test/test_free_threading/test_slots.py | 43 ++++++++++++++++++++++ Python/structmember.c | 42 ++++++++++++++++----- Tools/tsan/suppressions_free_threading.txt | 2 - 4 files changed, 77 insertions(+), 12 deletions(-) create mode 100644 Lib/test/test_free_threading/test_slots.py diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index c3f292467a6..14bd87eb9c8 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -1314,7 +1314,7 @@ class ClassPropertiesAndMethods(unittest.TestCase): # Inherit from object on purpose to check some backwards compatibility paths class X(object): __slots__ = "a" - with self.assertRaisesRegex(AttributeError, "'X' object has no attribute 'a'"): + with self.assertRaisesRegex(AttributeError, "'test.test_descr.ClassPropertiesAndMethods.test_slots..X' object has no attribute 'a'"): X().a # Test string subclass in `__slots__`, see gh-98783 diff --git a/Lib/test/test_free_threading/test_slots.py b/Lib/test/test_free_threading/test_slots.py new file mode 100644 index 00000000000..758f74f54d0 --- /dev/null +++ b/Lib/test/test_free_threading/test_slots.py @@ -0,0 +1,43 @@ +import threading +from test.support import threading_helper +from unittest import TestCase + + +def run_in_threads(targets): + """Run `targets` in separate threads""" + threads = [ + threading.Thread(target=target) + for target in targets + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + +@threading_helper.requires_working_threading() +class TestSlots(TestCase): + + def test_object(self): + class Spam: + __slots__ = [ + "eggs", + ] + + def __init__(self, initial_value): + self.eggs = initial_value + + spam = Spam(0) + iters = 20_000 + + def writer(): + for _ in range(iters): + spam.eggs += 1 + + def reader(): + for _ in range(iters): + eggs = spam.eggs + assert type(eggs) is int + assert 0 <= eggs <= iters + + run_in_threads([writer, reader, reader, reader]) diff --git a/Python/structmember.c b/Python/structmember.c index ba881d18a09..d5e7ab83093 100644 --- a/Python/structmember.c +++ b/Python/structmember.c @@ -4,8 +4,22 @@ #include "Python.h" #include "pycore_abstract.h" // _PyNumber_Index() #include "pycore_long.h" // _PyLong_IsNegative() +#include "pycore_object.h" // _Py_TryIncrefCompare(), FT_ATOMIC_*() +#include "pycore_critical_section.h" +static inline PyObject * +member_get_object(const char *addr, const char *obj_addr, PyMemberDef *l) +{ + PyObject *v = FT_ATOMIC_LOAD_PTR(*(PyObject **) addr); + if (v == NULL) { + PyErr_Format(PyExc_AttributeError, + "'%T' object has no attribute '%s'", + (PyObject *)obj_addr, l->name); + } + return v; +} + PyObject * PyMember_GetOne(const char *obj_addr, PyMemberDef *l) { @@ -75,15 +89,19 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) Py_INCREF(v); break; case Py_T_OBJECT_EX: - v = *(PyObject **)addr; - if (v == NULL) { - PyObject *obj = (PyObject *)obj_addr; - PyTypeObject *tp = Py_TYPE(obj); - PyErr_Format(PyExc_AttributeError, - "'%.200s' object has no attribute '%s'", - tp->tp_name, l->name); - } + v = member_get_object(addr, obj_addr, l); +#ifndef Py_GIL_DISABLED Py_XINCREF(v); +#else + if (v != NULL) { + if (!_Py_TryIncrefCompare((PyObject **) addr, v)) { + Py_BEGIN_CRITICAL_SECTION((PyObject *) obj_addr); + v = member_get_object(addr, obj_addr, l); + Py_XINCREF(v); + Py_END_CRITICAL_SECTION(); + } + } +#endif break; case Py_T_LONGLONG: v = PyLong_FromLongLong(*(long long *)addr); @@ -92,6 +110,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l) v = PyLong_FromUnsignedLongLong(*(unsigned long long *)addr); break; case _Py_T_NONE: + // doesn't require free-threading code path v = Py_NewRef(Py_None); break; default: @@ -118,6 +137,9 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) return -1; } +#ifdef Py_GIL_DISABLED + PyObject *obj = (PyObject *) addr; +#endif addr += l->offset; if ((l->flags & Py_READONLY)) @@ -281,8 +303,10 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v) break; case _Py_T_OBJECT: case Py_T_OBJECT_EX: + Py_BEGIN_CRITICAL_SECTION(obj); oldv = *(PyObject **)addr; - *(PyObject **)addr = Py_XNewRef(v); + FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v)); + Py_END_CRITICAL_SECTION(); Py_XDECREF(oldv); break; case Py_T_CHAR: { diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index b86c8fce11f..2986efe6774 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -29,8 +29,6 @@ race_top:_PyEval_EvalFrameDefault race_top:assign_version_tag race_top:insertdict race_top:lookup_tp_dict -race_top:PyMember_GetOne -race_top:PyMember_SetOne race_top:new_reference race_top:set_contains_key # https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35