ctypes pointer writes are not thread safe

[ZeroIntensity]

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.PyCData getters and setters #132082
• gh-128182: Switch ctypes to solely critical sections #132133
• gh-128182: add critical sections to ctypes arrays getters and setters #132152

[encukou]

Struct packing suggestion: change b_needsfree to char, and move b_ptr_lock next to it.
This would need a change to the _b_needsfree_ Python attribute definition. Perhaps best left to a separate 3.14-only PR.

[encukou]

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]

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]

OK. Feel free to ping me for a progress report, especially if you don't hear from me in ~a month :)

[kumaraditya303]

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]

concurrent reading and writing to a pointer isn't an important use case

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]

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.

ctypes is used for shared memory between processes

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 PyMutex or @critical_section (or pthread_mutex_t, etc.) is not going to work across processes.

[ZeroIntensity]

And ctypes is a low level API that can already crash if it's misused.

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 PyMutex instead 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 that ptr.contents is one of the things that "look atomic."

[kumaraditya303]

Kumar's point was more that we shouldn't optimize for using ctypes objects concurrently (i.e, using PyMutex instead 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 that ptr.contents is one of the things that "look atomic."

yes, it is much easier to make functions like PyCData_GetContainer thread safe by using critical section otherwise it is easy to deadlock the object if lock is already held somewhere up the call stack.

[kumaraditya303]

@ZeroIntensity There a few more issues I found in CDataObject which require critical sections and not done in #128490:

• KeepRef: This changes the b_objects so it is not safe in free-threading and currently is called without holding the lock.
• PyCData_GetContainer Same as KeepRef.

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]

It seems to me that LOCK_PTR/UNLOCK_PTR are proving to be maintenance-heavy. I'll put up a PR to remove them and just switch everything over to critical sections.

[ZeroIntensity]

#132133. For maintenance/simplicity reasons, it might be worth entirely removing the inline critical sections and using lock_held functions for everything, but I'll do that in a separate PR if we find more races.