gh-117657: Fix TSAN list set failure (#118260)

DinoV · web-flow · commit 1e67b9207c31 · 2024-05-02T13:03:05.000-07:00
* Fix TSAN list set failure

* Relaxed atomic is sufficient, add targetted test

* More list

* Remove atomic assign in list

* Fixup white space
diff --git a/Include/internal/pycore_list.h b/Include/internal/pycore_list.h
@@ -28,7 +28,11 @@ _PyList_AppendTakeRef(PyListObject *self, PyObject *newitem)
     Py_ssize_t allocated = self->allocated;
     assert((size_t)len + 1 < PY_SSIZE_T_MAX);
     if (allocated > len) {
+#ifdef Py_GIL_DISABLED
+        _Py_atomic_store_ptr_release(&self->ob_item[len], newitem);
+#else
         PyList_SET_ITEM(self, len, newitem);
+#endif
         Py_SET_SIZE(self, len + 1);
         return 0;
     }
diff --git a/Lib/test/test_free_threading/test_list.py b/Lib/test/test_free_threading/test_list.py
@@ -0,0 +1,80 @@
+import unittest
+
+from threading import Thread
+from unittest import TestCase
+
+from test.support import is_wasi
+
+
+class C:
+    def __init__(self, v):
+        self.v = v
+
+
+@unittest.skipIf(is_wasi, "WASI has no threads.")
+class TestList(TestCase):
+    def test_racing_iter_append(self):
+
+        l = []
+        OBJECT_COUNT = 10000
+
+        def writer_func():
+            for i in range(OBJECT_COUNT):
+                l.append(C(i + OBJECT_COUNT))
+
+        def reader_func():
+            while True:
+                count = len(l)
+                for i, x in enumerate(l):
+                    self.assertEqual(x.v, i + OBJECT_COUNT)
+                if count == OBJECT_COUNT:
+                    break
+
+        writer = Thread(target=writer_func)
+        readers = []
+        for x in range(30):
+            reader = Thread(target=reader_func)
+            readers.append(reader)
+            reader.start()
+
+        writer.start()
+        writer.join()
+        for reader in readers:
+            reader.join()
+
+    def test_racing_iter_extend(self):
+        iters = [
+            lambda x: [x],
+        ]
+        for iter_case in iters:
+            with self.subTest(iter=iter_case):
+                l = []
+                OBJECT_COUNT = 10000
+
+                def writer_func():
+                    for i in range(OBJECT_COUNT):
+                        l.extend(iter_case(C(i + OBJECT_COUNT)))
+
+                def reader_func():
+                    while True:
+                        count = len(l)
+                        for i, x in enumerate(l):
+                            self.assertEqual(x.v, i + OBJECT_COUNT)
+                        if count == OBJECT_COUNT:
+                            break
+
+                writer = Thread(target=writer_func)
+                readers = []
+                for x in range(30):
+                    reader = Thread(target=reader_func)
+                    readers.append(reader)
+                    reader.start()
+
+                writer.start()
+                writer.join()
+                for reader in readers:
+                    reader.join()
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/Objects/listobject.c b/Objects/listobject.c
@@ -141,6 +141,9 @@ list_resize(PyListObject *self, Py_ssize_t newsize)
             target_bytes = allocated * sizeof(PyObject*);
         }
         memcpy(array->ob_item, self->ob_item, target_bytes);
+    }
+    if (new_allocated > (size_t)allocated) {
+        memset(array->ob_item + allocated, 0, sizeof(PyObject *) * (new_allocated - allocated));
     }
      _Py_atomic_store_ptr_release(&self->ob_item, &array->ob_item);
     self->allocated = new_allocated;
@@ -502,7 +505,7 @@ _PyList_AppendTakeRefListResize(PyListObject *self, PyObject *newitem)
         Py_DECREF(newitem);
         return -1;
     }
-    PyList_SET_ITEM(self, len, newitem);
+    FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], newitem);
     return 0;
 }
 
@@ -1181,7 +1184,7 @@ list_extend_fast(PyListObject *self, PyObject *iterable)
     PyObject **dest = self->ob_item + m;
     for (Py_ssize_t i = 0; i < n; i++) {
         PyObject *o = src[i];
-        dest[i] = Py_NewRef(o);
+        FT_ATOMIC_STORE_PTR_RELEASE(dest[i], Py_NewRef(o));
     }
     return 0;
 }
@@ -1238,7 +1241,7 @@ list_extend_iter_lock_held(PyListObject *self, PyObject *iterable)
 
         if (Py_SIZE(self) < self->allocated) {
             Py_ssize_t len = Py_SIZE(self);
-            PyList_SET_ITEM(self, len, item);  // steals item ref
+            FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], item);  // steals item ref
             Py_SET_SIZE(self, len + 1);
         }
         else {

Original file line number	Diff line number	Diff line change
`@@ -141,6 +141,9 @@ list_resize(PyListObject *self, Py_ssize_t newsize)`
`141`	`141`	`target_bytes = allocated * sizeof(PyObject*);`
`142`	`142`	`}`
`143`	`143`	`memcpy(array->ob_item, self->ob_item, target_bytes);`
	`144`	`+ }`
	`145`	`+ if (new_allocated > (size_t)allocated) {`
	`146`	`+ memset(array->ob_item + allocated, 0, sizeof(PyObject ) (new_allocated - allocated));`
`144`	`147`	`}`
`145`	`148`	`_Py_atomic_store_ptr_release(&self->ob_item, &array->ob_item);`
`146`	`149`	`self->allocated = new_allocated;`
`@@ -502,7 +505,7 @@ _PyList_AppendTakeRefListResize(PyListObject self, PyObject newitem)`
`502`	`505`	`Py_DECREF(newitem);`
`503`	`506`	`return -1;`
`504`	`507`	`}`
`505`		`- PyList_SET_ITEM(self, len, newitem);`
	`508`	`+ FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], newitem);`
`506`	`509`	`return 0;`
`507`	`510`	`}`
`508`	`511`
`@@ -1181,7 +1184,7 @@ list_extend_fast(PyListObject self, PyObject iterable)`
`1181`	`1184`	`PyObject **dest = self->ob_item + m;`
`1182`	`1185`	`for (Py_ssize_t i = 0; i < n; i++) {`
`1183`	`1186`	`PyObject *o = src[i];`
`1184`		`- dest[i] = Py_NewRef(o);`
	`1187`	`+ FT_ATOMIC_STORE_PTR_RELEASE(dest[i], Py_NewRef(o));`
`1185`	`1188`	`}`
`1186`	`1189`	`return 0;`
`1187`	`1190`	`}`
`@@ -1238,7 +1241,7 @@ list_extend_iter_lock_held(PyListObject self, PyObject iterable)`
`1238`	`1241`
`1239`	`1242`	`if (Py_SIZE(self) < self->allocated) {`
`1240`	`1243`	`Py_ssize_t len = Py_SIZE(self);`
`1241`		`- PyList_SET_ITEM(self, len, item); // steals item ref`
	`1244`	`+ FT_ATOMIC_STORE_PTR_RELEASE(self->ob_item[len], item); // steals item ref`
`1242`	`1245`	`Py_SET_SIZE(self, len + 1);`
`1243`	`1246`	`}`
`1244`	`1247`	`else {`