gh-129069: make list ass_slice and memory_repeat safe in free-threading

Include/cpython/pyatomic.h

@@ -87,6 +87,8 @@
 # error "this header file must not be included directly"
 #endif
 
+#include <assert.h>
+
 // --- _Py_atomic_add --------------------------------------------------------
 // Atomically adds `value` to `obj` and returns the previous value
 
@@ -545,6 +547,63 @@ static inline Py_ssize_t
 _Py_atomic_load_ssize_acquire(const Py_ssize_t *obj);
 
 
+// --- _Py_atomic_memcpy / _Py_atomic_memmove ------------
+
+static inline void *
+_Py_atomic_memcpy_ptr_store_relaxed(void *dest, void *src, size_t n)
+{
+ void **dest_, **src_, **end;
+
+ assert(((uintptr_t)dest & (uintptr_t)(sizeof (void *) - 1)) == 0);
+ assert(((uintptr_t)src & (uintptr_t)(sizeof (void *) - 1)) == 0);
+ assert(n % (size_t)sizeof(void *) == 0);
+
+ if (dest != src) {
+ dest_ = (void **)dest;
+ src_ = (void **)src;
+ end = dest_ + n / sizeof(void *);
+
+ for (; dest_ != end; dest_++, src_++) {
+ _Py_atomic_store_ptr_relaxed(dest_, *src_);
+ }
+ }
+
+ return dest;
+}
+
+static inline void *
+_Py_atomic_memmove_ptr_store_relaxed(void *dest, void *src, size_t n)
+{
+ void **dest_, **src_, **end;
+
+ assert(((uintptr_t)dest & (uintptr_t)(sizeof (void *) - 1)) == 0);
+ assert(((uintptr_t)src & (uintptr_t)(sizeof (void *) - 1)) == 0);
+ assert(n % (size_t)sizeof(void *) == 0);
+
+ if (dest < src || dest >= (void *)((char *)src + n)) {
+ dest_ = (void **)dest;
+ src_ = (void **)src;
+ end = dest_ + n / sizeof(void *);
+
+ for (; dest_ != end; dest_++, src_++) {
+ _Py_atomic_store_ptr_relaxed(dest_, *src_);
+ }
+ }
+ else if (dest > src) {
+ n = n / sizeof(void *) - 1;
+ dest_ = (void **)dest + n;
+ src_ = (void **)src + n;
+ end = (void **)dest - 1;
+
+ for (; dest_ != end; dest_--, src_--) {
+ _Py_atomic_store_ptr_relaxed(dest_, *src_);
+ }
+ }
+
+ return dest;
+}
+
+
 
 
 // --- _Py_atomic_fence ------------------------------------------------------

Include/internal/pycore_list.h

@@ -12,6 +12,8 @@ extern "C" {
 #include "pycore_stackref.h"
 #endif
 
+#include "pycore_pyatomic_ft_wrappers.h"
+
 PyAPI_FUNC(PyObject*) _PyList_Extend(PyListObject *, PyObject *);
 PyAPI_FUNC(PyObject) *_PyList_SliceSubscript(PyObject*, PyObject*);
 extern void _PyList_DebugMallocStats(FILE *out);
@@ -51,15 +53,17 @@ _PyList_AppendTakeRef(PyListObject *self, PyObject *newitem)
  return _PyList_AppendTakeRefListResize(self, newitem);
 }
 
-// Repeat the bytes of a buffer in place
+// Repeat the bytes of a buffer of pointers in place
 static inline void
-_Py_memory_repeat(char* dest, Py_ssize_t len_dest, Py_ssize_t len_src)
+_Py_memory_ptrs_repeat(char* dest, Py_ssize_t len_dest, Py_ssize_t len_src)
 {
  assert(len_src > 0);
+ assert(len_src % sizeof(void *) == 0);
+ assert(((uintptr_t)dest & (sizeof (void *) - 1)) == 0);
  Py_ssize_t copied = len_src;
  while (copied < len_dest) {
  Py_ssize_t bytes_to_copy = Py_MIN(copied, len_dest - copied);
-memcpy(dest + copied, dest, (size_t)bytes_to_copy);
+FT_ATOMIC_MEMCPY_PTR_STORE_RELAXED(dest + copied, dest, (size_t)bytes_to_copy);
  copied += bytes_to_copy;
  }
 }

Include/internal/pycore_pyatomic_ft_wrappers.h

@@ -114,6 +114,12 @@ extern "C" {
 #define FT_MUTEX_LOCK(lock) PyMutex_Lock(lock)
 #define FT_MUTEX_UNLOCK(lock) PyMutex_Unlock(lock)
 
+#define FT_ATOMIC_MEMCPY_PTR_STORE_RELAXED(dest, src, n) \
+ _Py_atomic_memcpy_ptr_store_relaxed(dest, src, (size_t)(n))
+#define FT_ATOMIC_MEMMOVE_PTR_STORE_RELAXED(dest, src, n) \
+ _Py_atomic_memmove_ptr_store_relaxed(dest, src, (size_t)(n))
+
+
 #else
 #define FT_ATOMIC_LOAD_PTR(value) value
 #define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value
@@ -164,6 +170,9 @@ extern "C" {
 #define FT_MUTEX_LOCK(lock) do {} while (0)
 #define FT_MUTEX_UNLOCK(lock) do {} while (0)
 
+#define FT_ATOMIC_MEMCPY_PTR_STORE_RELAXED(dest, src, n) memcpy(dest, src, n)
+#define FT_ATOMIC_MEMMOVE_PTR_STORE_RELAXED(dest, src, n) memmove(dest, src, n)
+
 #endif
 
 #ifdef __cplusplus

Lib/test/test_list.py

@@ -1,8 +1,10 @@
 import signal
 import sys
 import textwrap
+import threading
 from test import list_tests, support
 from test.support import cpython_only
+from test.support import threading_helper
 from test.support.import_helper import import_module
 from test.support.script_helper import assert_python_failure, assert_python_ok
 import pickle
@@ -379,6 +381,34 @@ def foo(x):
 
  self.assertEqual(foo(list(range(10))), 45)
 
+ @unittest.skipUnless(support.Py_GIL_DISABLED,
+ 'this test can only possibly fail with GIL disabled')
+ @threading_helper.reap_threads
+ @threading_helper.requires_working_threading()
+ def test_free_threading(self):
+ def read(done, l):
+ while not done.is_set():
+ for _ in range(100):
+ _ = l[1]
+
+ NUM_THREADS = 10
+ done = threading.Event()
+ threads = []
+ l = [None] * 500
+ d = [None] * 498
+
+ for _ in range(NUM_THREADS):
+ thread = threading.Thread(target=read, args=(done, l))
+
+ threads.append(thread)
+
+ with threading_helper.start_threads(threads):
+ for _ in range(100):
+ del l[:498]
+ l[1:-1] = d
+
+ done.set()
+
 
 if __name__ == "__main__":
  unittest.main()

Misc/NEWS.d/next/Core_and_Builtins/2025-03-29-20-12-28.gh-issue-129069.QwbbWV.rst

@@ -0,0 +1 @@
+Fix data race when assigning list slices concurrently.

Modules/_testcapi/pyatomic.c

@@ -5,6 +5,7 @@
  */
 
 #include "parts.h"
+#include "pyconfig.h" // SIZEOF_VOID_P
 
 // We define atomic bitwise operations on these types
 #define FOR_BITWISE_TYPES(V) \
@@ -156,6 +157,78 @@ test_atomic_load_store_int_release_acquire(PyObject *self, PyObject *obj) { \
  Py_RETURN_NONE;
 }
 
+static PyObject *
+test_atomic_memcpy_ptr_store_relaxed(PyObject *self, PyObject *obj) {
+#if SIZEOF_VOID_P == 8
+#define p0 (void *)0x5555555555555555
+#define p1 (void *)0xaaaaaaaaaaaaaaaa
+#define p2 (void *)0xfedcba9876543210
+#define p3 (void *)0x0123456789abcdef
+#else
+#if SIZEOF_VOID_P == 4
+#define p0 (void *)0x55555555
+#define p1 (void *)0xaaaaaaaa
+#define p2 (void *)0x76543210
+#define p3 (void *)0x01234567
+#else
+#error "unexpected sizeof(void *), expecting 8 or 4"
+#endif
+#endif
+ void *src[4] = { (void *)0, p2, p3, (void *)0 };
+ void *dst[4] = { p0, (void *)0, (void *)0, p1 };
+ assert(_Py_atomic_memcpy_ptr_store_relaxed(&dst[1], &src[1], SIZEOF_VOID_P * 2) == &dst[1]);
+ assert(dst[0] == p0);
+ assert(dst[1] == p2);
+ assert(dst[2] == p3);
+ assert(dst[3] == p1);
+ Py_RETURN_NONE;
+#undef p3
+#undef p2
+#undef p1
+#undef p0
+}
+
+static PyObject *
+test_atomic_memmove_ptr_store_relaxed(PyObject *self, PyObject *obj) {
+#if SIZEOF_VOID_P == 8
+#define p0 (void *)0x5555555555555555
+#define p1 (void *)0xaaaaaaaaaaaaaaaa
+#define p2 (void *)0xfedcba9876543210
+#define p3 (void *)0x0123456789abcdef
+#define p4 (void *)0x0f2d4b6987a5c3e1
+#else
+#if SIZEOF_VOID_P == 4
+#define p0 (void *)0x55555555
+#define p1 (void *)0xaaaaaaaa
+#define p2 (void *)0x76543210
+#define p3 (void *)0x01234567
+#define p4 (void *)0x07254361
+#else
+#error "unexpected sizeof(void *), expecting 8 or 4"
+#endif
+#endif
+ void *back[5] = { p0, p2, p3, p4, p1 };
+ assert(_Py_atomic_memmove_ptr_store_relaxed(&back[1], &back[2], SIZEOF_VOID_P * 2) == &back[1]);
+ assert(back[0] == p0);
+ assert(back[1] == p3);
+ assert(back[2] == p4);
+ assert(back[3] == p4);
+ assert(back[4] == p1);
+ void *fwd[5] = { p0, p2, p3, p4, p1 };
+ assert(_Py_atomic_memmove_ptr_store_relaxed(&fwd[2], &fwd[1], SIZEOF_VOID_P * 2) == &fwd[2]);
+ assert(fwd[0] == p0);
+ assert(fwd[1] == p2);
+ assert(fwd[2] == p2);
+ assert(fwd[3] == p3);
+ assert(fwd[4] == p1);
+ Py_RETURN_NONE;
+#undef p4
+#undef p3
+#undef p2
+#undef p1
+#undef p0
+}
+
 // NOTE: all tests should start with "test_atomic_" to be included
 // in test_pyatomic.py
 
@@ -179,6 +252,8 @@ static PyMethodDef test_methods[] = {
  {"test_atomic_fences", test_atomic_fences, METH_NOARGS},
  {"test_atomic_release_acquire", test_atomic_release_acquire, METH_NOARGS},
  {"test_atomic_load_store_int_release_acquire", test_atomic_load_store_int_release_acquire, METH_NOARGS},
+ {"test_atomic_memcpy_ptr_store_relaxed", test_atomic_memcpy_ptr_store_relaxed, METH_NOARGS},
+ {"test_atomic_memmove_ptr_store_relaxed", test_atomic_memmove_ptr_store_relaxed, METH_NOARGS},
  {NULL, NULL} /* sentinel */
 };
 

Objects/listobject.c

@@ -818,10 +818,8 @@ list_repeat_lock_held(PyListObject *a, Py_ssize_t n)
  _Py_RefcntAdd(*src, n);
  *dest++ = *src++;
  }
- // TODO: _Py_memory_repeat calls are not safe for shared lists in
- // GIL_DISABLED builds. (See issue #129069)
- _Py_memory_repeat((char *)np->ob_item, sizeof(PyObject *)*output_size,
- sizeof(PyObject *)*input_size);
+ _Py_memory_ptrs_repeat((char *)np->ob_item, sizeof(PyObject *)*output_size,
+ sizeof(PyObject *)*input_size);
  }
 
  Py_SET_SIZE(np, output_size);
@@ -954,12 +952,10 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO
  if (d < 0) { /* Delete -d items */
  Py_ssize_t tail;
  tail = (Py_SIZE(a) - ihigh) * sizeof(PyObject *);
- // TODO: these memmove/memcpy calls are not safe for shared lists in
- // GIL_DISABLED builds. (See issue #129069)
- memmove(&item[ihigh+d], &item[ihigh], tail);
+ FT_ATOMIC_MEMMOVE_PTR_STORE_RELAXED(&item[ihigh+d], &item[ihigh], tail);
  if (list_resize(a, Py_SIZE(a) + d) < 0) {
-memmove(&item[ihigh], &item[ihigh+d], tail);
-memcpy(&item[ilow], recycle, s);
+FT_ATOMIC_MEMMOVE_PTR_STORE_RELAXED(&item[ihigh], &item[ihigh+d], tail);
+FT_ATOMIC_MEMCPY_PTR_STORE_RELAXED(&item[ilow], recycle, s);
  goto Error;
  }
  item = a->ob_item;
@@ -969,10 +965,8 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO
  if (list_resize(a, k+d) < 0)
  goto Error;
  item = a->ob_item;
- // TODO: these memmove/memcpy calls are not safe for shared lists in
- // GIL_DISABLED builds. (See issue #129069)
- memmove(&item[ihigh+d], &item[ihigh],
- (k - ihigh)*sizeof(PyObject *));
+ FT_ATOMIC_MEMMOVE_PTR_STORE_RELAXED(&item[ihigh+d], &item[ihigh],
+ (k - ihigh)*sizeof(PyObject *));
  }
  for (k = 0; k < n; k++, ilow++) {
  PyObject *w = vitem[k];
@@ -1056,10 +1050,8 @@ list_inplace_repeat_lock_held(PyListObject *self, Py_ssize_t n)
  for (Py_ssize_t j = 0; j < input_size; j++) {
  _Py_RefcntAdd(items[j], n-1);
  }
- // TODO: _Py_memory_repeat calls are not safe for shared lists in
- // GIL_DISABLED builds. (See issue #129069)
- _Py_memory_repeat((char *)items, sizeof(PyObject *)*output_size,
- sizeof(PyObject *)*input_size);
+ _Py_memory_ptrs_repeat((char *)items, sizeof(PyObject *)*output_size,
+ sizeof(PyObject *)*input_size);
  return 0;
 }
 

Objects/tupleobject.c

@@ -5,7 +5,7 @@
 #include "pycore_ceval.h" // _PyEval_GetBuiltin()
 #include "pycore_freelist.h" // _Py_FREELIST_PUSH()
 #include "pycore_gc.h" // _PyObject_GC_IS_TRACKED()
-#include "pycore_list.h" // _Py_memory_repeat()
+#include "pycore_list.h" // _Py_memory_ptrs_repeat()
 #include "pycore_modsupport.h" // _PyArg_NoKwnames()
 #include "pycore_object.h" // _PyObject_GC_TRACK()
 #include "pycore_stackref.h" // PyStackRef_AsPyObjectSteal()
@@ -571,8 +571,8 @@ tuple_repeat(PyObject *self, Py_ssize_t n)
  *dest++ = *src++;
  }
 
-_Py_memory_repeat((char *)np->ob_item, sizeof(PyObject *)*output_size,
- sizeof(PyObject *)*input_size);
+_Py_memory_ptrs_repeat((char *)np->ob_item, sizeof(PyObject *)*output_size,
+sizeof(PyObject *)*input_size);
  }
  _PyObject_GC_TRACK(np);
  return (PyObject *) np;

Tools/tsan/suppressions_free_threading.txt

@@ -32,12 +32,6 @@ thread:pthread_create
 # Range iteration is not thread-safe yet (issue #129068)
 race_top:rangeiter_next
 
-# List resizing happens through different paths ending in memcpy or memmove
-# (for efficiency), which will probably need to rewritten as explicit loops
-# of ptr-sized copies to be thread-safe. (Issue #129069)
-race:list_ass_slice_lock_held
-race:list_inplace_repeat_lock_held
-
 # PyObject_Realloc internally does memcpy which isn't atomic so can race
 # with non-locking reads. See #132070
 race:PyObject_Realloc