Fix data races when writing / reading ob_gc_bits

Use relaxed atomics when reading / writing to the field. There are still a few places in the GC where we do not use atomics. Those should be safe as the world is stopped. Sample races reported by TSAN: * [Race between `_PyObject_GC_IS_SHARED()` and `_PyObject_GC_SET_SHARED()`](https://gist.github.com/mpage/ebfd022773f39c33bec68c0993a2249e) * [Race between `_PyObject_GC_TRACK()` and `_PyObject_GC_IS_SHARED()`](https://gist.github.com/mpage/b53b5c22a2f923bf1571b887bb31ef88)
colesbury · May 8, 2024 · Apr 25, 2024 · Apr 25, 2024 · May 6, 2024 · May 6, 2024
commit b9ffd58f73d25bd3a512b50cfb426ace3736ab47
diff --git a/Include/cpython/pyatomic.h b/Include/cpython/pyatomic.h
@@ -250,6 +250,14 @@ static inline uintptr_t
 _Py_atomic_and_uintptr(uintptr_t *obj, uintptr_t value);
 
 
+// --- _Py_atomic_and_relaxed ------------------------------------------------
+// Performs `*obj &= value` atomically and returns the previous value of `*obj`
+// (relaxed consistency).
+
+static inline uint8_t
+_Py_atomic_and_uint8_relaxed(uint8_t *obj, uint8_t value);
+
+
 // --- _Py_atomic_or ---------------------------------------------------------
 // Performs `*obj |= value` atomically and returns the previous value of `*obj`.
 
@@ -269,6 +277,13 @@ static inline uintptr_t
 _Py_atomic_or_uintptr(uintptr_t *obj, uintptr_t value);
 
 
+// --- _Py_atomic_or_relaxed -------------------------------------------------
+// Performs `*obj |= value` atomically and returns the previous value of `*obj`
+// (relaxed consistency).
+
+static inline uint8_t
+_Py_atomic_or_uint8_relaxed(uint8_t *obj, uint8_t value);
+
 // --- _Py_atomic_load -------------------------------------------------------
 // Atomically loads `*obj` (sequential consistency)
 

diff --git a/Include/cpython/pyatomic_gcc.h b/Include/cpython/pyatomic_gcc.h
@@ -218,6 +218,13 @@ _Py_atomic_and_uintptr(uintptr_t *obj, uintptr_t value)
 { return __atomic_fetch_and(obj, value, __ATOMIC_SEQ_CST); }
 
 
+// --- _Py_atomic_and_relaxed ------------------------------------------------
+
+static inline uint8_t
+_Py_atomic_and_uint8_relaxed(uint8_t *obj, uint8_t value)
+{ return __atomic_fetch_and(obj, value, __ATOMIC_RELAXED); }
+
+
 // --- _Py_atomic_or ---------------------------------------------------------
 
 static inline uint8_t
@@ -241,6 +248,15 @@ _Py_atomic_or_uintptr(uintptr_t *obj, uintptr_t value)
 { return __atomic_fetch_or(obj, value, __ATOMIC_SEQ_CST); }
 
 
+// --- _Py_atomic_or_relaxed -------------------------------------------------
+
+static inline uint8_t
+_Py_atomic_or_uint8_relaxed(uint8_t *obj, uint8_t value)
+{
+    return __atomic_fetch_or(obj, value, __ATOMIC_RELAXED);
+}
+
+
 // --- _Py_atomic_load -------------------------------------------------------
 
 static inline int

diff --git a/Include/cpython/pyatomic_msc.h b/Include/cpython/pyatomic_msc.h
@@ -450,6 +450,16 @@ _Py_atomic_and_uintptr(uintptr_t *obj, uintptr_t value)
 }
 
 
+// --- _Py_atomic_and_relaxed ------------------------------------------------
+
+static inline uint8_t
+_Py_atomic_and_uint8_relaxed(uint8_t *obj, uint8_t value)
+{
+    _Py_atomic_ASSERT_ARG_TYPE(char);
+    return (uint8_t)_InterlockedAnd8NoFence((volatile char *)obj, (char)value);
+}
+
+
 // --- _Py_atomic_or ---------------------------------------------------------
 
 static inline uint8_t
@@ -490,7 +500,6 @@ _Py_atomic_or_uint64(uint64_t *obj, uint64_t value)
 #endif
 }
 
-
 static inline uintptr_t
 _Py_atomic_or_uintptr(uintptr_t *obj, uintptr_t value)
 {
@@ -506,6 +515,16 @@ _Py_atomic_or_uintptr(uintptr_t *obj, uintptr_t value)
 }
 
 
+// --- _Py_atomic_or_relaxed -------------------------------------------------
+
+static inline uint8_t
+_Py_atomic_or_uint8_relaxed(uint8_t *obj, uint8_t value)
+{
+    _Py_atomic_ASSERT_ARG_TYPE(char);
+    return (uint8_t)_InterlockedOr8NoFence((volatile char *)obj, (char)value);
+}
+
+
 // --- _Py_atomic_load -------------------------------------------------------
 
 static inline uint8_t

diff --git a/Include/cpython/pyatomic_std.h b/Include/cpython/pyatomic_std.h
@@ -366,6 +366,16 @@ _Py_atomic_and_uintptr(uintptr_t *obj, uintptr_t value)
 }
 
 
+// --- _Py_atomic_and --------------------------------------------------------
+
+static inline uint8_t
+_Py_atomic_and_uint8_relaxed(uint8_t *obj, uint8_t value)
+{
+    _Py_USING_STD;
+    return atomic_fetch_and_explicit((_Atomic(uint8_t)*)obj, value, memory_order_relaxed);
+}
+
+
 // --- _Py_atomic_or ---------------------------------------------------------
 
 static inline uint8_t
@@ -404,6 +414,16 @@ _Py_atomic_or_uintptr(uintptr_t *obj, uintptr_t value)
 }
 
 
+// --- _Py_atomic_or_relaxed -------------------------------------------------
+
+static inline uint8_t
+_Py_atomic_or_uint8_relaxed(uint8_t *obj, uint8_t value)
+{
+    _Py_USING_STD;
+    return atomic_fetch_or_explicit((_Atomic(uint8_t)*)obj, value, memory_order_relaxed);
+}
+
+
 // --- _Py_atomic_load -------------------------------------------------------
 
 static inline int

diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h
@@ -48,10 +48,32 @@ static inline PyObject* _Py_FROM_GC(PyGC_Head *gc) {
 #  define _PyGC_BITS_DEFERRED       (64)    // Use deferred reference counting
 #endif
 
+#ifdef Py_GIL_DISABLED
+
+static inline void
+_PyObject_SET_GC_BITS(PyObject *op, uint8_t bits)
+{
+    _Py_atomic_or_uint8_relaxed(&op->ob_gc_bits, bits);
+}
+
+static inline int
+_PyObject_HAS_GC_BITS(PyObject *op, uint8_t bits)
+{
+    return (_Py_atomic_load_uint8_relaxed(&op->ob_gc_bits) & bits) != 0;
+}
+
+static inline void
+_PyObject_CLEAR_GC_BITS(PyObject *op, uint8_t bits)
+{
+    _Py_atomic_and_uint8_relaxed(&op->ob_gc_bits, ~bits);
+}
+
+#endif
+
 /* True if the object is currently tracked by the GC. */
 static inline int _PyObject_GC_IS_TRACKED(PyObject *op) {
 #ifdef Py_GIL_DISABLED
-    return (op->ob_gc_bits & _PyGC_BITS_TRACKED) != 0;
+    return _PyObject_HAS_GC_BITS(op, _PyGC_BITS_TRACKED);
 #else
     PyGC_Head *gc = _Py_AS_GC(op);
     return (gc->_gc_next != 0);
@@ -80,12 +102,12 @@ static inline int _PyObject_GC_MAY_BE_TRACKED(PyObject *obj) {
  * for calling _PyMem_FreeDelayed on the referenced
  * memory. */
 static inline int _PyObject_GC_IS_SHARED(PyObject *op) {
-    return (op->ob_gc_bits & _PyGC_BITS_SHARED) != 0;
+    return _PyObject_HAS_GC_BITS(op, _PyGC_BITS_SHARED);
 }
 #define _PyObject_GC_IS_SHARED(op) _PyObject_GC_IS_SHARED(_Py_CAST(PyObject*, op))
 
 static inline void _PyObject_GC_SET_SHARED(PyObject *op) {
-    op->ob_gc_bits |= _PyGC_BITS_SHARED;
+    _PyObject_SET_GC_BITS(op, _PyGC_BITS_SHARED);
 }
 #define _PyObject_GC_SET_SHARED(op) _PyObject_GC_SET_SHARED(_Py_CAST(PyObject*, op))
 
@@ -95,13 +117,13 @@ static inline void _PyObject_GC_SET_SHARED(PyObject *op) {
  * Objects with this bit that are GC objects will automatically
  * delay-freed by PyObject_GC_Del. */
 static inline int _PyObject_GC_IS_SHARED_INLINE(PyObject *op) {
-    return (op->ob_gc_bits & _PyGC_BITS_SHARED_INLINE) != 0;
+    return _PyObject_HAS_GC_BITS(op, _PyGC_BITS_SHARED_INLINE);
 }
 #define _PyObject_GC_IS_SHARED_INLINE(op) \
     _PyObject_GC_IS_SHARED_INLINE(_Py_CAST(PyObject*, op))
 
 static inline void _PyObject_GC_SET_SHARED_INLINE(PyObject *op) {
-    op->ob_gc_bits |= _PyGC_BITS_SHARED_INLINE;
+    _PyObject_SET_GC_BITS(op, _PyGC_BITS_SHARED_INLINE);
 }
 #define _PyObject_GC_SET_SHARED_INLINE(op) \
     _PyObject_GC_SET_SHARED_INLINE(_Py_CAST(PyObject*, op))
@@ -178,23 +200,23 @@ static inline void _PyGCHead_SET_PREV(PyGC_Head *gc, PyGC_Head *prev) {
 
 static inline int _PyGC_FINALIZED(PyObject *op) {
 #ifdef Py_GIL_DISABLED
-    return (op->ob_gc_bits & _PyGC_BITS_FINALIZED) != 0;
+    return _PyObject_HAS_GC_BITS(op, _PyGC_BITS_FINALIZED);
 #else
     PyGC_Head *gc = _Py_AS_GC(op);
     return ((gc->_gc_prev & _PyGC_PREV_MASK_FINALIZED) != 0);
 #endif
 }
 static inline void _PyGC_SET_FINALIZED(PyObject *op) {
 #ifdef Py_GIL_DISABLED
-    op->ob_gc_bits |= _PyGC_BITS_FINALIZED;
+    _PyObject_SET_GC_BITS(op, _PyGC_BITS_FINALIZED);
 #else
     PyGC_Head *gc = _Py_AS_GC(op);
     gc->_gc_prev |= _PyGC_PREV_MASK_FINALIZED;
 #endif
 }
 static inline void _PyGC_CLEAR_FINALIZED(PyObject *op) {
 #ifdef Py_GIL_DISABLED
-    op->ob_gc_bits &= ~_PyGC_BITS_FINALIZED;
+    _PyObject_CLEAR_GC_BITS(op, _PyGC_BITS_FINALIZED);
 #else
     PyGC_Head *gc = _Py_AS_GC(op);
     gc->_gc_prev &= ~_PyGC_PREV_MASK_FINALIZED;

diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h
@@ -168,7 +168,7 @@ static inline int
 _PyObject_HasDeferredRefcount(PyObject *op)
 {
 #ifdef Py_GIL_DISABLED
-    return (op->ob_gc_bits & _PyGC_BITS_DEFERRED) != 0;
+    return _PyObject_HAS_GC_BITS(op, _PyGC_BITS_DEFERRED);
 #else
     return 0;
 #endif
@@ -320,7 +320,7 @@ static inline void _PyObject_GC_TRACK(
                           "object already tracked by the garbage collector",
                           filename, lineno, __func__);
 #ifdef Py_GIL_DISABLED
-    op->ob_gc_bits |= _PyGC_BITS_TRACKED;
+    _PyObject_SET_GC_BITS(op, _PyGC_BITS_TRACKED);
 #else
     PyGC_Head *gc = _Py_AS_GC(op);
     _PyObject_ASSERT_FROM(op,
@@ -361,7 +361,7 @@ static inline void _PyObject_GC_UNTRACK(
                           filename, lineno, __func__);
 
 #ifdef Py_GIL_DISABLED
-    op->ob_gc_bits &= ~_PyGC_BITS_TRACKED;
+    _PyObject_CLEAR_GC_BITS(op, _PyGC_BITS_TRACKED);
 #else
     PyGC_Head *gc = _Py_AS_GC(op);
     PyGC_Head *prev = _PyGCHead_PREV(gc);

@@ -2430,7 +2430,7 @@ _PyObject_SetDeferredRefcount(PyObject *op)
     assert(PyType_IS_GC(Py_TYPE(op)));
     assert(_Py_IsOwnedByCurrentThread(op));
     assert(op->ob_ref_shared == 0);
-    op->ob_gc_bits |= _PyGC_BITS_DEFERRED;
+    _PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED);
     op->ob_ref_local += 1;
     op->ob_ref_shared = _Py_REF_QUEUED;
 #endif