From cb6f75a32ca2649c6cc1cabb0301eb783efbd55b Mon Sep 17 00:00:00 2001 From: mpage Date: Wed, 8 May 2024 13:03:39 -0700 Subject: [PATCH] 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. --- Include/internal/pycore_gc.h | 50 ++++++++++++++++++---- Include/internal/pycore_object.h | 6 +-- Objects/object.c | 2 +- Tools/tsan/suppressions_free_threading.txt | 3 -- 4 files changed, 45 insertions(+), 16 deletions(-) diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h index 281094df786735..60582521db5bd7 100644 --- 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 f15c332a7b0811..7602248f956405 100644 --- 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 29183dcc5b4a01..d4fe14c5b3d1aa 100644 --- 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 74dbf4bb1cb688..a669bc4d1d5c30 100644 --- 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