Race condition in `_Py_ExplicitMergeRefcount`

[colesbury]

Bug report

There is a subtle possible race condition in _Py_ExplicitMergeRefcount: we set ob_ref_local and ob_tid to zero after writing the merged refcount to ob_ref_shared.

That's not safe, because another thread might possibly deallocate the object after we merged refcount. For example:

• Assume that the merged refcount is 1
• Some other thread calls Py_DECREF() and immediately frees the object
• We write zero to ob_ref_local and ob_tid -- BUG!

cpython/Objects/object.c

Lines 413 to 422 in 41c1cef

	} while (!_Py_atomic_compare_exchange_ssize(&op->ob_ref_shared,
	&shared, new_shared));
	#ifdef Py_REF_DEBUG
	_Py_AddRefTotal(_PyThreadState_GET(), extra);
	#endif
	_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0);
	_Py_atomic_store_uintptr_relaxed(&op->ob_tid, 0);
	return refcnt;

Linked PRs

• gh-119999: Fix potential race condition in _Py_ExplicitMergeRefcount #120000
• [3.13] gh-119999: Fix potential race condition in _Py_ExplicitMergeRefcount (GH-120000) #120073

[colesbury]

The other aspect of this bug is that the current callers all pass -1 as extra. In other words, they're both effectively giving up their only reference and merging the refcount simultaneously:

cpython/Python/brc.c

Lines 102 to 113 in 41c1cef

	static void
	merge_queued_objects(_PyObjectStack *to_merge)
	{
	PyObject *ob;
	while ((ob = _PyObjectStack_Pop(to_merge)) != NULL) {
	// Subtract one when merging because the queue had a reference.
	Py_ssize_t refcount = _Py_ExplicitMergeRefcount(ob, -1);
	if (refcount == 0) {
	_Py_Dealloc(ob);
	}
	}
	}

An alternative would be to get rid of extra and have callees instead perform. In that case, we wouldn't be sensitive to the ordering of writing to the refcount fields.

_Py_ExplicitMergeRefcount(ob);
Py_DECREF(ob);  // the queue had a reference

The current implementation which combines the operation is more efficient.