gh-117657: Fix __slots__ thread safety in free-threaded build (#119368)

dpdani · web-flow · commit 362cd2680b45 · 2024-06-17T18:44:54.000Z
Fix a race in `PyMember_GetOne` and `PyMember_SetOne` for `Py_T_OBJECT_EX`.
These functions implement `__slots__` accesses for Python objects.
diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py
@@ -1314,7 +1314,7 @@ class X(object):
         # 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.<locals>.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
@@ -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
@@ -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
@@ -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
