-
-
Notifications
You must be signed in to change notification settings - Fork 35k
gh-154044: Fix data race on descriptor __qualname__ in FT build #154691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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, | ||
| 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`. |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -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() | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||||
|
|
||||
|
|
@@ -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); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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);
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 1 Py_DECREF the NULL after CAS
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 * | ||||
|
|
||||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. These mirror the builtin types from the issue's reproducer, so the test reproduces the exact reported case