Skip to content

Commit

Permalink
pythongh-117657: Fix data races when writing / reading ob_gc_bits (p…
Browse files Browse the repository at this point in the history
…ython#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.
  • Loading branch information
mpage authored May 8, 2024
1 parent 8d84120 commit cb6f75a
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 16 deletions.
50 changes: 41 additions & 9 deletions Include/internal/pycore_gc.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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))

Expand All @@ -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))
Expand Down Expand Up @@ -178,23 +210,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;
Expand Down
6 changes: 3 additions & 3 deletions Include/internal/pycore_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion Objects/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -2427,14 +2427,14 @@ _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
// counting for now.
_Py_SetImmortal(op);
return;
}
op->ob_gc_bits |= _PyGC_BITS_DEFERRED;
op->ob_ref_local += 1;
op->ob_ref_shared = _Py_REF_QUEUED;
#endif
Expand Down
3 changes: 0 additions & 3 deletions Tools/tsan/suppressions_free_threading.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit cb6f75a

Please sign in to comment.