gh-145272: Fix data race when accessing func_code in PyFunctionObject#145534
gh-145272: Fix data race when accessing func_code in PyFunctionObject#145534Krishna-web-hub wants to merge 3 commits intopython:mainfrom
Conversation
Krishna-web-hub
force-pushed
the
New_fix_clean
branch
from
d7c99b0 to
d926271
Compare
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Is this issue specific to func_code? What about other PyFunction methods such as PyFunction_GET_GLOBALS() or PyFunction_GET_MODULE()?
|
@vstinner actually this issue was mentioned only to fixation of the func_code. But the func_code have high chance of being reassigned at runtime. But others like func_globals and func_module are set at the time of the function creation. But there are few objects like func_annotations, func_defaults they can cause the race situation but i think applying the atomic pointer fix can slow down the cpython. Based on this i want to know your opinion on what i should be doing next ? |
| PyCodeObject *code = _Py_atomic_load_ptr(&op->func_code); | ||
| return Py_NewRef(code); |
There was a problem hiding this comment.
This isn't right.
code might have been deallocated by the time you do Py_NewRef
| @@ -0,0 +1 @@ | |||
| Fixing race condition in PyFunctionObject.func_code | |||
There was a problem hiding this comment.
I think we should be less precise in terms of implementation and rather mention __func__.__code__ directly.
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
But others like func_globals and func_module are set at the time of the function creation. But there are few objects like func_annotations, func_defaults they can cause the race situation but i think applying the atomic pointer fix can slow down the cpython
Regular build shouldn't be affected if add a Py_GIL_DISABLED guard. But I would indeed have a macro that atomically access attributes instead.
| } | ||
|
|
||
| PyObject *func_code = PyFunction_GET_CODE(op); | ||
| PyCodeObject *func_code = (PyCodeObject *)PyFunction_GET_CODE(op); |
| return _PyFunction_CAST(func)->func_code; | ||
| PyFunctionObject *op = _PyFunction_CAST(func); | ||
| #ifdef Py_GIL_DISABLED | ||
| return (PyObject *)_Py_atomic_load_ptr(&op->func_code); |
There was a problem hiding this comment.
Aren't all attributes of function objects subject to this issue then?
| handle_func_event(PyFunction_EVENT_MODIFY_CODE, op, value); | ||
| _PyFunction_ClearVersion(op); | ||
| Py_XSETREF(op->func_code, Py_NewRef(value)); | ||
| PyCodeObject *new = (PyCodeObject *)Py_NewRef(value); |
There was a problem hiding this comment.
This should be GIL_DISABLED-guarded
There was a problem hiding this comment.
Also, don't we have a FT-helper alterantive to Py_XSETREF?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
4 participants
Fix a data race when accessing PyFunctionObject.func_code in free-threaded builds.
This patch replaces the direct read with
_Py_atomic_load_ptr()and updates the setter to use_Py_atomic_exchange_ptr()so that updates tofunc_codeare performed atomically.The race was reproduced using
ThreadSanitizerand verified to be fixed after this change.func_set_code#145272