From 96b4087ce784ee7434dffdf69c475f5b40543982 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 30 Aug 2019 14:30:33 +0200 Subject: [PATCH] bpo-37140: Fix StructUnionType_paramfunc() (GH-15612) 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. --- 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 d1ea43bc7e3..fda104563d2 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 00000000000..4eaa226147f --- /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 c1941c16400..95bfe9d6348 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;