Add workaround for bpo-36880 crash to ctypes_patch

rubicon/objc/ctypes_patch.py

@@ -99,7 +99,9 @@ class ffi_type(ctypes.Structure):
 
 # The GETFUNC and SETFUNC typedefs from "Modules/_ctypes/ctypes.h".
 GETFUNC = ctypes.PYFUNCTYPE(ctypes.py_object, ctypes.c_void_p, ctypes.c_ssize_t)
-SETFUNC = ctypes.PYFUNCTYPE(ctypes.py_object, ctypes.c_void_p, ctypes.py_object, ctypes.c_ssize_t)
+# The return type of SETFUNC is declared here as a c_void_p instead of py_object to work around ctypes bug
+# bpo-36880 (https://bugs.python.org/issue36880). See the comment in make_callback_returnable's setfunc for details.
+SETFUNC = ctypes.PYFUNCTYPE(ctypes.c_void_p, ctypes.c_void_p, ctypes.py_object, ctypes.c_ssize_t)
 
 
 # The StgDictObject structure from "Modules/_ctypes/ctypes.h".
@@ -119,6 +121,10 @@ class StgDictObject(ctypes.Structure):
     ]
 
 
+ctypes.pythonapi.Py_IncRef.restype = None
+ctypes.pythonapi.Py_IncRef.argtypes = [ctypes.POINTER(PyObject)]
+
+
 def unwrap_mappingproxy(proxy):
     """Return the mapping contained in a mapping proxy object."""
 
@@ -208,7 +214,18 @@ def setfunc(ptr, value, size):
             )
 
         ctypes.memmove(ptr, ctypes.addressof(value), actual_size)
-        return None
+        # Because of ctypes bug bpo-36880 (https://bugs.python.org/issue36880), returning None from a callback with
+        # restype py_object causes a reference counting error that can crash Python.
+        # To work around this bug, the restype of SETFUNC is declared as c_void_p instead.
+        # This way ctypes performs no automatic reference counting for the returned object, which avoids the bug.
+        # However, this way we have to manually convert the Python object to a pointer and adjust its reference count.
+        none_ptr = ctypes.cast(id(None), ctypes.POINTER(PyObject))
+        # The return value of a SETFUNC is expected to have an extra reference
+        # (which will be owned by the caller of the SETFUNC).
+        ctypes.pythonapi.Py_IncRef(none_ptr)
+        # The returned pointer must be returned as a plain int, not as a c_void_p,
+        # otherwise ctypes won't recognize it and will raise a TypeError.
+        return ctypes.cast(none_ptr, ctypes.c_void_p).value
 
     # Store the getfunc and setfunc as attributes on the ctype, so they don't get garbage-collected.
     ctype._rubicon_objc_ctypes_patch_getfunc = getfunc

tests/test_ctypes_patch.py

@@ -0,0 +1,123 @@
+import ctypes
+import unittest
+
+from rubicon.objc import ctypes_patch
+
+
+class CtypesPatchTest(unittest.TestCase):
+    def test_patch_structure(self):
+        """A custom structure can be patched successfully."""
+
+        class TestStruct(ctypes.Structure):
+            _fields_ = [
+                ("spam", ctypes.c_int),
+                ("ham", ctypes.c_double),
+            ]
+        functype = ctypes.CFUNCTYPE(TestStruct)
+
+        # Before patching, the structure cannot be returned from a callback.
+        with self.assertRaises(TypeError):
+            @functype
+            def get_struct_fail():
+                return TestStruct(123, 123)
+
+        ctypes_patch.make_callback_returnable(TestStruct)
+
+        # After patching, the structure can be returned from a callback.
+        @functype
+        def get_struct():
+            return TestStruct(123, 123)
+
+        # After being returned from the callback, the structure's data is intact.
+        struct = get_struct()
+        self.assertEqual(struct.spam, 123)
+        self.assertEqual(struct.ham, 123)
+
+    def test_patch_pointer(self):
+        """A custom pointer type can be patched successfully."""
+
+        class TestStruct(ctypes.Structure):
+            _fields_ = [
+                ("spam", ctypes.c_int),
+                ("ham", ctypes.c_double),
+            ]
+        pointertype = ctypes.POINTER(TestStruct)
+        functype = ctypes.CFUNCTYPE(pointertype)
+
+        original_struct = TestStruct(123, 123)
+
+        # Before patching, the pointer cannot be returned from a callback.
+        with self.assertRaises(TypeError):
+            @functype
+            def get_struct_fail():
+                return pointertype(original_struct)
+
+        ctypes_patch.make_callback_returnable(pointertype)
+
+        # After patching, the structure can be returned from a callback.
+        @functype
+        def get_struct():
+            return pointertype(original_struct)
+
+        # After being returned from the callback, the pointer's data is intact.
+        struct_pointer = get_struct()
+        self.assertEqual(ctypes.addressof(struct_pointer.contents), ctypes.addressof(original_struct))
+        self.assertEqual(struct_pointer.contents.spam, 123)
+        self.assertEqual(struct_pointer.contents.ham, 123)
+
+    def test_no_patch_primitives(self):
+        """Primitive types cannot be patched."""
+
+        for tp in (ctypes.c_int, ctypes.c_double, ctypes.c_char_p, ctypes.c_void_p):
+            with self.subTest(tp):
+                with self.assertRaises(ValueError):
+                    ctypes_patch.make_callback_returnable(tp)
+
+    def test_patch_idempotent(self):
+        """Patching a type multiple times is equivalent to patching once."""
+
+        class TestStruct(ctypes.Structure):
+            _fields_ = [
+                ("spam", ctypes.c_int),
+                ("ham", ctypes.c_double),
+            ]
+        functype = ctypes.CFUNCTYPE(TestStruct)
+
+        for _ in range(5):
+            ctypes_patch.make_callback_returnable(TestStruct)
+
+            # After patching, the structure can be returned from a callback.
+            @functype
+            def get_struct():
+                return TestStruct(123, 123)
+
+            # After being returned from the callback, the structure's data is intact.
+            struct = get_struct()
+            self.assertEqual(struct.spam, 123)
+            self.assertEqual(struct.ham, 123)
+
+    def test_patched_type_returned_often(self):
+        """Returning a patched type very often works properly without crashing anything.
+
+        This checks that bpo-36880 is either fixed or worked around.
+        """
+
+        class TestStruct(ctypes.Structure):
+            _fields_ = [
+                ("spam", ctypes.c_int),
+                ("ham", ctypes.c_double),
+            ]
+        functype = ctypes.CFUNCTYPE(TestStruct)
+
+        ctypes_patch.make_callback_returnable(TestStruct)
+
+        # After patching, the structure can be returned from a callback.
+        @functype
+        def get_struct():
+            return TestStruct(123, 123)
+
+        for _ in range(10000):
+            # After being returned from the callback, the structure's data is intact.
+            struct = get_struct()
+            self.assertEqual(struct.spam, 123)
+            self.assertEqual(struct.ham, 123)

tox.ini

@@ -24,7 +24,7 @@ setenv =
     DYLD_LIBRARY_PATH = {toxinidir}/tests/objc
 
 [testenv:flake8]
-basepython = python3.6
+basepython = python3.7
 deps =
     flake8
 commands = flake8 {posargs}