gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF #134377
Conversation
Objects/obmalloc.c
Outdated
| _PyObject_XSetRefDelayed(PyObject **ptr, PyObject *value) | ||
| { | ||
| PyObject *old = *ptr; | ||
| FT_ATOMIC_STORE_PTR_RELEASE(*ptr, Py_NewRef(value)); |
There was a problem hiding this comment.
The default build implementation in pycore_pymem.h doesn't include the Py_NewRef().
My preference is to keep the Py_NewRef(value) in the callers, i.e.:
_PyObject_XSetRefDelayed(dictptr, Py_NewRef(value));to match the semantics of Py_XSETREF.
|
Interesting... CI passed in my local but not in the CI.. :( |
|
Ah okay.. |
|
Why do we need Also, what is "delayed"? Either choose a clearer name, or add an explanatory comment in the header file. |
IIUC, the critical section is for the root object to prevent other threads from mutating the child object, and a delayed reference to prevent use-after-free from other threads that reference the child object. |
|
Gentle ping @kumaraditya303 @vstinner |
Objects/genobject.c
Outdated
| @@ -718,15 +720,19 @@ gen_set_name(PyObject *self, PyObject *value, void *Py_UNUSED(ignored)) | |||
| "__name__ must be set to a string object"); | |||
| return -1; | |||
| } | |||
| Py_XSETREF(op->gi_name, Py_NewRef(value)); | |||
| Py_BEGIN_CRITICAL_SECTION(self); | |||
| // To prevent use-after-free from other threads that reference the gi_name. | |||
There was a problem hiding this comment.
Maybe add gh-133931: prefix to these comments.
Include/internal/pycore_pymem.h
Outdated
| #ifdef Py_GIL_DISABLED | ||
| // Same as `Py_XSETREF` but in free-threading, it stores the object atomically | ||
| // and queues the old object to be decrefed at a safe point using QSBR. | ||
| PyAPI_FUNC(void) _PyObject_XSetRefDelayed(PyObject **p_obj, PyObject *obj); |
There was a problem hiding this comment.
Why not adding this function to pycore_object.h header instead?
There was a problem hiding this comment.
Because _PyObject_XDecRefDelayed is also declared here?
There was a problem hiding this comment.
I think it's fine to place _PyObject_XDecRefDelayed and _PyObject_XSetRefDelayed in pycore_object.h instead if you prefer. (I agree that they should probably be in the same header as each other).
Objects/typeobject.c
Outdated
| @@ -3967,7 +3967,8 @@ _PyObject_SetDict(PyObject *obj, PyObject *value) | |||
| return -1; | |||
| } | |||
| Py_BEGIN_CRITICAL_SECTION(obj); | |||
| // To prevent use-after-free from other threads that reference the __dict__ | |||
| // gh-133931: To prevent use-after-free from other threads that reference | |||
There was a problem hiding this comment.
Is it correct issue reference for __dict__ too?
Include/internal/pycore_pymem.h
Outdated
| #ifdef Py_GIL_DISABLED | ||
| // Same as `Py_XSETREF` but in free-threading, it stores the object atomically | ||
| // and queues the old object to be decrefed at a safe point using QSBR. | ||
| PyAPI_FUNC(void) _PyObject_XSetRefDelayed(PyObject **p_obj, PyObject *obj); |
There was a problem hiding this comment.
I think it's fine to place _PyObject_XDecRefDelayed and _PyObject_XSetRefDelayed in pycore_object.h instead if you prefer. (I agree that they should probably be in the same header as each other).
gen_set_nameandgen_set_qualnamethread-safe in free-threaded builds #133931