[Python-checkins] bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64 (GH-168) (GH-8625)

[Victor Stinner]

https://github.com/python/cpython/commit/3243f8c1fb16b6de73f1d7a30f5d09047553bce3
commit: 3243f8c1fb16b6de73f1d7a30f5d09047553bce3
branch: 2.7
author: Victor Stinner <vstinner at redhat.com>
committer: GitHub <noreply at github.com>
date: 2018-08-02T16:47:26+02:00
summary:

bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64 (GH-168) (GH-8625)

Fixed bpo-29565: Corrected ctypes passing of large structs by value.

Added code and test to check that when a structure passed by value
is large enough to need to be passed by reference, a copy of the
original structure is passed. The callee updates the passed-in value,
and the test verifies that the caller's copy is unchanged. A similar
change was also added to the test added for bpo-20160 (that test was
passing, but the changes should guard against regressions).

(cherry picked from commit a86339b83fbd0932e0529a3c91935e997a234582)

files:
M Lib/ctypes/test/test_callbacks.py
M Lib/ctypes/test/test_structures.py
M Modules/_ctypes/_ctypes_test.c
M Modules/_ctypes/libffi_msvc/ffi.c

diff --git a/Lib/ctypes/test/test_callbacks.py b/Lib/ctypes/test/test_callbacks.py
index bf894d481c3e..db3d9e7b198b 100644
--- a/Lib/ctypes/test/test_callbacks.py
+++ b/Lib/ctypes/test/test_callbacks.py
@@ -250,6 +250,7 @@ def callback(a, b, c, d, e):
     def test_callback_large_struct(self):
         class Check: pass
 
+        # This should mirror the structure in Modules/_ctypes/_ctypes_test.c
         class X(Structure):
             _fields_ = [
                 ('first', c_ulong),
@@ -261,6 +262,11 @@ def callback(check, s):
             check.first = s.first
             check.second = s.second
             check.third = s.third
+            # See issue #29565.
+            # The structure should be passed by value, so
+            # any changes to it should not be reflected in
+            # the value passed
+            s.first = s.second = s.third = 0x0badf00d
 
         check = Check()
         s = X()
@@ -281,6 +287,11 @@ def callback(check, s):
         self.assertEqual(check.first, 0xdeadbeef)
         self.assertEqual(check.second, 0xcafebabe)
         self.assertEqual(check.third, 0x0bad1dea)
+        # See issue #29565.
+        # Ensure that the original struct is unchanged.
+        self.assertEqual(s.first, check.first)
+        self.assertEqual(s.second, check.second)
+        self.assertEqual(s.third, check.third)
 
 ################################################################
 
diff --git a/Lib/ctypes/test/test_structures.py b/Lib/ctypes/test/test_structures.py
index 5650189f8a89..9a863c94dafc 100644
--- a/Lib/ctypes/test/test_structures.py
+++ b/Lib/ctypes/test/test_structures.py
@@ -3,6 +3,7 @@
 from ctypes.test import need_symbol
 from struct import calcsize
 import _testcapi
+import _ctypes_test
 
 class SubclassesTest(unittest.TestCase):
     def test_subclass(self):
@@ -401,6 +402,28 @@ class Z(Y):
                          (1, 0, 0, 0, 0, 0))
         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):
+            _fields_ = [
+                ('first', c_ulong),
+                ('second', c_ulong),
+                ('third', c_ulong),
+            ]
+
+        s = X()
+        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.restype = None
+        func(s)
+        self.assertEqual(s.first, 0xdeadbeef)
+        self.assertEqual(s.second, 0xcafebabe)
+        self.assertEqual(s.third, 0x0bad1dea)
+
 class PointerMemberTestCase(unittest.TestCase):
 
     def test(self):
diff --git a/Modules/_ctypes/_ctypes_test.c b/Modules/_ctypes/_ctypes_test.c
index f295c6fc4c69..94678f3189ce 100644
--- a/Modules/_ctypes/_ctypes_test.c
+++ b/Modules/_ctypes/_ctypes_test.c
@@ -52,6 +52,19 @@ _testfunc_cbk_large_struct(Test in, void (*func)(Test))
     func(in);
 }
 
+/*
+ * See issue 29565. Update a structure passed by value;
+ * the caller should not see any change.
+ */
+
+EXPORT(void)
+_testfunc_large_struct_update_value(Test in)
+{
+    in.first = 0x0badf00d;
+    in.second = 0x0badf00d;
+    in.third = 0x0badf00d;
+}
+
 EXPORT(void)testfunc_array(int values[4])
 {
     printf("testfunc_array %d %d %d %d\n",
diff --git a/Modules/_ctypes/libffi_msvc/ffi.c b/Modules/_ctypes/libffi_msvc/ffi.c
index 515d802fd894..75dada56133d 100644
--- a/Modules/_ctypes/libffi_msvc/ffi.c
+++ b/Modules/_ctypes/libffi_msvc/ffi.c
@@ -220,6 +220,16 @@ ffi_call(/*@dependent@*/ ffi_cif *cif,
       break;
 #else
     case FFI_SYSV:
+      /* If a single argument takes more than 8 bytes,
+         then a copy is passed by reference. */
+      for (unsigned i = 0; i < cif->nargs; i++) {
+          size_t z = cif->arg_types[i]->size;
+          if (z > 8) {
+              void *temp = alloca(z);
+              memcpy(temp, avalue[i], z);
+              avalue[i] = temp;
+          }
+      }
       /*@-usedef@*/
       return ffi_call_AMD64(ffi_prep_args, &ecif, cif->bytes,
 			   cif->flags, ecif.rvalue, fn);