gh-117657: Fix data races when writing / reading `ob_gc_bits` (#118292)

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.

diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h
index 281094df786735c788d647d40f24dc9129ac1e82..60582521db5bd78438ee113e9785fd7f5f4dd94a 100644 (file)
--- a/Include/internal/pycore_gc.h
+++ b/Include/internal/pycore_gc.h
@@ -37,7 +37,15 @@ static inline PyObject* _Py_FROM_GC(PyGC_Head *gc) {
 }
 
 
-/* Bit flags for ob_gc_bits (in Py_GIL_DISABLED builds) */
+/* Bit flags for ob_gc_bits (in Py_GIL_DISABLED builds)
+ *
+ * Setting the bits requires a relaxed store. The per-object lock must also be
+ * held, except when the object is only visible to a single thread (e.g. during
+ * object initialization or destruction).
+ *
+ * Reading the bits requires using a relaxed load, but does not require holding
+ * the per-object lock.
+ */
 #ifdef Py_GIL_DISABLED
 #  define _PyGC_BITS_TRACKED        (1)     // Tracked by the GC
 #  define _PyGC_BITS_FINALIZED      (2)     // tp_finalize was called
@@ -48,10 +56,34 @@ 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 new_bits)
+{
+    uint8_t bits = _Py_atomic_load_uint8_relaxed(&op->ob_gc_bits);
+    _Py_atomic_store_uint8_relaxed(&op->ob_gc_bits, bits | new_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_to_clear)
+{
+    uint8_t bits = _Py_atomic_load_uint8_relaxed(&op->ob_gc_bits);
+    _Py_atomic_store_uint8_relaxed(&op->ob_gc_bits, bits & ~bits_to_clear);
+}
+
+#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 +112,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 +127,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,7 +210,7 @@ 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);
@@ -186,7 +218,7 @@ static inline int _PyGC_FINALIZED(PyObject *op) {
 }
 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;
@@ -194,7 +226,7 @@ static inline void _PyGC_SET_FINALIZED(PyObject *op) {
 }
 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
index f15c332a7b0811ef493a37b78a640a8d78b86b4a..7602248f956405f5df78504c6adb1842eeadc22c 100644 (file)
--- 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);

diff --git a/Objects/object.c b/Objects/object.c
index 29183dcc5b4a010e7616628984feee83103d7fa1..d4fe14c5b3d1aa6ab6a7ae790514c774ae3c2cc3 100644 (file)
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -2427,6 +2427,7 @@ _PyObject_SetDeferredRefcount(PyObject *op)
     assert(PyType_IS_GC(Py_TYPE(op)));
     assert(_Py_IsOwnedByCurrentThread(op));
     assert(op->ob_ref_shared == 0);
+    _PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED);
     PyInterpreterState *interp = _PyInterpreterState_GET();
     if (interp->gc.immortalize.enabled) {
         // gh-117696: immortalize objects instead of using deferred reference
@@ -2434,7 +2435,6 @@ _PyObject_SetDeferredRefcount(PyObject *op)
         _Py_SetImmortal(op);
         return;
     }
-    op->ob_gc_bits |= _PyGC_BITS_DEFERRED;
     op->ob_ref_local += 1;
     op->ob_ref_shared = _Py_REF_QUEUED;
 #endif

diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt
index 74dbf4bb1cb6882cad8f2861d41274a414966773..a669bc4d1d5c30b9d6a35cd1434c18870962411a 100644 (file)
--- a/Tools/tsan/suppressions_free_threading.txt
+++ b/Tools/tsan/suppressions_free_threading.txt
@@ -19,9 +19,6 @@ race:_PyImport_AcquireLock
 race:_PyImport_ReleaseLock
 race:_PyInterpreterState_SetNotRunningMain
 race:_PyInterpreterState_IsRunningMain
-race:_PyObject_GC_IS_SHARED
-race:_PyObject_GC_SET_SHARED
-race:_PyObject_GC_TRACK
 # https://gist.github.com/mpage/0a24eb2dd458441ededb498e9b0e5de8
 race:_PyParkingLot_Park
 race:_PyType_HasFeature