From 81f2e2b1dba93336a6be3425e41dbacb2a0352bd Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 5 Apr 2025 12:02:24 -0400 Subject: [PATCH 01/10] Remove LOCK_PTR()/UNLOCK_PTR() macros and fix data races. --- Modules/_ctypes/_ctypes.c | 170 +++++++++++++++++++++----------------- Modules/_ctypes/ctypes.h | 34 +++----- 2 files changed, 108 insertions(+), 96 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index f3b15226f028e5..676dcfd9fd0743 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -1462,9 +1462,9 @@ CharArray_get_raw(PyObject *op, void *Py_UNUSED(ignored)) { PyObject *res; CDataObject *self = _CDataObject_CAST(op); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = PyBytes_FromStringAndSize(self->b_ptr, self->b_size); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -1474,13 +1474,13 @@ CharArray_get_value(PyObject *op, void *Py_UNUSED(ignored)) Py_ssize_t i; PyObject *res; CDataObject *self = _CDataObject_CAST(op); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); char *ptr = self->b_ptr; for (i = 0; i < self->b_size; ++i) if (*ptr++ == '\0') break; res = PyBytes_FromStringAndSize(self->b_ptr, i); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -1513,11 +1513,11 @@ CharArray_set_value(PyObject *op, PyObject *value, void *Py_UNUSED(ignored)) } ptr = PyBytes_AS_STRING(value); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); memcpy(self->b_ptr, ptr, size); if (size < self->b_size) self->b_ptr[size] = '\0'; - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); Py_DECREF(value); return 0; @@ -1536,12 +1536,12 @@ WCharArray_get_value(PyObject *op, void *Py_UNUSED(ignored)) PyObject *res; CDataObject *self = _CDataObject_CAST(op); wchar_t *ptr = (wchar_t *)self->b_ptr; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); for (i = 0; i < self->b_size/(Py_ssize_t)sizeof(wchar_t); ++i) if (*ptr++ == (wchar_t)0) break; res = PyUnicode_FromWideChar((wchar_t *)self->b_ptr, i); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -1575,9 +1575,9 @@ WCharArray_set_value(PyObject *op, PyObject *value, void *Py_UNUSED(ignored)) return -1; } Py_ssize_t rc; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); rc = PyUnicode_AsWideChar(value, (wchar_t *)self->b_ptr, size); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return rc < 0 ? -1 : 0; } @@ -2777,7 +2777,7 @@ static PyType_Spec pycfuncptr_type_spec = { */ static CDataObject * -PyCData_GetContainer(CDataObject *self) +PyCData_GetContainer_lock_held(CDataObject *self) { while (self->b_base) self = self->b_base; @@ -2793,6 +2793,16 @@ PyCData_GetContainer(CDataObject *self) return self; } +static CDataObject * +PyCData_GetContainer_lock_held(CDataObject *self) +{ + CDataObject *res; + Py_BEGIN_CRITICAL_SECTION(self); + res = PyCData_GetContainer_lock_held(self); + Py_END_CRITICAL_SECTION(); + return res; +} + static PyObject * GetKeepedObjects(CDataObject *target) { @@ -2826,26 +2836,8 @@ unique_key(CDataObject *target, Py_ssize_t index) return PyUnicode_FromStringAndSize(string, cp-string); } -/* - * Keep a reference to 'keep' in the 'target', at index 'index'. - * - * If 'keep' is None, do nothing. - * - * Otherwise create a dictionary (if it does not yet exist) id the root - * objects 'b_objects' item, which will store the 'keep' object under a unique - * key. - * - * The unique_key helper travels the target's b_base pointer down to the root, - * building a string containing hex-formatted indexes found during traversal, - * separated by colons. - * - * The index tuple is used as a key into the root object's b_objects dict. - * - * Note: This function steals a refcount of the third argument, even if it - * fails! - */ static int -KeepRef(CDataObject *target, Py_ssize_t index, PyObject *keep) +KeepRef_lock_held(CDataObject *target, Py_ssize_t index, PyObject *keep) { int result; CDataObject *ob; @@ -2876,6 +2868,34 @@ KeepRef(CDataObject *target, Py_ssize_t index, PyObject *keep) return result; } +/* + * Keep a reference to 'keep' in the 'target', at index 'index'. + * + * If 'keep' is None, do nothing. + * + * Otherwise create a dictionary (if it does not yet exist) id the root + * objects 'b_objects' item, which will store the 'keep' object under a unique + * key. + * + * The unique_key helper travels the target's b_base pointer down to the root, + * building a string containing hex-formatted indexes found during traversal, + * separated by colons. + * + * The index tuple is used as a key into the root object's b_objects dict. + * + * Note: This function steals a refcount of the third argument, even if it + * fails! + */ +static int +KeepRef(CDataObject *target, Py_ssize_t index, PyObject *keep) +{ + int res; + Py_BEGIN_CRITICAL_SECTION(target); + target = KeepRef_lock_held(target, index, keep); + Py_END_CRITICAL_SECTION(); + return res; +} + /******************************************************************/ /* PyCData_Type @@ -3260,9 +3280,9 @@ PyCData_get(ctypes_state *st, PyObject *type, GETFUNC getfunc, PyObject *src, CDataObject *cdata = _CDataObject_CAST(src); if (getfunc) { PyObject *res; - LOCK_PTR(cdata); + Py_BEGIN_CRITICAL_SECTION(cdata); res = getfunc(adr, size); - UNLOCK_PTR(cdata); + Py_END_CRITICAL_SECTION(); return res; } assert(type); @@ -3272,9 +3292,9 @@ PyCData_get(ctypes_state *st, PyObject *type, GETFUNC getfunc, PyObject *src, } if (info && info->getfunc && !_ctypes_simple_instance(st, type)) { PyObject *res; - LOCK_PTR(cdata); + Py_BEGIN_CRITICAL_SECTION(cdata); res = info->getfunc(adr, size); - UNLOCK_PTR(cdata); + Py_END_CRITICAL_SECTION(); return res; } return PyCData_FromBaseObj(st, type, src, index, adr); @@ -3293,9 +3313,9 @@ _PyCData_set(ctypes_state *st, if (setfunc) { PyObject *res; - LOCK_PTR(dst); + Py_BEGIN_CRITICAL_SECTION(dst); res = setfunc(ptr, value, size); - UNLOCK_PTR(dst); + Py_END_CRITICAL_SECTION(); return res; } if (!CDataObject_Check(st, value)) { @@ -3305,9 +3325,9 @@ _PyCData_set(ctypes_state *st, } if (info && info->setfunc) { PyObject *res; - LOCK_PTR(dst); + Py_BEGIN_CRITICAL_SECTION(dst); res = info->setfunc(ptr, value, size); - UNLOCK_PTR(dst); + Py_END_CRITICAL_SECTION(); return res; } /* @@ -3329,9 +3349,9 @@ _PyCData_set(ctypes_state *st, Py_DECREF(ob); return result; } else if (value == Py_None && PyCPointerTypeObject_Check(st, type)) { - LOCK_PTR(dst); + Py_BEGIN_CRITICAL_SECTION(dst); *(void **)ptr = NULL; - UNLOCK_PTR(dst); + Py_END_CRITICAL_SECTION(); Py_RETURN_NONE; } else { PyErr_Format(PyExc_TypeError, @@ -3347,13 +3367,15 @@ _PyCData_set(ctypes_state *st, if (err == -1) return NULL; if (err) { - locked_memcpy_from(ptr, src, size); + Py_BEGIN_CRITICAL_SECTION(src); + memcpy(ptr, src->b_ptr, size); if (PyCPointerTypeObject_Check(st, type)) { /* XXX */ } value = GetKeepedObjects(src); + Py_END_CRITICAL_SECTION(); if (value == NULL) return NULL; @@ -3381,9 +3403,9 @@ _PyCData_set(ctypes_state *st, ((PyTypeObject *)type)->tp_name); return NULL; } - LOCK_PTR(src); + Py_BEGIN_CRITICAL_SECTION(src); *(void **)ptr = src->b_ptr; - UNLOCK_PTR(src); + Py_END_CRITICAL_SECTION(); keep = GetKeepedObjects(src); if (keep == NULL) @@ -4901,10 +4923,10 @@ Array_subscript(PyObject *myself, PyObject *item) return Py_GetConstant(Py_CONSTANT_EMPTY_BYTES); if (step == 1) { PyObject *res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = PyBytes_FromStringAndSize(ptr + start, slicelen); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } dest = (char *)PyMem_Malloc(slicelen); @@ -4912,12 +4934,12 @@ Array_subscript(PyObject *myself, PyObject *item) if (dest == NULL) return PyErr_NoMemory(); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); for (cur = start, i = 0; i < slicelen; cur += step, i++) { dest[i] = ptr[cur]; } - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); np = PyBytes_FromStringAndSize(dest, slicelen); PyMem_Free(dest); @@ -4931,10 +4953,10 @@ Array_subscript(PyObject *myself, PyObject *item) return Py_GetConstant(Py_CONSTANT_EMPTY_STR); if (step == 1) { PyObject *res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = PyUnicode_FromWideChar(ptr + start, slicelen); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -4944,12 +4966,12 @@ Array_subscript(PyObject *myself, PyObject *item) return NULL; } - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); for (cur = start, i = 0; i < slicelen; cur += step, i++) { dest[i] = ptr[cur]; } - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); np = PyUnicode_FromWideChar(dest, slicelen); PyMem_Free(dest); @@ -5287,9 +5309,9 @@ Simple_bool(PyObject *op) { int cmp; CDataObject *self = _CDataObject_CAST(op); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); 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); + Py_END_CRITICAL_SECTION(); return cmp; } @@ -5383,13 +5405,13 @@ Pointer_item(PyObject *myself, Py_ssize_t index) { CDataObject *self = _CDataObject_CAST(myself); PyObject *res; - // TODO: The plan is to make LOCK_PTR() a mutex instead of a critical + // TODO: The plan is to make Py_BEGIN_CRITICAL_SECTION() a mutex instead of a critical // section someday, so when that happens, this needs to get refactored // to be re-entrant safe. // This goes for all the locks here. - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = Pointer_item_lock_held(myself, index); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -5443,9 +5465,9 @@ Pointer_ass_item(PyObject *myself, Py_ssize_t index, PyObject *value) { CDataObject *self = _CDataObject_CAST(myself); int res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = Pointer_ass_item_lock_held(myself, index, value); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -5474,9 +5496,9 @@ Pointer_get_contents(PyObject *myself, void *closure) { CDataObject *self = _CDataObject_CAST(myself); PyObject *res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = Pointer_get_contents_lock_held(myself, closure); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } @@ -5514,13 +5536,13 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure) dst = (CDataObject *)value; if (dst != self) { - LOCK_PTR(dst); + Py_BEGIN_CRITICAL_SECTION(dst); locked_deref_assign(self, dst->b_ptr); - UNLOCK_PTR(dst); + Py_END_CRITICAL_SECTION(); } else { - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); *((void **)self->b_ptr) = dst->b_ptr; - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); } /* @@ -5677,22 +5699,22 @@ Pointer_subscript(PyObject *myself, PyObject *item) return Py_GetConstant(Py_CONSTANT_EMPTY_BYTES); if (step == 1) { PyObject *res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); char *ptr = *(void **)self->b_ptr; res = PyBytes_FromStringAndSize(ptr + start, len); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } dest = (char *)PyMem_Malloc(len); if (dest == NULL) return PyErr_NoMemory(); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); char *ptr = *(void **)self->b_ptr; for (cur = start, i = 0; i < len; cur += step, i++) { dest[i] = ptr[cur]; } - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); np = PyBytes_FromStringAndSize(dest, len); PyMem_Free(dest); return np; @@ -5704,22 +5726,22 @@ Pointer_subscript(PyObject *myself, PyObject *item) return Py_GetConstant(Py_CONSTANT_EMPTY_STR); if (step == 1) { PyObject *res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); wchar_t *ptr = *(wchar_t **)self->b_ptr; res = PyUnicode_FromWideChar(ptr + start, len); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return res; } dest = PyMem_New(wchar_t, len); if (dest == NULL) return PyErr_NoMemory(); - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); wchar_t *ptr = *(wchar_t **)self->b_ptr; for (cur = start, i = 0; i < len; cur += step, i++) { dest[i] = ptr[cur]; } - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); np = PyUnicode_FromWideChar(dest, len); PyMem_Free(dest); return np; @@ -5730,9 +5752,9 @@ Pointer_subscript(PyObject *myself, PyObject *item) return NULL; int res; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); res = copy_pointer_to_list_lock_held(myself, np, len, start, step); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); if (res < 0) { Py_DECREF(np); return NULL; diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h index 7ffe83dc63d6db..f84810abebd531 100644 --- a/Modules/_ctypes/ctypes.h +++ b/Modules/_ctypes/ctypes.h @@ -644,50 +644,40 @@ 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) Py_BEGIN_CRITICAL_SECTION(self) -# define UNLOCK_PTR(self) Py_END_CRITICAL_SECTION() -#else -/* - * Dummy functions instead of macros so that 'self' can be - * unused in the caller without triggering a compiler warning. - */ -static inline void LOCK_PTR(CDataObject *Py_UNUSED(self)) {} -static inline void UNLOCK_PTR(CDataObject *Py_UNUSED(self)) {} -#endif - +/* Equivalent to memcpy(self->b_ptr, buf, size) with a lock. */ static inline void locked_memcpy_to(CDataObject *self, void *buf, Py_ssize_t size) { - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); (void)memcpy(self->b_ptr, buf, size); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); } +/* Equivalent to memcpy(buf, self->b_ptr, size) with a lock. */ static inline void locked_memcpy_from(void *buf, CDataObject *self, Py_ssize_t size) { - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); (void)memcpy(buf, self->b_ptr, size); - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); } +/* Equivalent to *self->b_ptr with a lock. */ static inline void * locked_deref(CDataObject *self) { void *ptr; - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); ptr = *(void **)self->b_ptr; - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); return ptr; } +/* Equivalent to *self->b_ptr = new_ptr with a lock. */ static inline void locked_deref_assign(CDataObject *self, void *new_ptr) { - LOCK_PTR(self); + Py_BEGIN_CRITICAL_SECTION(self); *(void **)self->b_ptr = new_ptr; - UNLOCK_PTR(self); + Py_END_CRITICAL_SECTION(); } From 7457b2816af90b26503c979268b2c66521b62e80 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 5 Apr 2025 12:03:43 -0400 Subject: [PATCH 02/10] Fix typos. --- Modules/_ctypes/_ctypes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 676dcfd9fd0743..596ee1c91a2f61 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -2794,7 +2794,7 @@ PyCData_GetContainer_lock_held(CDataObject *self) } static CDataObject * -PyCData_GetContainer_lock_held(CDataObject *self) +PyCData_GetContainer(CDataObject *self) { CDataObject *res; Py_BEGIN_CRITICAL_SECTION(self); @@ -2891,7 +2891,7 @@ KeepRef(CDataObject *target, Py_ssize_t index, PyObject *keep) { int res; Py_BEGIN_CRITICAL_SECTION(target); - target = KeepRef_lock_held(target, index, keep); + res = KeepRef_lock_held(target, index, keep); Py_END_CRITICAL_SECTION(); return res; } From 30fdda39e215bc128fc1e3072b179c385486b841 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 5 Apr 2025 12:06:09 -0400 Subject: [PATCH 03/10] Fix build warnings. --- Modules/_ctypes/_ctypes.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 596ee1c91a2f61..ba2ae1b181b245 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -3277,10 +3277,9 @@ PyObject * PyCData_get(ctypes_state *st, PyObject *type, GETFUNC getfunc, PyObject *src, Py_ssize_t index, Py_ssize_t size, char *adr) { - CDataObject *cdata = _CDataObject_CAST(src); if (getfunc) { PyObject *res; - Py_BEGIN_CRITICAL_SECTION(cdata); + Py_BEGIN_CRITICAL_SECTION(src); res = getfunc(adr, size); Py_END_CRITICAL_SECTION(); return res; @@ -3292,7 +3291,7 @@ PyCData_get(ctypes_state *st, PyObject *type, GETFUNC getfunc, PyObject *src, } if (info && info->getfunc && !_ctypes_simple_instance(st, type)) { PyObject *res; - Py_BEGIN_CRITICAL_SECTION(cdata); + Py_BEGIN_CRITICAL_SECTION(src); res = info->getfunc(adr, size); Py_END_CRITICAL_SECTION(); return res; @@ -5401,16 +5400,11 @@ Pointer_item_lock_held(PyObject *myself, Py_ssize_t index) } static PyObject * -Pointer_item(PyObject *myself, Py_ssize_t index) +Pointer_item(PyObject *self, Py_ssize_t index) { - CDataObject *self = _CDataObject_CAST(myself); PyObject *res; - // TODO: The plan is to make Py_BEGIN_CRITICAL_SECTION() a mutex instead of a critical - // section someday, so when that happens, this needs to get refactored - // to be re-entrant safe. - // This goes for all the locks here. Py_BEGIN_CRITICAL_SECTION(self); - res = Pointer_item_lock_held(myself, index); + res = Pointer_item_lock_held(self, index); Py_END_CRITICAL_SECTION(); return res; } @@ -5461,12 +5455,11 @@ Pointer_ass_item_lock_held(PyObject *myself, Py_ssize_t index, PyObject *value) } static int -Pointer_ass_item(PyObject *myself, Py_ssize_t index, PyObject *value) +Pointer_ass_item(PyObject *self, Py_ssize_t index, PyObject *value) { - CDataObject *self = _CDataObject_CAST(myself); int res; Py_BEGIN_CRITICAL_SECTION(self); - res = Pointer_ass_item_lock_held(myself, index, value); + res = Pointer_ass_item_lock_held(self, index, value); Py_END_CRITICAL_SECTION(); return res; } @@ -5492,12 +5485,11 @@ Pointer_get_contents_lock_held(PyObject *self, void *closure) } static PyObject * -Pointer_get_contents(PyObject *myself, void *closure) +Pointer_get_contents(PyObject *self, void *closure) { - CDataObject *self = _CDataObject_CAST(myself); PyObject *res; Py_BEGIN_CRITICAL_SECTION(self); - res = Pointer_get_contents_lock_held(myself, closure); + res = Pointer_get_contents_lock_held(self, closure); Py_END_CRITICAL_SECTION(); return res; } From cdeb279fc15ac1db236e4e6e4640dc1f2c0c5eb9 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 7 Apr 2025 07:02:08 -0400 Subject: [PATCH 04/10] Add a lock to StructUnionType_paramfunc --- Modules/_ctypes/_ctypes.c | 25 +++++++++++++++++-------- Modules/_ctypes/ctypes.h | 18 ------------------ 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 786b7e64e2cd73..09c9bf77cd6220 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -594,14 +594,8 @@ PyType_Spec pyctype_type_spec = { .slots = ctype_type_slots, }; -/* - PyCStructType_Type - a meta type/class. Creating a new class using this one as - __metaclass__ will call the constructor StructUnionType_new. - It initializes the C accessible fields somehow. -*/ - static PyCArgObject * -StructUnionType_paramfunc(ctypes_state *st, CDataObject *self) +StructUnionType_paramfunc_lock_held(ctypes_state *st, CDataObject *self) { PyCArgObject *parg; PyObject *obj; @@ -612,7 +606,7 @@ StructUnionType_paramfunc(ctypes_state *st, CDataObject *self) if (ptr == NULL) { return NULL; } - locked_memcpy_from(ptr, self, self->b_size); + memcpy(ptr, self->b_ptr, self->b_size); /* Create a Python object which calls PyMem_Free(ptr) in its deallocator. The object will be destroyed @@ -653,6 +647,21 @@ StructUnionType_paramfunc(ctypes_state *st, CDataObject *self) return parg; } +/* + PyCStructType_Type - a meta type/class. Creating a new class using this one as + __metaclass__ will call the constructor StructUnionType_new. + It initializes the C accessible fields somehow. +*/ +static PyCArgObject * +StructUnionType_paramfunc(ctypes_state *st, CDataObject *self) +{ + PyCArgObject *res; + Py_BEGIN_CRITICAL_SECTION(self); + res = StructUnionType_paramfunc_lock_held(st, self); + Py_END_CRITICAL_SECTION(); + return res; +} + static int StructUnionType_init(PyObject *self, PyObject *args, PyObject *kwds, int isStruct) { diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h index f84810abebd531..a05368356a4c1b 100644 --- a/Modules/_ctypes/ctypes.h +++ b/Modules/_ctypes/ctypes.h @@ -644,24 +644,6 @@ PyStgInfo_Init(ctypes_state *state, PyTypeObject *type) return info; } -/* Equivalent to memcpy(self->b_ptr, buf, size) with a lock. */ -static inline void -locked_memcpy_to(CDataObject *self, void *buf, Py_ssize_t size) -{ - Py_BEGIN_CRITICAL_SECTION(self); - (void)memcpy(self->b_ptr, buf, size); - Py_END_CRITICAL_SECTION(); -} - -/* Equivalent to memcpy(buf, self->b_ptr, size) with a lock. */ -static inline void -locked_memcpy_from(void *buf, CDataObject *self, Py_ssize_t size) -{ - Py_BEGIN_CRITICAL_SECTION(self); - (void)memcpy(buf, self->b_ptr, size); - Py_END_CRITICAL_SECTION(); -} - /* Equivalent to *self->b_ptr with a lock. */ static inline void * locked_deref(CDataObject *self) From 7b5120151e6d18236b17147bfd7a42c3835112b0 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 7 Apr 2025 07:04:02 -0400 Subject: [PATCH 05/10] Lock not needed. --- Modules/_ctypes/_ctypes.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 09c9bf77cd6220..782c22a2e0b752 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -110,6 +110,7 @@ bytes(cdata) #include "pycore_ceval.h" // _Py_EnterRecursiveCall() #include "pycore_unicodeobject.h" // _PyUnicode_EqualToASCIIString() #include "pycore_pyatomic_ft_wrappers.h" +#include "pycore_object.h" #ifdef MS_WIN32 # include "pycore_modsupport.h" // _PyArg_NoKeywords() #endif @@ -932,7 +933,8 @@ CDataType_from_buffer_copy_impl(PyObject *type, PyTypeObject *cls, result = generic_pycdata_new(st, (PyTypeObject *)type, NULL, NULL); if (result != NULL) { - locked_memcpy_to((CDataObject *) result, (char *)buffer->buf + offset, info->size); + assert(_PyObject_IsUniquelyReferenced(result)); + memcpy(((CDataObject *) result)->b_ptr, (char *)buffer->buf + offset, info->size); } return result; } From a17024c0d8c7ef0aaf5ee705f58da97f0345497e Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 7 Apr 2025 07:08:37 -0400 Subject: [PATCH 06/10] Remove some more locked_memcpy() --- Modules/_ctypes/_ctypes.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 782c22a2e0b752..df38eb81930d8a 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -1469,7 +1469,7 @@ _ctypes_PyCArrayType_Type_raw_set_impl(CDataObject *self, PyObject *value) goto fail; } - locked_memcpy_to(self, ptr, size); + memcpy(self->b_ptr, ptr, size); PyBuffer_Release(&view); return 0; @@ -2248,7 +2248,7 @@ static PyObject *CreateSwappedType(ctypes_state *st, PyTypeObject *type, } static PyCArgObject * -PyCSimpleType_paramfunc(ctypes_state *st, CDataObject *self) +PyCSimpleType_paramfunc_lock_held(ctypes_state *st, CDataObject *self) { const char *fmt; PyCArgObject *parg; @@ -2272,10 +2272,20 @@ PyCSimpleType_paramfunc(ctypes_state *st, CDataObject *self) parg->tag = fmt[0]; parg->pffi_type = fd->pffi_type; parg->obj = Py_NewRef(self); - locked_memcpy_from(&parg->value, self, self->b_size); + memcpy(&parg->value, self->b_ptr, self->b_size); return parg; } +static PyCArgObject * +PyCSimpleType_paramfunc(ctypes_state *st, CDataObject *self) +{ + PyCArgObject *res; + Py_BEGIN_CRITICAL_SECTION(self); + res = PyCSimpleType_paramfunc_lock_held(st, self); + Py_END_CRITICAL_SECTION(); + return res; +} + static int PyCSimpleType_init(PyObject *self, PyObject *args, PyObject *kwds) { @@ -5975,7 +5985,7 @@ cast_check_pointertype(ctypes_state *st, PyObject *arg) } static PyObject * -cast(void *ptr, PyObject *src, PyObject *ctype) +cast_lock_held(void *ptr, PyObject *src, PyObject *ctype) { PyObject *mod = PyType_GetModuleByDef(Py_TYPE(ctype), &_ctypesmodule); if (!mod) { @@ -6030,7 +6040,7 @@ cast(void *ptr, PyObject *src, PyObject *ctype) } } /* Should we assert that result is a pointer type? */ - locked_memcpy_to(result, &ptr, sizeof(void *)); + memcpy(result->b_ptr, &ptr, sizeof(void *)); return (PyObject *)result; failed: @@ -6038,6 +6048,15 @@ cast(void *ptr, PyObject *src, PyObject *ctype) return NULL; } +static PyObject * +cast(void *ptr, PyObject *src, PyObject *ctype) +{ + PyObject *res; + Py_BEGIN_CRITICAL_SECTION(src); + res = cast_lock_held(ptr, src, ctype); + Py_END_CRITICAL_SECTION(); + return res; +} static PyObject * wstring_at(const wchar_t *ptr, int size) From 2652de2b78ab25ad71945d523b07a773a0895e43 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 7 Apr 2025 07:18:29 -0400 Subject: [PATCH 07/10] Remove usage of locked_deref_assign() --- Modules/_ctypes/_ctypes.c | 24 +++++++++++++----------- Modules/_ctypes/ctypes.h | 9 --------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index df38eb81930d8a..ded50caaa75d67 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -3472,9 +3472,9 @@ _PyCData_set(ctypes_state *st, } Py_BEGIN_CRITICAL_SECTION(src); *(void **)ptr = src->b_ptr; - Py_END_CRITICAL_SECTION(); keep = GetKeepedObjects(src); + Py_END_CRITICAL_SECTION(); if (keep == NULL) return NULL; @@ -5563,7 +5563,7 @@ Pointer_get_contents(PyObject *self, void *closure) } static int -Pointer_set_contents(PyObject *op, PyObject *value, void *closure) +Pointer_set_contents_lock_held(PyObject *op, PyObject *value, void *closure) { CDataObject *dst; PyObject *keep; @@ -5595,15 +5595,7 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure) } dst = (CDataObject *)value; - if (dst != self) { - Py_BEGIN_CRITICAL_SECTION(dst); - locked_deref_assign(self, dst->b_ptr); - Py_END_CRITICAL_SECTION(); - } else { - Py_BEGIN_CRITICAL_SECTION(self); - *((void **)self->b_ptr) = dst->b_ptr; - Py_END_CRITICAL_SECTION(); - } + *((void **)self->b_ptr) = dst->b_ptr; /* A Pointer instance must keep the value it points to alive. So, a @@ -5622,6 +5614,16 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure) return KeepRef(self, 0, keep); } +static int +Pointer_set_contents(PyObject *op, PyObject *value, void *closure) +{ + int res; + Py_BEGIN_CRITICAL_SECTION2(op, value); + res = Pointer_set_contents_lock_held(op, value, closure); + Py_END_CRITICAL_SECTION2(); + return res; +} + static PyGetSetDef Pointer_getsets[] = { { "contents", Pointer_get_contents, Pointer_set_contents, "the object this pointer points to (read-write)", NULL }, diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h index a05368356a4c1b..056c73e0208854 100644 --- a/Modules/_ctypes/ctypes.h +++ b/Modules/_ctypes/ctypes.h @@ -654,12 +654,3 @@ locked_deref(CDataObject *self) Py_END_CRITICAL_SECTION(); return ptr; } - -/* Equivalent to *self->b_ptr = new_ptr with a lock. */ -static inline void -locked_deref_assign(CDataObject *self, void *new_ptr) -{ - Py_BEGIN_CRITICAL_SECTION(self); - *(void **)self->b_ptr = new_ptr; - Py_END_CRITICAL_SECTION(); -} From 95231de6e221d22f4a9abbf44aba8c901e4657b0 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 7 Apr 2025 07:20:14 -0400 Subject: [PATCH 08/10] Remove AC-replaced function. --- Modules/_ctypes/_ctypes.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index ded50caaa75d67..c3750f88e8c2e3 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -1478,16 +1478,6 @@ _ctypes_PyCArrayType_Type_raw_set_impl(CDataObject *self, PyObject *value) return -1; } -static PyObject * -CharArray_get_raw(PyObject *op, void *Py_UNUSED(ignored)) -{ - PyObject *res; - CDataObject *self = _CDataObject_CAST(op); - Py_BEGIN_CRITICAL_SECTION(self); - res = PyBytes_FromStringAndSize(self->b_ptr, self->b_size); - Py_END_CRITICAL_SECTION(); - return res; -} /*[clinic input] @critical_section @getter From 3af4f9e043d7855c71ad40543ccabb5ecb1f68a3 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 7 Apr 2025 07:24:18 -0400 Subject: [PATCH 09/10] Add _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED() --- Modules/_ctypes/_ctypes.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index c3750f88e8c2e3..987b35ecba4d59 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -598,6 +598,7 @@ PyType_Spec pyctype_type_spec = { static PyCArgObject * StructUnionType_paramfunc_lock_held(ctypes_state *st, CDataObject *self) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); PyCArgObject *parg; PyObject *obj; void *ptr; @@ -1563,6 +1564,7 @@ static PyGetSetDef CharArray_getsets[] = { static PyObject * WCharArray_get_value_lock_held(PyObject *op) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op); Py_ssize_t i; PyObject *res; CDataObject *self = _CDataObject_CAST(op); @@ -1587,6 +1589,7 @@ WCharArray_get_value(PyObject *op, void *Py_UNUSED(ignored)) static int WCharArray_set_value_lock_held(PyObject *op, PyObject *value) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op); CDataObject *self = _CDataObject_CAST(op); if (value == NULL) { @@ -2240,6 +2243,7 @@ static PyObject *CreateSwappedType(ctypes_state *st, PyTypeObject *type, static PyCArgObject * PyCSimpleType_paramfunc_lock_held(ctypes_state *st, CDataObject *self) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); const char *fmt; PyCArgObject *parg; struct fielddesc *fd; @@ -2836,6 +2840,7 @@ static PyType_Spec pycfuncptr_type_spec = { static CDataObject * PyCData_GetContainer_lock_held(CDataObject *self) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); while (self->b_base) { self = self->b_base; } @@ -2897,6 +2902,7 @@ unique_key(CDataObject *target, Py_ssize_t index) static int KeepRef_lock_held(CDataObject *target, Py_ssize_t index, PyObject *keep) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(target); int result; CDataObject *ob; PyObject *key; @@ -4492,6 +4498,7 @@ _build_result(PyObject *result, PyObject *callargs, static PyObject * PyCFuncPtr_call_lock_held(PyObject *op, PyObject *inargs, PyObject *kwds) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op); PyObject *restype; PyObject *converters; PyObject *checker; @@ -5422,6 +5429,7 @@ static PyObject * Pointer_item_lock_held(PyObject *myself, Py_ssize_t index) { CDataObject *self = _CDataObject_CAST(myself); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); Py_ssize_t size; Py_ssize_t offset; PyObject *proto; @@ -5471,6 +5479,7 @@ static int Pointer_ass_item_lock_held(PyObject *myself, Py_ssize_t index, PyObject *value) { CDataObject *self = _CDataObject_CAST(myself); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); Py_ssize_t size; Py_ssize_t offset; PyObject *proto; @@ -5525,6 +5534,7 @@ Pointer_ass_item(PyObject *self, Py_ssize_t index, PyObject *value) static PyObject * Pointer_get_contents_lock_held(PyObject *self, void *closure) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); void *deref = *(void **)_CDataObject_CAST(self)->b_ptr; if (deref == NULL) { PyErr_SetString(PyExc_ValueError, @@ -5555,6 +5565,7 @@ Pointer_get_contents(PyObject *self, void *closure) static int Pointer_set_contents_lock_held(PyObject *op, PyObject *value, void *closure) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op); CDataObject *dst; PyObject *keep; CDataObject *self = _CDataObject_CAST(op); @@ -5651,6 +5662,7 @@ static int copy_pointer_to_list_lock_held(PyObject *myself, PyObject *np, Py_ssize_t len, Py_ssize_t start, Py_ssize_t step) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(myself); Py_ssize_t i; size_t cur; for (cur = start, i = 0; i < len; cur += step, i++) { @@ -5804,7 +5816,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) return NULL; int res; - Py_BEGIN_CRITICAL_SECTION(self); + Py_BEGIN_CRITICAL_SECTION(myself); res = copy_pointer_to_list_lock_held(myself, np, len, start, step); Py_END_CRITICAL_SECTION(); if (res < 0) { @@ -5979,6 +5991,7 @@ cast_check_pointertype(ctypes_state *st, PyObject *arg) static PyObject * cast_lock_held(void *ptr, PyObject *src, PyObject *ctype) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(src); PyObject *mod = PyType_GetModuleByDef(Py_TYPE(ctype), &_ctypesmodule); if (!mod) { PyErr_SetString(PyExc_TypeError, From dcdc517c85b8ca33295dc9e6033925917a92d3d9 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Mon, 7 Apr 2025 18:59:58 +0530 Subject: [PATCH 10/10] remove lock from get container --- Modules/_ctypes/_ctypes.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 987b35ecba4d59..bba006772efbfa 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -2838,9 +2838,8 @@ static PyType_Spec pycfuncptr_type_spec = { */ static CDataObject * -PyCData_GetContainer_lock_held(CDataObject *self) +PyCData_GetContainer(CDataObject *self) { - _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self); while (self->b_base) { self = self->b_base; } @@ -2856,16 +2855,6 @@ PyCData_GetContainer_lock_held(CDataObject *self) return self; } -static CDataObject * -PyCData_GetContainer(CDataObject *self) -{ - CDataObject *res; - Py_BEGIN_CRITICAL_SECTION(self); - res = PyCData_GetContainer_lock_held(self); - Py_END_CRITICAL_SECTION(); - return res; -} - static PyObject * GetKeepedObjects(CDataObject *target) {