Repository navigation
ctypes pointer writes are not thread safe #128182
Description
Bug report
Bug description:
Part of #127945.
ctypes C data objects have an internal pointer for what they're looking at (b_ptr). This field itself is generally fine to read non-atomically, because ctypes objects don't tend to overwrite the pointer that they're pointing to, but reading and writing the pointer's contents (such as via memcpy) isn't thread safe. This can be seen primarily with arrays:
from threading import Thread
import ctypes
buffer = (ctypes.c_char_p * 10)()
def main():
for i in range(100):
buffer.value = b"hello"
buffer[1] = b"j"
threads = [Thread(target=main) for _ in range(100)]
for thread in threads:
thread.start()There's really only two options here, because the lockless _Py_atomic APIs can't be used for allocations of arbitrary sizes: add a critical section around all functions touching b_ptr, or just add a PyMutex around it.
I think that a mutex is the right way to go here:
- There's no chance of re-entrancy with
memcpy. - AC doesn't work for some type slots (e.g.
__call__), so they can't get wrapped with@critical_section. We'd need ugly wrapper functions.
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Linked PRs
- gh-128182: Add per-object memory access synchronization to
ctypes#128490 - gh-128182: add critical section to _ctypes.Simple getters and setters #132081
- gh-128182: add critical section to
_ctypes.PyCDatagetters and setters #132082 - gh-128182: Switch
ctypesto solely critical sections #132133 - gh-128182: add critical sections to ctypes arrays getters and setters #132152
Activity
encukou commentedon Jan 7, 2025
Struct packing suggestion: change
b_needsfreetochar, and moveb_ptr_locknext to it.This would need a change to the
_b_needsfree_Python attribute definition. Perhaps best left to a separate 3.14-only PR.gh-128182: Add per-object memory access synchronization to `ctypes` (G…
encukou commentedon Jan 13, 2025
The merged PR uses critical sections.
Once we have the “owning” object available in every function that uses the pointers, we should switch to dedicated locks, as in the first iterations of #128490.
I'm slowly working toward that.
ZeroIntensity commentedon Jan 13, 2025
Thanks for merging! I'm going to close this as completed, because otherwise it will just sit here and collect dust for a while until we go for mutexes. I'll open a seperate issue once we're ready to do that.
encukou commentedon Jan 14, 2025
OK. Feel free to ping me for a progress report, especially if you don't hear from me in ~a month :)
kumaraditya303 commentedon Apr 2, 2025
I am not sure if it is worth the effort to change it to mutex instead of critical section. It is easy to miss re-entrancy with a mutex and concurrent reading and writing to a pointer isn't an important use case. I propose to drop moving a mutex and instead use critical sections on functions with AC
@critical_section.ZeroIntensity commentedon Apr 2, 2025
I'm inclined to argue otherwise. ctypes is broad and used for all sorts of things, so I'm not sure we should immediately rule out concurrency as a use-case. ctypes is used for shared memory between processes, right?
cc @colesbury though.
colesbury commentedon Apr 2, 2025
I don't think we should be synchronizing accesses to ctypes data structures. It's not broad enough to be safe: pointers can overlap. And ctypes is a low level API that can already crash if it's misused.
I'm not aware of people using ctypes for that. I guess you can do that if you have shared memory that's also writable between processes. But any synchronization with
PyMutexor@critical_section(orpthread_mutex_t, etc.) is not going to work across processes.ZeroIntensity commentedon Apr 2, 2025
Yeah, but I'd rather do what we can to be a little helpful. Spurious crashes caused by races with ctypes objects will definitely not be fun to debug.
Kumar's point was more that we shouldn't optimize for using ctypes objects concurrently (i.e, using
PyMutexinstead of critical sections). I think I can get behind that, but I'm not so sure about entirely dropping support for concurrent use. I'd argue thatptr.contentsis one of the things that "look atomic."kumaraditya303 commentedon Apr 4, 2025
yes, it is much easier to make functions like
PyCData_GetContainerthread safe by using critical section otherwise it is easy to deadlock the object if lock is already held somewhere up the call stack.gh-128182: add critical section to `_ctypes.PyCData` methods (#132082)
gh-128182: add critical section to `_ctypes.Simple` getters and sette…
kumaraditya303 commentedon Apr 5, 2025
@ZeroIntensity There a few more issues I found in
CDataObjectwhich require critical sections and not done in #128490:b_objectsso it is not safe in free-threading and currently is called without holding the lock.Both of these should be called while holding the critical section as currently tsan reports them with data races when ctypes tests are run in parallel.
ZeroIntensity commentedon Apr 5, 2025
It seems to me that
LOCK_PTR/UNLOCK_PTRare proving to be maintenance-heavy. I'll put up a PR to remove them and just switch everything over to critical sections.ZeroIntensity commentedon Apr 5, 2025
#132133. For maintenance/simplicity reasons, it might be worth entirely removing the inline critical sections and using
lock_heldfunctions for everything, but I'll do that in a separate PR if we find more races.gh-128182: add critical sections to ctypes arrays getters and setters (…
gh-128182: switch `ctypes` locking to critical sections (#132133)