gh-132070: Fix PyObject_Realloc thread-safety in free threaded Python#143441
gh-132070: Fix PyObject_Realloc thread-safety in free threaded Python#143441colesbury merged 3 commits intopython:mainfrom
Conversation
…Python The PyObject header reference count fields must be initialized using atomic operations because they may be concurrently read by another thread (e.g., from _Py_TryIncref).
|
Reimplementing the |
| for (size_t i = 0; i != offset; i += sizeof(void*)) { | ||
| void *word; | ||
| memcpy(&word, (char*)ptr + i, sizeof(void*)); | ||
| _Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), word); |
There was a problem hiding this comment.
Is there a reason for the intermediate word? And why the memcpy instead of just a cast + deref + assignment? Can't this just be:
| _Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), word); | |
| _Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), (void *)((char *)ptr + 1)); |
(If it can't be, it probably deserves a comment.)
There was a problem hiding this comment.
My thinking was that the memcpy avoids accessing data the pointers of an incorrect size or strict aliasing issues. For example, we read ob_flags, ob_mutex, ob_gc_bits, and ob_ref_local together as a 64-bit load (on 64-bit systems).
I guess there's still the same issue when we write the data with _Py_atomic_store_ptr_relaxed, so maybe it doesn't matter.
From what I can tell, UBSan and ASan don't check for this kind of thing.
Co-authored-by: T. Wouters <[email protected]>
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 3387cd2 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143441%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
…Python (pythongh-143441) The PyObject header reference count fields must be initialized using atomic operations because they may be concurrently read by another thread (e.g., from `_Py_TryIncref`).
3 participants
The PyObject header reference count fields must be initialized using atomic operations because they may be concurrently read by another thread (e.g., from
_Py_TryIncref).