Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions Lib/test/test_free_threading/test_descr.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import unittest

from test import support
from test.support import threading_helper


N = 8


@threading_helper.requires_working_threading()
class TestDescrQualnameRace(unittest.TestCase):
# gh-154044: reading __qualname__ on a shared descriptor for the first time
# concurrently raced on the lazy d_qualname cache.

def race_first_access(self, descr):
results = []

def read():
results.append(descr.__qualname__)

threading_helper.run_concurrently(read, N)
self.assertEqual(len(set(results)), 1)
self.assertIsNotNone(results[0])

def test_slot_member_descriptors(self):
count = 100 if support.check_sanitizer(thread=True) else 300
for _ in range(count):
class C:
__slots__ = ("value",)
self.race_first_access(C.__dict__["value"])

def test_builtin_descriptors(self):
kinds = {"method_descriptor", "getset_descriptor", "wrapper_descriptor"}
descrs = [
v
for tp in (str, bytes, list, dict, set, int, float, tuple,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do wee need to test all of these types?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do wee need to test all of these types?

Yes. These mirror the builtin types from the issue's reproducer, so the test reproduces the exact reported case

frozenset, bytearray)
for v in vars(tp).values()
if type(v).__name__ in kinds
]
for descr in descrs:
self.race_first_access(descr)


if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a data race on a descriptor's ``__qualname__`` cache in the
:term:`free-threaded build`.
22 changes: 19 additions & 3 deletions Objects/descrobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "pycore_modsupport.h" // _PyArg_UnpackStack()
#include "pycore_object.h" // _PyObject_GC_UNTRACK()
#include "pycore_object_deferred.h" // _PyObject_SetDeferredRefcount()
#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_PTR_ACQUIRE()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Suggested change
#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_PTR_ACQUIRE()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's implicit, the rest of the codebase includes it explicitly when using FT_ATOMIC_*, so Its better leave it

#include "pycore_pystate.h" // _PyThreadState_GET()
#include "pycore_tuple.h" // _PyTuple_ITEMS()

Expand Down Expand Up @@ -621,9 +622,24 @@ static PyObject *
descr_get_qualname(PyObject *self, void *Py_UNUSED(ignored))
{
PyDescrObject *descr = (PyDescrObject *)self;
if (descr->d_qualname == NULL)
descr->d_qualname = calculate_qualname(descr);
return Py_XNewRef(descr->d_qualname);

PyObject *qualname = FT_ATOMIC_LOAD_PTR_ACQUIRE(descr->d_qualname);
if (qualname != NULL) {
return Py_NewRef(qualname);
}

Py_BEGIN_CRITICAL_SECTION(self);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can get away without using a critical section. Something like this:

static PyObject *
descr_get_qualname(PyObject *self, void *Py_UNUSED(ignored))
{
    PyDescrObject *descr = (PyDescrObject *)self;
    PyObject *qualname = FT_ATOMIC_LOAD_PTR_ACQUIRE(descr->d_qualname);
    if (qualname == NULL) {
        qualname = calculate_qualname(descr);
#ifdef Py_GIL_DISABLED
        PyObject *expected = NULL;
        if (!_Py_atomic_compare_exchange_ptr(&descr->d_qualname, &expected, qualname)) {
            Py_DECREF(qualname);
            qualname = expected;
        }
#else
        descr->d_qualname = qualname;
#endif
    }
    return Py_NewRef(qualname);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAS is a valid solution, but id prefer CS to avoid #ifdef and keep a single compute.

But your snippet contains UB, because if calculate_qualname fails it will return NULL and code will try to:

1 Py_DECREF the NULL after CAS
2 Py_NewRef on NULL

@deadlovelll deadlovelll Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW thanks for the review! Means a lot

qualname = FT_ATOMIC_LOAD_PTR_RELAXED(descr->d_qualname);
if (qualname == NULL) {
qualname = calculate_qualname(descr);
if (qualname != NULL) {
FT_ATOMIC_STORE_PTR_RELEASE(descr->d_qualname, qualname);
}
}
Py_XINCREF(qualname);
Py_END_CRITICAL_SECTION();

return qualname;
}

static PyObject *
Expand Down
Loading