gh-127945: fix thread safety of creating instances of ctypes structures

Include/internal/pycore_pyatomic_ft_wrappers.h

@@ -20,6 +20,8 @@ extern "C" {
 #endif
 
 #ifdef Py_GIL_DISABLED
+#define FT_ATOMIC_LOAD_INT(value) _Py_atomic_load_int(&value)
+#define FT_ATOMIC_STORE_INT(value, new_value) _Py_atomic_store_int(&value, new_value)
 #define FT_ATOMIC_LOAD_PTR(value) _Py_atomic_load_ptr(&value)
 #define FT_ATOMIC_STORE_PTR(value, new_value) _Py_atomic_store_ptr(&value, new_value)
 #define FT_ATOMIC_LOAD_SSIZE(value) _Py_atomic_load_ssize(&value)
@@ -111,6 +113,8 @@ extern "C" {
     _Py_atomic_load_ullong_relaxed(&value)
 
 #else
+#define FT_ATOMIC_LOAD_INT(value) value
+#define FT_ATOMIC_STORE_INT(value, new_value) value = new_value
 #define FT_ATOMIC_LOAD_PTR(value) value
 #define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value
 #define FT_ATOMIC_LOAD_SSIZE(value) value

Lib/test/test_ctypes/test_free_threading.py

@@ -0,0 +1,89 @@
+from test.support import threading_helper
+import unittest
+import ctypes
+import threading
+
+threading_helper.requires_working_threading(module=True)
+
+
+class FreeThreadingTests(unittest.TestCase):
+
+    def test_PyCFuncPtr_new_race(self):
+        # See https://github.com/python/cpython/issues/128567
+
+        def raw_func(x):
+            pass
+
+        def race():
+            barrier.wait()
+            def lookup():
+                prototype = ctypes.CFUNCTYPE(None, ctypes.c_void_p)
+                return prototype(raw_func)
+
+            ctypes_args = ()
+            func = lookup()
+            packed_args = (ctypes.c_void_p * len(ctypes_args))()
+            for argNum in range(len(ctypes_args)):
+                packed_args[argNum] = ctypes.cast(ctypes_args[argNum], ctypes.c_void_p)
+            func(packed_args)
+
+        num_workers = 20
+        barrier = threading.Barrier(num_workers)
+
+        threads = []
+        for _ in range(num_workers):
+            t = threading.Thread(target=race)
+            threads.append(t)
+
+        with threading_helper.start_threads(threads):
+            pass
+
+    def test_Structure_creation_race(self):
+        class POINT(ctypes.Structure):
+            _fields_ = [("x", ctypes.c_int), ("y", ctypes.c_int)]
+
+        def race():
+            barrier.wait()
+            return POINT(1, 2)
+
+        num_workers = 20
+        barrier = threading.Barrier(num_workers)
+
+        threads = []
+        for _ in range(num_workers):
+            t = threading.Thread(target=race)
+            threads.append(t)
+
+        with threading_helper.start_threads(threads):
+            pass
+
+    def test_stginfo_update_race(self):
+        # See https://github.com/python/cpython/issues/128570
+        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 race():
+            barrier.wait()
+            ctp = F32
+            ctypes.pointer(
+                ctypes.pointer(make_nd_memref_descriptor(1, ctp)())
+            )
+
+        num_workers = 20
+        barrier = threading.Barrier(num_workers)
+
+        threads = []
+        for _ in range(num_workers):
+            t = threading.Thread(target=race)
+            threads.append(t)
+
+        with threading_helper.start_threads(threads):
+            pass

Modules/_ctypes/_ctypes.c

@@ -109,6 +109,7 @@ bytes(cdata)
 #include "pycore_call.h"          // _PyObject_CallNoArgs()
 #include "pycore_ceval.h"         // _Py_EnterRecursiveCall()
 #include "pycore_unicodeobject.h" // _PyUnicode_EqualToASCIIString()
+#include "pycore_pyatomic_ft_wrappers.h"
 #ifdef MS_WIN32
 #  include "pycore_modsupport.h"  // _PyArg_NoKeywords()
 #endif
@@ -710,13 +711,18 @@ StructUnionType_init(PyObject *self, PyObject *args, PyObject *kwds, int isStruc
         if (baseinfo == NULL) {
             return 0;
         }
-
+        int ret = 0;
+        STGINFO_LOCK(baseinfo);
         /* copy base info */
-        if (PyCStgInfo_clone(info, baseinfo) < 0) {
-            return -1;
+        ret = PyCStgInfo_clone(info, baseinfo);
+        if (ret >= 0) {
+            // clear the 'final' bit in the subclass info
+            // safe to modify without atomics as it is not exposed to other threads
+            info->dict_final = 0;
+            stginfo_set_dict_final_lock_held(baseinfo); /* set the 'final' flag in the baseclass info */
         }
-        info->flags &= ~DICTFLAG_FINAL; /* clear the 'final' flag in the subclass info */
-        baseinfo->flags |= DICTFLAG_FINAL; /* set the 'final' flag in the baseclass info */
+        STGINFO_UNLOCK();
+        return ret;
     }
     return 0;
 }
@@ -3122,6 +3128,7 @@ PyCData_MallocBuffer(CDataObject *obj, StgInfo *info)
      * access.
      */
    assert (Py_REFCNT(obj) == 1);
+   assert(stginfo_get_dict_final(info) == 1);
 
     if ((size_t)info->size <= sizeof(obj->b_value)) {
         /* No need to call malloc, can use the default buffer */
@@ -3167,7 +3174,7 @@ PyCData_FromBaseObj(ctypes_state *st,
         return NULL;
     }
 
-    info->flags |= DICTFLAG_FINAL;
+    stginfo_set_dict_final(info);
     cmem = (CDataObject *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0);
     if (cmem == NULL) {
         return NULL;
@@ -3216,7 +3223,7 @@ PyCData_AtAddress(ctypes_state *st, PyObject *type, void *buf)
         return NULL;
     }
 
-    info->flags |= DICTFLAG_FINAL;
+    stginfo_set_dict_final(info);
 
     pd = (CDataObject *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0);
     if (!pd) {
@@ -3451,7 +3458,7 @@ generic_pycdata_new(ctypes_state *st,
         return NULL;
     }
 
-    info->flags |= DICTFLAG_FINAL;
+    stginfo_set_dict_final(info);
 
     obj = (CDataObject *)type->tp_alloc(type, 0);
     if (!obj)

Modules/_ctypes/ctypes.h

@@ -6,6 +6,8 @@
 
 #include "pycore_moduleobject.h"  // _PyModule_GetState()
 #include "pycore_typeobject.h"    // _PyType_GetModuleState()
+#include "pycore_critical_section.h"
+#include "pycore_pyatomic_ft_wrappers.h"
 
 // Do we support C99 complex types in ffi?
 // For Apple's libffi, this must be determined at runtime (see gh-128156).
@@ -373,6 +375,9 @@ typedef struct CFieldObject {
 *****************************************************************/
 
 typedef struct {
+#ifdef Py_GIL_DISABLED
+    PyMutex mutex;              /* critical section mutex */
+#endif
     int initialized;
     Py_ssize_t size;            /* number of bytes */
     Py_ssize_t align;           /* alignment requirements */
@@ -390,6 +395,7 @@ typedef struct {
     PyObject *checker;
     PyObject *module;
     int flags;                  /* calling convention and such */
+    int dict_final;
 
     /* pep3118 fields, pointers need PyMem_Free */
     char *format;
@@ -399,6 +405,54 @@ typedef struct {
 /*      Py_ssize_t *suboffsets; */ /* unused in ctypes */
 } StgInfo;
 
+/*
+    In free-threading, concurrent mutations to StgInfo is not thread safe.
+    Therefore to make it thread safe, when modifying StgInfo, `STGINFO_LOCK` and
+    `STGINFO_UNLOCK` macros are used to acquire critical section of the StgInfo.
+    The critical section is write only and is acquired when modifying the
+    StgInfo fields and while setting the `dict_final` bit. Once the `dict_final`
+    is set, StgInfo is treated as read only and no further modifications are
+    allowed. This allows to avoid acquiring the critical section for most
+    read operations when `dict_final` is set (general case).
+
+    It is important to set all the fields before setting the `dict_final` bit
+    in functions like `PyCStructUnionType_update_stginfo` because the store of
+    `dict_final` uses sequential consistency memory ordering. This ensures that
+    all the other fields are visible to other threads before the `dict_final` bit
+    is set thus allowing for lock free reads when `dict_final` is set.
+
+*/
+
+#define STGINFO_LOCK(stginfo)   Py_BEGIN_CRITICAL_SECTION_MUT(&(stginfo)->mutex)
+#define STGINFO_UNLOCK()        Py_END_CRITICAL_SECTION()
+
+static inline int
+stginfo_get_dict_final(StgInfo *info)
+{
+    return FT_ATOMIC_LOAD_INT(info->dict_final);
+}
+
+static inline void
+stginfo_set_dict_final_lock_held(StgInfo *info)
+{
+    _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(&info->mutex);
+    FT_ATOMIC_STORE_INT(info->dict_final, 1);
+}
+
+
+// Set the `dict_final` bit in StgInfo. It checks if the bit is already set
+// and in that avoids acquiring the critical section (general case).
+static inline void
+stginfo_set_dict_final(StgInfo *info)
+{
+    if (stginfo_get_dict_final(info) == 1) {
+        return;
+    }
+    STGINFO_LOCK(info);
+    stginfo_set_dict_final_lock_held(info);
+    STGINFO_UNLOCK();
+}
+
 extern int PyCStgInfo_clone(StgInfo *dst_info, StgInfo *src_info);
 extern void ctype_clear_stginfo(StgInfo *info);
 
@@ -427,8 +481,6 @@ PyObject *_ctypes_callproc(ctypes_state *st,
 #define TYPEFLAG_ISPOINTER 0x100
 #define TYPEFLAG_HASPOINTER 0x200
 
-#define DICTFLAG_FINAL 0x1000
-
 struct tagPyCArgObject {
     PyObject_HEAD
     ffi_type *pffi_type;

Modules/_ctypes/stgdict.c

@@ -34,6 +34,9 @@ PyCStgInfo_clone(StgInfo *dst_info, StgInfo *src_info)
     dst_info->ffi_type_pointer.elements = NULL;
 
     memcpy(dst_info, src_info, sizeof(StgInfo));
+#ifdef Py_GIL_DISABLED
+    dst_info->mutex = (PyMutex){0};
+#endif
 
     Py_XINCREF(dst_info->proto);
     Py_XINCREF(dst_info->argtypes);
@@ -248,23 +251,30 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct
     ctypes_state *st = get_module_state_by_def(Py_TYPE(type));
     StgInfo *stginfo;
     if (PyStgInfo_FromType(st, type, &stginfo) < 0) {
-        goto error;
+        return -1;
     }
     if (!stginfo) {
         PyErr_SetString(PyExc_TypeError,
                         "ctypes state is not initialized");
-        goto error;
+        return -1;
     }
     PyObject *base = (PyObject *)((PyTypeObject *)type)->tp_base;
     StgInfo *baseinfo;
     if (PyStgInfo_FromType(st, base, &baseinfo) < 0) {
-        goto error;
+        return -1;
     }
 
     /* If this structure/union is already marked final we cannot assign
        _fields_ anymore. */
 
-    if (stginfo->flags & DICTFLAG_FINAL) {/* is final ? */
+    if (stginfo_get_dict_final(stginfo) == 1) {/* is final ? */
+        PyErr_SetString(PyExc_AttributeError,
+                        "_fields_ is final");
+        return -1;
+    }
+    STGINFO_LOCK(stginfo);
+    // check again after locking
+    if (stginfo_get_dict_final(stginfo) == 1) {/* is final ? */
         PyErr_SetString(PyExc_AttributeError,
                         "_fields_ is final");
         goto error;
@@ -422,12 +432,13 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct
             goto error;
         }
         assert(info);
-
+        STGINFO_LOCK(info);
         stginfo->ffi_type_pointer.elements[ffi_ofs + i] = &info->ffi_type_pointer;
         if (info->flags & (TYPEFLAG_ISPOINTER | TYPEFLAG_HASPOINTER))
             stginfo->flags |= TYPEFLAG_HASPOINTER;
-        info->flags |= DICTFLAG_FINAL; /* mark field type final */
 
+        stginfo_set_dict_final_lock_held(info); /* mark field type final */
+        STGINFO_UNLOCK();
         if (-1 == PyObject_SetAttr(type, prop->name, prop_obj)) {
             goto error;
         }
@@ -461,15 +472,15 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct
 
     /* We did check that this flag was NOT set above, it must not
        have been set until now. */
-    if (stginfo->flags & DICTFLAG_FINAL) {
+    if (stginfo_get_dict_final(stginfo) == 1) {
         PyErr_SetString(PyExc_AttributeError,
                         "Structure or union cannot contain itself");
         goto error;
     }
-    stginfo->flags |= DICTFLAG_FINAL;
+    stginfo_set_dict_final_lock_held(stginfo);
 
     retval = MakeAnonFields(type);
-error:
+error:;
     Py_XDECREF(layout_func);
     Py_XDECREF(kwnames);
     Py_XDECREF(align_obj);
@@ -478,6 +489,7 @@ PyCStructUnionType_update_stginfo(PyObject *type, PyObject *fields, int isStruct
     Py_XDECREF(layout_fields);
     Py_XDECREF(layout);
     Py_XDECREF(format_spec_obj);
+    STGINFO_UNLOCK();
     return retval;
 }