From 091ae930cd8bac805a083ea68e4f886200f57de0 Mon Sep 17 00:00:00 2001
From: "Miss Islington (bot)"
<31488909+miss-islington@users.noreply.github.com>
Date: Fri, 30 Aug 2019 05:50:44 -0700
Subject: [PATCH] 00331: Fix StructUnionType_paramfunc()
Fix a ctypes regression of Python 3.8. When a ctypes.Structure is
passed by copy to a function, ctypes internals created a temporary
object which had the side effect of calling the structure finalizer
(__del__) twice. The Python semantics requires a finalizer to be
called exactly once. Fix ctypes internals to no longer call the
finalizer twice.
Create a new internal StructParam_Type which is only used by
_ctypes_callproc() to call PyMem_Free(ptr) on Py_DECREF(argument).
StructUnionType_paramfunc() creates such object.
(cherry picked from commit 96b4087ce784ee7434dffdf69c475f5b40543982)
Co-authored-by: Victor Stinner <vstinner@redhat.com>
---
Lib/ctypes/test/test_structures.py | 51 +++++++++++--
.../2019-08-30-11-21-10.bpo-37140.cFAX-a.rst | 5 ++
Modules/_ctypes/_ctypes.c | 73 +++++++++++++++----
3 files changed, 109 insertions(+), 20 deletions(-)
create mode 100644 Misc/NEWS.d/next/Library/2019-08-30-11-21-10.bpo-37140.cFAX-a.rst
diff --git a/Lib/ctypes/test/test_structures.py b/Lib/ctypes/test/test_structures.py
index d1ea43bc7e..fda104563d 100644
--- a/Lib/ctypes/test/test_structures.py
+++ b/Lib/ctypes/test/test_structures.py
@@ -3,7 +3,7 @@ from ctypes import *
from ctypes.test import need_symbol
from struct import calcsize
import _ctypes_test
-import test.support
+from test import support
class SubclassesTest(unittest.TestCase):
def test_subclass(self):
@@ -202,7 +202,7 @@ class StructureTestCase(unittest.TestCase):
"_pack_": -1}
self.assertRaises(ValueError, type(Structure), "X", (Structure,), d)
- @test.support.cpython_only
+ @support.cpython_only
def test_packed_c_limits(self):
# Issue 15989
import _testcapi
@@ -396,27 +396,66 @@ class StructureTestCase(unittest.TestCase):
self.assertRaises(TypeError, lambda: Z(1, 2, 3, 4, 5, 6, 7))
def test_pass_by_value(self):
- # This should mirror the structure in Modules/_ctypes/_ctypes_test.c
- class X(Structure):
+ # This should mirror the Test structure
+ # in Modules/_ctypes/_ctypes_test.c
+ class Test(Structure):
_fields_ = [
('first', c_ulong),
('second', c_ulong),
('third', c_ulong),
]
- s = X()
+ s = Test()
s.first = 0xdeadbeef
s.second = 0xcafebabe
s.third = 0x0bad1dea
dll = CDLL(_ctypes_test.__file__)
func = dll._testfunc_large_struct_update_value
- func.argtypes = (X,)
+ func.argtypes = (Test,)
func.restype = None
func(s)
self.assertEqual(s.first, 0xdeadbeef)
self.assertEqual(s.second, 0xcafebabe)
self.assertEqual(s.third, 0x0bad1dea)
+ def test_pass_by_value_finalizer(self):
+ # bpo-37140: Similar to test_pass_by_value(), but the Python structure
+ # has a finalizer (__del__() method): the finalizer must only be called
+ # once.
+
+ finalizer_calls = []
+
+ class Test(Structure):
+ _fields_ = [
+ ('first', c_ulong),
+ ('second', c_ulong),
+ ('third', c_ulong),
+ ]
+ def __del__(self):
+ finalizer_calls.append("called")
+
+ s = Test(1, 2, 3)
+ # Test the StructUnionType_paramfunc() code path which copies the
+ # structure: if the stucture is larger than sizeof(void*).
+ self.assertGreater(sizeof(s), sizeof(c_void_p))
+
+ dll = CDLL(_ctypes_test.__file__)
+ func = dll._testfunc_large_struct_update_value
+ func.argtypes = (Test,)
+ func.restype = None
+ func(s)
+ # bpo-37140: Passing the structure by refrence must not call
+ # its finalizer!
+ self.assertEqual(finalizer_calls, [])
+ self.assertEqual(s.first, 1)
+ self.assertEqual(s.second, 2)
+ self.assertEqual(s.third, 3)
+
+ # The finalizer must be called exactly once
+ s = None
+ support.gc_collect()
+ self.assertEqual(finalizer_calls, ["called"])
+
def test_pass_by_value_in_register(self):
class X(Structure):
_fields_ = [
diff --git a/Misc/NEWS.d/next/Library/2019-08-30-11-21-10.bpo-37140.cFAX-a.rst b/Misc/NEWS.d/next/Library/2019-08-30-11-21-10.bpo-37140.cFAX-a.rst
new file mode 100644
index 0000000000..4eaa226147
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2019-08-30-11-21-10.bpo-37140.cFAX-a.rst
@@ -0,0 +1,5 @@
+Fix a ctypes regression of Python 3.8. When a ctypes.Structure is passed by
+copy to a function, ctypes internals created a temporary object which had
+the side effect of calling the structure finalizer (__del__) twice. The
+Python semantics requires a finalizer to be called exactly once. Fix ctypes
+internals to no longer call the finalizer twice.
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index 2201c4520a..4728874006 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -392,6 +392,35 @@ _ctypes_alloc_format_string_with_shape(int ndim, const Py_ssize_t *shape,
return result;
}
+/* StructParamObject and StructParam_Type are used in _ctypes_callproc()
+ for argument.keep to call PyMem_Free(ptr) on Py_DECREF(argument).
+
+ StructUnionType_paramfunc() creates such object when a ctypes Structure is
+ passed by copy to a C function. */
+typedef struct {
+ PyObject_HEAD
+ void *ptr;
+} StructParamObject;
+
+
+static void
+StructParam_dealloc(PyObject *myself)
+{
+ StructParamObject *self = (StructParamObject *)myself;
+ PyMem_Free(self->ptr);
+ Py_TYPE(self)->tp_free(myself);
+}
+
+
+static PyTypeObject StructParam_Type = {
+ PyVarObject_HEAD_INIT(NULL, 0)
+ .tp_name = "_ctypes.StructParam_Type",
+ .tp_basicsize = sizeof(StructParamObject),
+ .tp_dealloc = StructParam_dealloc,
+ .tp_flags = Py_TPFLAGS_DEFAULT,
+};
+
+
/*
PyCStructType_Type - a meta type/class. Creating a new class using this one as
__metaclass__ will call the constructor StructUnionType_new. It replaces the
@@ -403,35 +432,47 @@ static PyCArgObject *
StructUnionType_paramfunc(CDataObject *self)
{
PyCArgObject *parg;
- CDataObject *copied_self;
+ PyObject *obj;
StgDictObject *stgdict;
+ void *ptr;
if ((size_t)self->b_size > sizeof(void*)) {
- void *new_ptr = PyMem_Malloc(self->b_size);
- if (new_ptr == NULL)
+ ptr = PyMem_Malloc(self->b_size);
+ if (ptr == NULL) {
return NULL;
- memcpy(new_ptr, self->b_ptr, self->b_size);
- copied_self = (CDataObject *)PyCData_AtAddress(
- (PyObject *)Py_TYPE(self), new_ptr);
- copied_self->b_needsfree = 1;
+ }
+ 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
+ at _ctypes_callproc() cleanup. */
+ obj = (&StructParam_Type)->tp_alloc(&StructParam_Type, 0);
+ if (obj == NULL) {
+ PyMem_Free(ptr);
+ return NULL;
+ }
+
+ StructParamObject *struct_param = (StructParamObject *)obj;
+ struct_param->ptr = ptr;
} else {
- copied_self = self;
- Py_INCREF(copied_self);
+ ptr = self->b_ptr;
+ obj = (PyObject *)self;
+ Py_INCREF(obj);
}
parg = PyCArgObject_new();
if (parg == NULL) {
- Py_DECREF(copied_self);
+ Py_DECREF(obj);
return NULL;
}
parg->tag = 'V';
- stgdict = PyObject_stgdict((PyObject *)copied_self);
+ stgdict = PyObject_stgdict((PyObject *)self);
assert(stgdict); /* Cannot be NULL for structure/union instances */
parg->pffi_type = &stgdict->ffi_type_pointer;
- parg->value.p = copied_self->b_ptr;
- parg->size = copied_self->b_size;
- parg->obj = (PyObject *)copied_self;
+ parg->value.p = ptr;
+ parg->size = self->b_size;
+ parg->obj = obj;
return parg;
}
@@ -5700,6 +5741,10 @@ PyInit__ctypes(void)
if (PyType_Ready(&DictRemover_Type) < 0)
return NULL;
+ if (PyType_Ready(&StructParam_Type) < 0) {
+ return NULL;
+ }
+
#ifdef MS_WIN32
if (create_comerror() < 0)
return NULL;
--
2.21.0