Race in stgdict PyCStructUnionType_update_stginfo under free-threading

[vfdev-5]

Bug report

Bug description:

I built cpython (3.13 branch) with free-threading and TSAN. The following python code reports TSAN warnings:

import ctypes
import concurrent.futures
import threading


def make_nd_memref_descriptor(rank, dtype):
    class MemRefDescriptor(ctypes.Structure):
        _fields_ = [
            ("aligned", ctypes.POINTER(dtype)),
        ]

    return MemRefDescriptor


class F32(ctypes.Structure):
    _fields_ = [("f32", ctypes.c_float)]


def test_ctypes():
    arg1_ndim = 1
    ctp = F32
    ctypes.pointer(
        ctypes.pointer(make_nd_memref_descriptor(arg1_ndim, ctp)())
    )


if __name__ == "__main__":
    num_workers = 20

    barrier = threading.Barrier(num_workers)

    def closure():
        barrier.wait()
        test_ctypes()

    with concurrent.futures.ThreadPoolExecutor(max_workers=num_workers) as executor:
        futures = []
        for i in range(num_workers):
            futures.append(executor.submit(closure))
        assert len(list(f.result() for f in futures)) == num_workers

TSAN report extract:

WARNING: ThreadSanitizer: data race (pid=264065)
  Read of size 4 at 0x7fffb80a0430 by thread T17:
    #0 PyCStructUnionType_update_stginfo /project/cpython/./Modules/_ctypes/stgdict.c:438:19 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0x20976) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
    #1 PyCStructType_setattro /project/cpython/./Modules/_ctypes/_ctypes.c:1096:16 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0xc809) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
    #2 PyObject_SetAttr /project/cpython/Objects/object.c:1415:15 (python3.13t+0x294fba) (BuildId: 65e52348ca9319985f9f5e265fd43aab2c867d59)
    #3 StructUnionType_init /project/cpython/./Modules/_ctypes/_ctypes.c:685:13 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0xe222) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
    #4 PyCStructType_init /project/cpython/./Modules/_ctypes/_ctypes.c:715:12 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0xc84a) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)

Full traceback

Related to #127945

CPython versions tested on:

3.13

Operating systems tested on:

Linux

[tom-pytel]

PyCStructUnionType_update_stginfo() definitely needs a lock:

import ctypes
import threading

if __name__ == "__main__":
    num_workers = 100

    def closure(barrier, cls, i):
        fields = [(f'i{i}', ctypes.c_int * i)]

        barrier.wait()

        try:
            cls._fields_ = fields
        except AttributeError as exc:
            pass

    while True:
        class cls(ctypes.Structure):
            pass

        barrier = threading.Barrier(num_workers)
        threads = []

        for i in range(num_workers):
            thread = threading.Thread(target=closure, args=(barrier, cls, i))

            threads.append(thread)
            thread.start()

        for thread in threads:
            thread.join()

        print(cls._fields_)

Results:

$ ./python ../../free/check2.py 
[('i24', <class '__main__.c_int_Array_24'>)]
[('i98', <class '__main__.c_int_Array_98'>)]
[('i97', <class '__main__.c_int_Array_97'>)]
[('i97', <class '__main__.c_int_Array_97'>)]
[('i98', <class '__main__.c_int_Array_98'>)]
mimalloc: error: double free detected of block 0x20000091050 with size 48
Segmentation fault (core dumped)

$ ./python ../../free/check2.py 
[('i99', <class '__main__.c_int_Array_99'>)]
[('i78', <class '__main__.c_int_Array_78'>)]
[('i99', <class '__main__.c_int_Array_99'>)]
Debug memory block at address p=0x20000090b50: API '�'
    16102724419182469211 bytes originally requested
    The 7 pad bytes at p-7 are not all FORBIDDENBYTE (0xfd):
        at p-7: 0xdd *** OUCH
        at p-6: 0xdd *** OUCH
        at p-5: 0xdd *** OUCH
        at p-4: 0xdd *** OUCH
        at p-3: 0xdd *** OUCH
        at p-2: 0xdd *** OUCH
        at p-1: 0xdd *** OUCH
    Because memory is corrupted at the start, the count of bytes requested
       may be bogus, and checking the trailing pad bytes may segfault.
    The 8 pad bytes at tail=0xdf78608b196a2bab are Segmentation fault (core dumped)

Also in some other functions like generic_pycdata_new() because that may wind up attempting allocation as the type is being finalized with an assignment to _fields_. This also applies to #128567.

[ZeroIntensity]

I haven't fully decided what to do about stgdict's yet. I'm leaning towards just slapping big critical sections on them, but I need to look through what fields are written so we can go for locks or atomics as needed.

[tom-pytel]

I'm leaning towards just slapping big critical sections on them, but I need to look through what fields are written so we can go for locks or atomics as needed.

A mutex fixed the problem for me. Global instead of per-type because of possibility of A._fields_ = [("b", B)] in one thread simultaneously with B._fields_ = [("a", A)] in another:

$ git diff main
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index ede95bdf98..f6479df571 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -699,11 +699,14 @@ StructUnionType_init(PyObject *self, PyObject *args, PyObject *kwds, int isStruc
         }
 
         /* copy base info */
+        PyMutex_Lock(&st->mutex);
         if (PyCStgInfo_clone(info, baseinfo) < 0) {
+            PyMutex_Unlock(&st->mutex);
             return -1;
         }
         info->flags &= ~DICTFLAG_FINAL; /* clear the 'final' flag in the subclass info */
         baseinfo->flags |= DICTFLAG_FINAL; /* set the 'final' flag in the baseclass info */
+        PyMutex_Unlock(&st->mutex);
     }
     return 0;
 }
@@ -3138,7 +3141,9 @@ PyCData_FromBaseObj(ctypes_state *st,
         return NULL;
     }
 
+    PyMutex_Lock(&st->mutex);
     info->flags |= DICTFLAG_FINAL;
+    PyMutex_Unlock(&st->mutex);
     cmem = (CDataObject *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0);
     if (cmem == NULL) {
         return NULL;
@@ -3187,7 +3192,9 @@ PyCData_AtAddress(ctypes_state *st, PyObject *type, void *buf)
         return NULL;
     }
 
+    PyMutex_Lock(&st->mutex);
     info->flags |= DICTFLAG_FINAL;
+    PyMutex_Unlock(&st->mutex);
 
     pd = (CDataObject *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0);
     if (!pd) {
@@ -3401,7 +3408,9 @@ generic_pycdata_new(ctypes_state *st,
         return NULL;
     }
 
+    PyMutex_Lock(&st->mutex);
     info->flags |= DICTFLAG_FINAL;
+    PyMutex_Unlock(&st->mutex);
 
     obj = (CDataObject *)type->tp_alloc(type, 0);
     if (!obj)
diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
index 45e00a538f..82688f0084 100644
--- a/Modules/_ctypes/ctypes.h
+++ b/Modules/_ctypes/ctypes.h
@@ -75,6 +75,7 @@ typedef struct {
     PyObject *error_object_name;  // callproc.c
     PyObject *PyExc_ArgError;
     PyObject *swapped_suffix;
+    PyMutex mutex;
 } ctypes_state;

Not sure is in all needed places though or fits desired convention so I leave it there if u are messing with it.

[ZeroIntensity]

That's not safe, we need to use critical sections (Py_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTION) because PyCStgInfo_clone is prone to re-entrancy deadlocks via an object's finalizer. For info->flags, we probably want _Py_atomic_or_uint8 (I think it's a uint8?), and then add some critical section wrapper functions around the stginfo functions.