gh-129069: Fix listobject.c data races due to memmove #142957
Conversation
The use of memmove and _Py_memory_repeat were not thread-safe in the free threading build in some cases. In theory, memmove and _Py_memory_repeat can copy byte-by-byte instead of pointer-by-pointer, so concurrent readers could see uninitialized data or tearing. Additionally, we should be using "release" (or stronger) ordering to be compliant with the C11 memory model when copying objects within a list.
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 97e8d13 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F142957%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
Objects/listobject.c
Outdated
| PyObject **src = items; | ||
| PyObject **dst = items + input_size; | ||
| Py_ssize_t remaining = output_size - input_size; | ||
| while (remaining > 0) { | ||
| _Py_atomic_store_ptr_release(dst++, *src++); | ||
| remaining--; | ||
| } |
There was a problem hiding this comment.
Why can't this just call list_atomic_memmove?
There was a problem hiding this comment.
Yeah, that will simplify this a bit and avoid the special case here (since list_atomic_memmove has it internally).
Objects/listobject.c
Outdated
| goto Error; | ||
| } | ||
| Py_ssize_t tail = Py_SIZE(a) - ihigh; | ||
| list_atomic_memmove(a, &item[ihigh+d], &item[ihigh], tail); |
There was a problem hiding this comment.
This is so much nicer, I wish we'd refactored this the first time round :P
Objects/listobject.c
Outdated
| // Pointer-by-pointer memmove for PyObject** arrays that is safe | ||
| // for shared lists in Py_GIL_DISABLED builds. | ||
| static void | ||
| list_atomic_memmove(PyListObject *a, PyObject **dest, PyObject **src, Py_ssize_t n) |
There was a problem hiding this comment.
Not a fan of the name (it's not the memmove as a whole that's atomic, just the individual writes) but I can't think of a better name and I think the comment explains it well enough.
There was a problem hiding this comment.
Yeah, it's a bit misleading. I can name it ptr_wise_atomic_memmove (as in Byte-wise atomic memcpy)
3 participants
The use of memmove and _Py_memory_repeat were not thread-safe in the free threading build in some cases. In theory, memmove and _Py_memory_repeat can copy byte-by-byte instead of pointer-by-pointer, so concurrent readers could see uninitialized data or tearing.
Additionally, we should be using "release" (or stronger) ordering to be compliant with the C11 memory model when copying objects within a list.
I've also updated
list_resizeso it definitively doesn't fail when shrinking a list. This simplifies some of the call sites.