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