From 0a27451b3dafb71256800eb327b6bc7c2af590de Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 1 Apr 2025 09:18:49 -0400 Subject: [PATCH 1/8] Fix usages of locked_deref() --- Modules/_ctypes/_ctypes.c | 89 ++++++++++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 15 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 59ea579627e56f..6ab3020488385f 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5326,13 +5326,13 @@ static PyType_Spec pycsimple_spec = { PyCPointer_Type */ static PyObject * -Pointer_item(PyObject *myself, Py_ssize_t index) +Pointer_item_lock_held(PyObject *myself, Py_ssize_t index) { CDataObject *self = _CDataObject_CAST(myself); Py_ssize_t size; Py_ssize_t offset; PyObject *proto; - void *deref = locked_deref(self); + void *deref = *(void **)self->b_ptr; if (deref == NULL) { PyErr_SetString(PyExc_ValueError, @@ -5364,8 +5364,23 @@ Pointer_item(PyObject *myself, Py_ssize_t index) index, size, (char *)((char *)deref + offset)); } +static PyObject * +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 + // 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); + return res; +} + static int -Pointer_ass_item(PyObject *myself, Py_ssize_t index, PyObject *value) +Pointer_ass_item_lock_held(PyObject *myself, Py_ssize_t index, PyObject *value) { CDataObject *self = _CDataObject_CAST(myself); Py_ssize_t size; @@ -5378,7 +5393,7 @@ Pointer_ass_item(PyObject *myself, Py_ssize_t index, PyObject *value) return -1; } - void *deref = locked_deref(self); + void *deref = *(void **)self->b_ptr; if (deref == NULL) { PyErr_SetString(PyExc_ValueError, "NULL pointer access"); @@ -5409,10 +5424,21 @@ Pointer_ass_item(PyObject *myself, Py_ssize_t index, PyObject *value) index, size, ((char *)deref + offset)); } +static int +Pointer_ass_item(PyObject *myself, 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); + return res; +} + static PyObject * -Pointer_get_contents(PyObject *self, void *closure) +Pointer_get_contents_lock_held(PyObject *self, void *closure) { - void *deref = locked_deref(_CDataObject_CAST(self)); + void *deref = *(void **)_CDataObject_CAST(self)->b_ptr; if (deref == NULL) { PyErr_SetString(PyExc_ValueError, "NULL pointer access"); @@ -5429,6 +5455,17 @@ Pointer_get_contents(PyObject *self, void *closure) return PyCData_FromBaseObj(st, stginfo->proto, self, 0, deref); } +static PyObject * +Pointer_get_contents(PyObject *myself, void *closure) +{ + CDataObject *self = _CDataObject_CAST(myself); + PyObject *res; + LOCK_PTR(self); + res = Pointer_get_contents_lock_held(myself, closure); + UNLOCK_PTR(self); + return res; +} + static int Pointer_set_contents(PyObject *op, PyObject *value, void *closure) { @@ -5462,7 +5499,10 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure) } dst = (CDataObject *)value; + assert(dst != self); // XXX Can the user do this? + LOCK_PTR(dst); locked_deref_assign(self, dst->b_ptr); + UNLOCK_PTR(dst); /* A Pointer instance must keep the value it points to alive. So, a @@ -5514,6 +5554,22 @@ Pointer_new(PyTypeObject *type, PyObject *args, PyObject *kw) return generic_pycdata_new(st, type, args, kw); } +static int +copy_pointer_to_list_lock_held(PyObject *myself, PyObject *np, Py_ssize_t start, Py_ssize_t step) +{ + Py_ssize_t i, len; + size_t cur; + for (cur = start, i = 0; i < len; cur += step, i++) { + PyObject *v = Pointer_item_lock_held(myself, cur); + if (!v) { + return -1; + } + PyList_SET_ITEM(np, i, v); + } + + return 0; +} + static PyObject * Pointer_subscript(PyObject *myself, PyObject *item) { @@ -5595,7 +5651,6 @@ Pointer_subscript(PyObject *myself, PyObject *item) } assert(iteminfo); if (iteminfo->getfunc == _ctypes_get_fielddesc("c")->getfunc) { - char *ptr = locked_deref(self); char *dest; if (len <= 0) @@ -5603,6 +5658,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (step == 1) { PyObject *res; LOCK_PTR(self); + char *ptr = *(void **)self->b_ptr; res = PyBytes_FromStringAndSize(ptr + start, len); UNLOCK_PTR(self); @@ -5612,6 +5668,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (dest == NULL) return PyErr_NoMemory(); LOCK_PTR(self); + char *ptr = *(void **)self->b_ptr; for (cur = start, i = 0; i < len; cur += step, i++) { dest[i] = ptr[cur]; } @@ -5621,7 +5678,6 @@ Pointer_subscript(PyObject *myself, PyObject *item) return np; } if (iteminfo->getfunc == _ctypes_get_fielddesc("u")->getfunc) { - wchar_t *ptr = locked_deref(self); wchar_t *dest; if (len <= 0) @@ -5629,6 +5685,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (step == 1) { PyObject *res; LOCK_PTR(self); + wchar_t *ptr = *(void **)self; res = PyUnicode_FromWideChar(ptr + start, len); UNLOCK_PTR(self); @@ -5638,6 +5695,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (dest == NULL) return PyErr_NoMemory(); LOCK_PTR(self); + wchar_t *ptr = *(void **)self; for (cur = start, i = 0; i < len; cur += step, i++) { dest[i] = ptr[cur]; } @@ -5651,14 +5709,15 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (np == NULL) return NULL; - for (cur = start, i = 0; i < len; cur += step, i++) { - PyObject *v = Pointer_item(myself, cur); - if (!v) { - Py_DECREF(np); - return NULL; - } - PyList_SET_ITEM(np, i, v); + int res; + LOCK_PTR(self); + res = copy_pointer_to_list_lock_held(myself, np, start, step); + UNLOCK_PTR(self); + if (res < 0) { + Py_DECREF(np); + return NULL; } + return np; } else { From e2a4ea0c8f29a8b3b12c396e84c4795690f759be Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 1 Apr 2025 09:20:37 -0400 Subject: [PATCH 2/8] Add blurb entry. --- .../next/Library/2025-04-01-09-20-32.gh-issue-131974.AIzshA.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2025-04-01-09-20-32.gh-issue-131974.AIzshA.rst diff --git a/Misc/NEWS.d/next/Library/2025-04-01-09-20-32.gh-issue-131974.AIzshA.rst b/Misc/NEWS.d/next/Library/2025-04-01-09-20-32.gh-issue-131974.AIzshA.rst new file mode 100644 index 00000000000000..abf2f54f8c2035 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-04-01-09-20-32.gh-issue-131974.AIzshA.rst @@ -0,0 +1,2 @@ +Fix several thread-safety issues in :mod:`ctypes` on the :term:`free +threaded ` build. From 22004fbce7e2e119ebd065a3bd7e8fe2ff460705 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 1 Apr 2025 09:24:22 -0400 Subject: [PATCH 3/8] Pass len to copy_pointer_to_list_lock_held() --- Modules/_ctypes/_ctypes.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 6ab3020488385f..da816d98894993 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5555,9 +5555,10 @@ Pointer_new(PyTypeObject *type, PyObject *args, PyObject *kw) } static int -copy_pointer_to_list_lock_held(PyObject *myself, PyObject *np, Py_ssize_t start, Py_ssize_t step) +copy_pointer_to_list_lock_held(PyObject *myself, PyObject *np, Py_ssize_t len, + Py_ssize_t start, Py_ssize_t step) { - Py_ssize_t i, len; + Py_ssize_t i; size_t cur; for (cur = start, i = 0; i < len; cur += step, i++) { PyObject *v = Pointer_item_lock_held(myself, cur); @@ -5711,7 +5712,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) int res; LOCK_PTR(self); - res = copy_pointer_to_list_lock_held(myself, np, start, step); + res = copy_pointer_to_list_lock_held(myself, np, len, start, step); UNLOCK_PTR(self); if (res < 0) { Py_DECREF(np); From 8f52814658ff5cf7a597f7e6eff984240c0fe268 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 1 Apr 2025 09:26:43 -0400 Subject: [PATCH 4/8] Fix wchar_t dereferences. --- 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 da816d98894993..312d05b2817b3b 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5686,7 +5686,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (step == 1) { PyObject *res; LOCK_PTR(self); - wchar_t *ptr = *(void **)self; + wchar_t *ptr = *(wchar_t **)self->b_ptr; res = PyUnicode_FromWideChar(ptr + start, len); UNLOCK_PTR(self); @@ -5696,7 +5696,7 @@ Pointer_subscript(PyObject *myself, PyObject *item) if (dest == NULL) return PyErr_NoMemory(); LOCK_PTR(self); - wchar_t *ptr = *(void **)self; + wchar_t *ptr = *(wchar_t **)self->b_ptr; for (cur = start, i = 0; i < len; cur += step, i++) { dest[i] = ptr[cur]; } From 011c747d1c4464d17646f8a34eee590d95a9fc5a Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 2 Apr 2025 07:09:36 -0400 Subject: [PATCH 5/8] Update Modules/_ctypes/_ctypes.c Co-authored-by: Victor Stinner --- Modules/_ctypes/_ctypes.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 312d05b2817b3b..de8a8e434f875c 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5558,9 +5558,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_ssize_t i; - size_t cur; - for (cur = start, i = 0; i < len; cur += step, i++) { + for (size_t cur = start, Py_ssize_t i = 0; i < len; cur += step, i++) { PyObject *v = Pointer_item_lock_held(myself, cur); if (!v) { return -1; From c31a405bd8d97f0b8e77dad228180e1c731c0541 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 2 Apr 2025 08:44:39 -0400 Subject: [PATCH 6/8] Revert "Update Modules/_ctypes/_ctypes.c" This reverts commit 011c747d1c4464d17646f8a34eee590d95a9fc5a. --- 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 de8a8e434f875c..312d05b2817b3b 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5558,7 +5558,9 @@ static int copy_pointer_to_list_lock_held(PyObject *myself, PyObject *np, Py_ssize_t len, Py_ssize_t start, Py_ssize_t step) { - for (size_t cur = start, Py_ssize_t i = 0; i < len; cur += step, i++) { + Py_ssize_t i; + size_t cur; + for (cur = start, i = 0; i < len; cur += step, i++) { PyObject *v = Pointer_item_lock_held(myself, cur); if (!v) { return -1; From 7a44c77a3bd12600478b8f99bf06a9bc6da769a2 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 2 Apr 2025 08:58:24 -0400 Subject: [PATCH 7/8] Only lock if we're not self. --- Modules/_ctypes/_ctypes.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 312d05b2817b3b..03c9045c9fccdc 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5499,10 +5499,14 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure) } dst = (CDataObject *)value; - assert(dst != self); // XXX Can the user do this? - LOCK_PTR(dst); - locked_deref_assign(self, dst->b_ptr); - UNLOCK_PTR(dst); + if (dst != self) { + LOCK_PTR(dst); + locked_deref_assign(self, dst->b_ptr); + UNLOCK_PTR(dst); + } else { + // We already hold the lock + *((void **)self->b_ptr) = dst->b_ptr; + } /* A Pointer instance must keep the value it points to alive. So, a From 9648678103d8f4051678291c15aaa82e093e277d Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 2 Apr 2025 09:28:23 -0400 Subject: [PATCH 8/8] Add the lock. --- Modules/_ctypes/_ctypes.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c index 03c9045c9fccdc..efefc0157237bd 100644 --- a/Modules/_ctypes/_ctypes.c +++ b/Modules/_ctypes/_ctypes.c @@ -5504,8 +5504,9 @@ Pointer_set_contents(PyObject *op, PyObject *value, void *closure) locked_deref_assign(self, dst->b_ptr); UNLOCK_PTR(dst); } else { - // We already hold the lock + LOCK_PTR(self); *((void **)self->b_ptr) = dst->b_ptr; + UNLOCK_PTR(self); } /*