From 4055577221f5f52af329e87f31d81bb8fb02c504 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 4 Jun 2024 15:26:26 -0400 Subject: [PATCH] gh-119999: Fix potential race condition in `_Py_ExplicitMergeRefcount` (#120000) We need to write to `ob_ref_local` and `ob_tid` before `ob_ref_shared`. Once we mark `ob_ref_shared` as merged, some other thread may free the object because the caller also passes in `-1` as `extra` to give up its only reference. --- Objects/object.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index 2e9962f4651..b7730475ac3 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -401,24 +401,27 @@ Py_ssize_t _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra) { assert(!_Py_IsImmortal(op)); - Py_ssize_t refcnt; - Py_ssize_t new_shared; - Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared); - do { - refcnt = Py_ARITHMETIC_RIGHT_SHIFT(Py_ssize_t, shared, _Py_REF_SHARED_SHIFT); - refcnt += (Py_ssize_t)op->ob_ref_local; - refcnt += extra; - - new_shared = _Py_REF_SHARED(refcnt, _Py_REF_MERGED); - } while (!_Py_atomic_compare_exchange_ssize(&op->ob_ref_shared, - &shared, new_shared)); #ifdef Py_REF_DEBUG _Py_AddRefTotal(_PyThreadState_GET(), extra); #endif + // gh-119999: Write to ob_ref_local and ob_tid before merging the refcount. + Py_ssize_t local = (Py_ssize_t)op->ob_ref_local; _Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0); _Py_atomic_store_uintptr_relaxed(&op->ob_tid, 0); + + Py_ssize_t refcnt; + Py_ssize_t new_shared; + Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared); + do { + refcnt = Py_ARITHMETIC_RIGHT_SHIFT(Py_ssize_t, shared, _Py_REF_SHARED_SHIFT); + refcnt += local; + refcnt += extra; + + new_shared = _Py_REF_SHARED(refcnt, _Py_REF_MERGED); + } while (!_Py_atomic_compare_exchange_ssize(&op->ob_ref_shared, + &shared, new_shared)); return refcnt; } #endif /* Py_GIL_DISABLED */