gh-117657: TSAN Fix race in PyMember_Get and PyMember_Set, for Py_T_OBJECT_EX type

Include/internal/pycore_pyatomic_ft_wrappers.h

@@ -59,6 +59,8 @@ extern "C" {
     _Py_atomic_store_uint16_relaxed(&value, new_value)
 #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) \
     _Py_atomic_store_uint32_relaxed(&value, new_value)
+#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \
+    _Py_atomic_exchange_ptr(value, new_value)
 
 #else
 #define FT_ATOMIC_LOAD_PTR(value) value
@@ -83,6 +85,17 @@ extern "C" {
 #define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value
 #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value
 
+static inline void *
+_Py_non_atomic_exchange_ptr(void **value, void *new_value)
+{
+    void *current = *value;
+    *value = new_value;
+    return current;
+}
+
+#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \
+    _Py_non_atomic_exchange_ptr(value, new_value)
+
 #endif
 
 #ifdef __cplusplus

Lib/test/test_free_threading/test_slots.py

@@ -0,0 +1,65 @@
+import threading
+from test.support import threading_helper
+from unittest import TestCase
+
+
+# import _testcapi
+
+
+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",  # the Py_T_OBJECT_EX type is missing in
+                         # Modules/_testcapi/structmember.c
+            ]
+
+            def __init__(self, initial_value):
+                self.eggs = initial_value
+
+        spam = Spam(0)
+        iters = 1_000_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])
+
+    # def test_bool(self):
+    #     spam_old = _testcapi._test_structmembersType_OldAPI()
+    #     spam_new = _testcapi._test_structmembersType_NewAPI()
+    #
+    #     def writer():
+    #         for _ in range(1_000):
+    #             spam_old.T_BOOL = True
+    #             spam_old.T_BOOL = False  # separate code path for True/False
+    #             spam_new.T_BOOL = True
+    #             spam_new.T_BOOL = False
+    #
+    #     def reader():
+    #         for _ in range(1_000):
+    #             spam_old.T_BOOL
+    #             spam_new.T_BOOL
+    #
+    #     run_in_threads([writer, reader, reader, reader])

Python/structmember.c

@@ -4,6 +4,26 @@
 #include "Python.h"
 #include "pycore_abstract.h"      // _PyNumber_Index()
 #include "pycore_long.h"          // _PyLong_IsNegative()
+#include "pycore_pyatomic_ft_wrappers.h"
+#include "pycore_object.h"
+#include "pycore_critical_section.h"
+
+
+static inline PyObject *
+_PyMember_GetOneObject(const char *addr, const char *obj_addr, PyMemberDef *l)
+{
+    PyObject *v = FT_ATOMIC_LOAD_PTR(*(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);
+    }
+    return v;
+}
+
+
 
 
 PyObject *
@@ -75,15 +95,18 @@ 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);
-        }
+#ifndef Py_GIL_DISABLED
+        v = _PyMember_GetOneObject(addr, obj_addr, l);
         Py_XINCREF(v);
+#else
+            v = _PyMember_GetOneObject(addr, obj_addr, l);
+            if (v != NULL && !_Py_TryIncrefCompare((PyObject **) addr, v)) {
+                Py_BEGIN_CRITICAL_SECTION((PyObject *)obj_addr);
+                        v = _PyMember_GetOneObject(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 +115,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 +142,7 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
         return -1;
     }
 
+    PyObject *obj = (PyObject *) addr;
     addr += l->offset;
 
     if ((l->flags & Py_READONLY))
@@ -281,8 +306,14 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
         break;
     case _Py_T_OBJECT:
     case Py_T_OBJECT_EX:
-        oldv = *(PyObject **)addr;
-        *(PyObject **)addr = Py_XNewRef(v);
+#ifdef Py_GIL_DISABLED
+        Py_BEGIN_CRITICAL_SECTION(obj);
+#endif
+        oldv = FT_ATOMIC_LOAD_PTR(*(PyObject **)addr);
+        FT_ATOMIC_STORE_PTR(*(PyObject **)addr, Py_XNewRef(v));
+#ifdef Py_GIL_DISABLED
+        Py_END_CRITICAL_SECTION();
+#endif
         Py_XDECREF(oldv);
         break;
     case Py_T_CHAR: {

Tools/tsan/suppressions_free_threading.txt

@@ -41,8 +41,6 @@ race_top:gc_restore_tid
 race_top:insertdict
 race_top:lookup_tp_dict
 race_top:mi_heap_visit_pages
-race_top:PyMember_GetOne
-race_top:PyMember_SetOne
 race_top:new_reference
 race_top:set_contains_key
 race_top:set_inheritable