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.

Author: vstinner

Lib/ctypes/test/test_structures.py

@@ -3,7 +3,7 @@
 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 X(Structure):
              "_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 Z(Y):
         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_ = [

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.

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;