Switch to critical sections.

python · encukou · Jan 13, 2025 · Jan 4, 2025 · Jan 4, 2025 · Jan 4, 2025
commit 9d35e86f68fc8d4fbccb2660ea9899d875e5c8e8
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
@@ -1443,9 +1443,9 @@
 static PyObject *
 CharArray_get_raw(CDataObject *self, void *Py_UNUSED(ignored))
 {
+    PyObject *res;
     LOCK_PTR(self);
-    // XXX Is it possible that PyBytes_FromStringAndSize is re-entrant?
-    PyObject *res = PyBytes_FromStringAndSize(self->b_ptr, self->b_size);
+    res = PyBytes_FromStringAndSize(self->b_ptr, self->b_size);
     UNLOCK_PTR(self);
     return res;
 }
@@ -1454,12 +1454,13 @@
 CharArray_get_value(CDataObject *self, void *Py_UNUSED(ignored))
 {
     Py_ssize_t i;
+    PyObject *res;
     LOCK_PTR(self);
     char *ptr = self->b_ptr;
     for (i = 0; i < self->b_size; ++i)
         if (*ptr++ == '\0')
             break;
-    PyObject *res = PyBytes_FromStringAndSize(self->b_ptr, i);
+    res = PyBytes_FromStringAndSize(self->b_ptr, i);
     UNLOCK_PTR(self);
     return res;
 }
@@ -1514,12 +1515,13 @@
 WCharArray_get_value(CDataObject *self, void *Py_UNUSED(ignored))
 {
     Py_ssize_t i;
+    PyObject *res;
     wchar_t *ptr = (wchar_t *)self->b_ptr;
     LOCK_PTR(self);
     for (i = 0; i < self->b_size/(Py_ssize_t)sizeof(wchar_t); ++i)
         if (*ptr++ == (wchar_t)0)
             break;
-    PyObject *res = PyUnicode_FromWideChar((wchar_t *)self->b_ptr, i);
+    res = PyUnicode_FromWideChar((wchar_t *)self->b_ptr, i);
     UNLOCK_PTR(self);
     return res;
 }
@@ -1551,8 +1553,9 @@
         PyErr_SetString(PyExc_ValueError, "string too long");
         return -1;
     }
+    int rc;
     LOCK_PTR(self);
-    int rc = PyUnicode_AsWideChar(value, (wchar_t *)self->b_ptr, size);
+    rc = PyUnicode_AsWideChar(value, (wchar_t *)self->b_ptr, size);
     UNLOCK_PTR(self);
     return rc < 0 ? -1 : 0;
 }
@@ -3029,8 +3032,9 @@
     if (dict == NULL) {
         return NULL;
     }
+    PyObject *bytes;
     LOCK_PTR(self);
-    PyObject *bytes = PyBytes_FromStringAndSize(self->b_ptr, self->b_size);
+    bytes = PyBytes_FromStringAndSize(self->b_ptr, self->b_size);
     UNLOCK_PTR(self);
     return Py_BuildValue("O(O(NN))", st->_unpickle, Py_TYPE(myself), dict,
                          bytes);
@@ -3245,8 +3249,9 @@
     CDataObject *cdata = (CDataObject *)src;
 #endif
     if (getfunc) {
+        PyObject *res;
         LOCK_PTR(cdata);
-        PyObject *res = getfunc(adr, size);
+        res = getfunc(adr, size);
         UNLOCK_PTR(cdata);
         return res;
     }
@@ -3256,8 +3261,9 @@
         return NULL;
     }
     if (info && info->getfunc && !_ctypes_simple_instance(st, type)) {
+        PyObject *res;
         LOCK_PTR(cdata);
-        PyObject *res = info->getfunc(adr, size);
+        res = info->getfunc(adr, size);
         UNLOCK_PTR(cdata);
         return res;
     }
@@ -3276,8 +3282,9 @@
     int err;
 
     if (setfunc) {
+        PyObject *res;
         LOCK_PTR(dst);
-        PyObject *res = setfunc(ptr, value, size);
+        res = setfunc(ptr, value, size);
         UNLOCK_PTR(dst);
         return res;
     }
@@ -3287,8 +3294,9 @@
             return NULL;
         }
         if (info && info->setfunc) {
+            PyObject *res;
             LOCK_PTR(dst);
-            PyObject *res = info->setfunc(ptr, value, size);
+            res = info->setfunc(ptr, value, size);
             UNLOCK_PTR(dst);
             return res;
         }
@@ -3932,7 +3940,8 @@
 
     self->paramflags = Py_XNewRef(paramflags);
 
-    // No other threads can have this object
+    // No other threads can have this object, no need to
+    // lock it.
     *(void **)self->b_ptr = address;
     Py_INCREF(dll);
     Py_DECREF(ftuple);
@@ -4865,9 +4874,10 @@
             if (slicelen <= 0)
                 return Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
             if (step == 1) {
+                PyObject *res;
                 LOCK_PTR(self);
-                PyObject *res = PyBytes_FromStringAndSize(ptr + start,
-                                                          slicelen);
+                res = PyBytes_FromStringAndSize(ptr + start,
+                                                slicelen);
                 UNLOCK_PTR(self);
                 return res;
             }
@@ -4894,8 +4904,9 @@
             if (slicelen <= 0)
                 return Py_GetConstant(Py_CONSTANT_EMPTY_STR);
             if (step == 1) {
+                PyObject *res;
                 LOCK_PTR(self);
-                PyObject *res = PyUnicode_FromWideChar(ptr + start,
+                res = PyUnicode_FromWideChar(ptr + start,
                                                        slicelen);
                 UNLOCK_PTR(self);
                 return res;
@@ -5201,8 +5212,9 @@
     }
     assert(info); /* Cannot be NULL for CDataObject instances */
     assert(info->getfunc);
+    PyObject *res;
     LOCK_PTR(self);
-    PyObject *res = info->getfunc(self->b_ptr, self->b_size);
+    res = info->getfunc(self->b_ptr, self->b_size);
     UNLOCK_PTR(self);
     return res;
 }
@@ -5240,8 +5252,9 @@
 
 static int Simple_bool(CDataObject *self)
 {
+    int cmp;
     LOCK_PTR(self);
-    int cmp = memcmp(self->b_ptr, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", self->b_size);
+    cmp = memcmp(self->b_ptr, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", self->b_size);
     UNLOCK_PTR(self);
     return cmp;
 }
@@ -5571,9 +5584,10 @@
             if (len <= 0)
                 return Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
             if (step == 1) {
+                PyObject *res;
                 LOCK_PTR(self);
-                PyObject *res = PyBytes_FromStringAndSize(ptr + start,
-                                                          len);
+                res = PyBytes_FromStringAndSize(ptr + start,
+                                                len);
                 UNLOCK_PTR(self);
                 return res;
             }
@@ -5596,9 +5610,10 @@
             if (len <= 0)
                 return Py_GetConstant(Py_CONSTANT_EMPTY_STR);
             if (step == 1) {
+                PyObject *res;
                 LOCK_PTR(self);
-                PyObject *res = PyUnicode_FromWideChar(ptr + start,
-                                                       len);
+                res = PyUnicode_FromWideChar(ptr + start,
+                                             len);
                 UNLOCK_PTR(self);
                 return res;
             }

diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
@@ -152,9 +152,6 @@ union value {
 
 struct tagCDataObject {
     PyObject_HEAD
-#ifdef Py_GIL_DISABLED
-    PyMutex b_ptr_lock;
-#endif
     char *b_ptr;                /* pointer to memory block */
     int  b_needsfree;           /* need _we_ free the memory? */
     CDataObject *b_base;        /* pointer to base object or NULL */
@@ -184,9 +181,6 @@ typedef struct {
 typedef struct {
     /* First part identical to tagCDataObject */
     PyObject_HEAD
-#ifdef Py_GIL_DISABLED
-    PyMutex b_ptr_lock;
-#endif
     char *b_ptr;                /* pointer to memory block */
     int  b_needsfree;           /* need _we_ free the memory? */
     CDataObject *b_base;        /* pointer to base object or NULL */
@@ -550,9 +544,11 @@ PyStgInfo_Init(ctypes_state *state, PyTypeObject *type)
     return info;
 }
 
+/* See discussion in gh-128490. The plan here is to eventually use a per-object
+ * lock rather than a critical section, but that work is for later. */
 #ifdef Py_GIL_DISABLED
-#  define LOCK_PTR(self) PyMutex_Lock(&self->b_ptr_lock)
-#  define UNLOCK_PTR(self) PyMutex_Unlock(&self->b_ptr_lock)
+#  define LOCK_PTR(self) Py_BEGIN_CRITICAL_SECTION(self)
+#  define UNLOCK_PTR(self) Py_END_CRITICAL_SECTION()
 #else
 #  define LOCK_PTR(self)
 #  define UNLOCK_PTR(self)
@@ -577,8 +573,9 @@ locked_memcpy_from(void *buf, CDataObject *self, Py_ssize_t size)
 static inline void *
 locked_deref(CDataObject *self)
 {
+    void *ptr;
     LOCK_PTR(self);
-    void *ptr = *(void **)self->b_ptr;
+    ptr = *(void **)self->b_ptr;
     UNLOCK_PTR(self);
     return ptr;
 }