gh-128182: switch ctypes locking to critical sections (#132133)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
This commit is contained in:
Peter Bierma 2025-04-07 12:30:31 -04:00 committed by GitHub
parent ed99e28d5b
commit 8e260b384a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 148 additions and 137 deletions

View file

@ -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
@ -594,15 +595,10 @@ 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)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self);
PyCArgObject *parg;
PyObject *obj;
void *ptr;
@ -612,7 +608,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 +649,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)
{
@ -923,7 +934,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;
}
@ -1458,7 +1470,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;
@ -1552,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);
@ -1576,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) {
@ -2227,8 +2241,9 @@ 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)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self);
const char *fmt;
PyCArgObject *parg;
struct fielddesc *fd;
@ -2251,10 +2266,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)
{
@ -2863,27 +2888,10 @@ 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)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(target);
int result;
CDataObject *ob;
PyObject *key;
@ -2913,6 +2921,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);
res = KeepRef_lock_held(target, index, keep);
Py_END_CRITICAL_SECTION();
return res;
}
/******************************************************************/
/*
PyCData_Type
@ -3294,12 +3330,11 @@ 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;
LOCK_PTR(cdata);
Py_BEGIN_CRITICAL_SECTION(src);
res = getfunc(adr, size);
UNLOCK_PTR(cdata);
Py_END_CRITICAL_SECTION();
return res;
}
assert(type);
@ -3309,9 +3344,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(src);
res = info->getfunc(adr, size);
UNLOCK_PTR(cdata);
Py_END_CRITICAL_SECTION();
return res;
}
return PyCData_FromBaseObj(st, type, src, index, adr);
@ -3330,9 +3365,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)) {
@ -3342,9 +3377,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;
}
/*
@ -3366,9 +3401,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,
@ -3384,13 +3419,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;
@ -3418,11 +3455,11 @@ _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);
keep = GetKeepedObjects(src);
Py_END_CRITICAL_SECTION();
if (keep == NULL)
return NULL;
@ -4450,6 +4487,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;
@ -4938,10 +4976,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);
@ -4949,12 +4987,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);
@ -4968,10 +5006,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;
}
@ -4981,12 +5019,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);
@ -5324,9 +5362,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;
}
@ -5380,6 +5418,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;
@ -5416,17 +5455,12 @@ 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 LOCK_PTR() 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);
res = Pointer_item_lock_held(myself, index);
UNLOCK_PTR(self);
Py_BEGIN_CRITICAL_SECTION(self);
res = Pointer_item_lock_held(self, index);
Py_END_CRITICAL_SECTION();
return res;
}
@ -5434,6 +5468,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;
@ -5476,19 +5511,19 @@ 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;
LOCK_PTR(self);
res = Pointer_ass_item_lock_held(myself, index, value);
UNLOCK_PTR(self);
Py_BEGIN_CRITICAL_SECTION(self);
res = Pointer_ass_item_lock_held(self, index, value);
Py_END_CRITICAL_SECTION();
return res;
}
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,
@ -5507,19 +5542,19 @@ 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;
LOCK_PTR(self);
res = Pointer_get_contents_lock_held(myself, closure);
UNLOCK_PTR(self);
Py_BEGIN_CRITICAL_SECTION(self);
res = Pointer_get_contents_lock_held(self, closure);
Py_END_CRITICAL_SECTION();
return res;
}
static int
Pointer_set_contents(PyObject *op, PyObject *value, void *closure)
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);
@ -5550,15 +5585,7 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure)
}
dst = (CDataObject *)value;
if (dst != self) {
LOCK_PTR(dst);
locked_deref_assign(self, dst->b_ptr);
UNLOCK_PTR(dst);
} else {
LOCK_PTR(self);
*((void **)self->b_ptr) = dst->b_ptr;
UNLOCK_PTR(self);
}
*((void **)self->b_ptr) = dst->b_ptr;
/*
A Pointer instance must keep the value it points to alive. So, a
@ -5577,6 +5604,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 },
@ -5614,6 +5651,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++) {
@ -5714,22 +5752,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;
@ -5741,22 +5779,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;
@ -5767,9 +5805,9 @@ Pointer_subscript(PyObject *myself, PyObject *item)
return NULL;
int res;
LOCK_PTR(self);
Py_BEGIN_CRITICAL_SECTION(myself);
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;
@ -5940,8 +5978,9 @@ 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)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(src);
PyObject *mod = PyType_GetModuleByDef(Py_TYPE(ctype), &_ctypesmodule);
if (!mod) {
PyErr_SetString(PyExc_TypeError,
@ -5995,7 +6034,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:
@ -6003,6 +6042,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)
@ -6342,4 +6390,4 @@ PyMODINIT_FUNC
PyInit__ctypes(void)
{
return PyModuleDef_Init(&_ctypesmodule);
}
}

View file

@ -644,50 +644,13 @@ 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
static inline void
locked_memcpy_to(CDataObject *self, void *buf, Py_ssize_t size)
{
LOCK_PTR(self);
(void)memcpy(self->b_ptr, buf, size);
UNLOCK_PTR(self);
}
static inline void
locked_memcpy_from(void *buf, CDataObject *self, Py_ssize_t size)
{
LOCK_PTR(self);
(void)memcpy(buf, self->b_ptr, size);
UNLOCK_PTR(self);
}
/* 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;
}
static inline void
locked_deref_assign(CDataObject *self, void *new_ptr)
{
LOCK_PTR(self);
*(void **)self->b_ptr = new_ptr;
UNLOCK_PTR(self);
}