Issue 20160: broken ctypes calling convention on MSVC / 64-bit Windows (large structs)

Messages (44) msg207520 - (view) Author: Mark Dickinson (mark.dickinson) * Date: 2014-01-07 09:37

The argument-passing code for passing structs larger than 8 bytes is broken on 64-bit Windows, leading to potential segmentation faults or other unpredictable behaviour.  According to 

   http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx

structs not of size 1, 2, 4 or 8 bytes should be passed by pointer.  ctypes instead puts sizeof(struct) bytes onto the stack.  The offending code is in ffi_prep_args in /Modules/_ctypes/libffi_msvc/ffi.c, which apparently hasn't been kept up to date with the /Modules/_ctypes/libffi/src/x86/ffi.c.  The latter module works correctly: it has an extra #ifdef X86_WIN64 block (shown below) to take care of structs not of size 1, 2, 4 or 8. That block is missing in the libffi_msvc version.

      z = (*p_arg)->size;
#ifdef X86_WIN64
      if (z > sizeof(ffi_arg)
          || ((*p_arg)->type == FFI_TYPE_STRUCT
              && (z != 1 && z != 2 && z != 4 && z != 8))
#if FFI_TYPE_DOUBLE != FFI_TYPE_LONGDOUBLE
          || ((*p_arg)->type == FFI_TYPE_LONGDOUBLE)
#endif
          )
        {
          z = sizeof(ffi_arg);
          *(void **)argp = *p_argv;
        }
      else if ((*p_arg)->type == FFI_TYPE_FLOAT)
        {
          memcpy(argp, *p_argv, z);
        }
      else
#endif


It looks to me as though issue 17310 may be related.

Credit for this discovery should go to Freek Mank.

msg207523 - (view) Author: Larry Hastings (larry) * Date: 2014-01-07 10:39

For the record, I'd be very happy to accept a patch for this into 3.4 at any time.

msg218507 - (view) Author: mattip (mattip) * Date: 2014-05-14 08:30

Here is a fix for arguments and return values, based on the support in cffi, for python2.7
I added a test in test_win32, removed the too-early attempt to fix in callproc.c, and merged in most of the changes in lib_msvc from cffi's code base.

These changes handle the following (in addition to callproc change):
- fix rtype handling by converting a return-by-value struct to a pointer-sized int
- fix stack byte-size calculation by checking for large pass-by-value structs, incrementing bytes by sizeof(void*) in this case
- fix avalue copying by checking for large pass-by-value structs and copying the pointer not the value
- fix bogus check for stack buffer < 40 bytes

A similar patch for 3.4 will be posted soon.

Other related issues: http://bugs.python.org/issue11835

msg218508 - (view) Author: mattip (mattip) * Date: 2014-05-14 08:32

and here is the promised patch for tip

msg230573 - (view) Author: Steve Dower (steve.dower) * Date: 2014-11-04 04:25

Patch looks good to me, but given this issue, #11835, #22733, and probably more, should we be integrating from libffi (which apparently has fixes for some of these) more often? I know nothing about how we move code between that and ctypes.

msg230659 - (view) Author: Steve Dower (steve.dower) * Date: 2014-11-05 05:44

Had a look at where libffi is at, and we're such a long way from them now that I don't think we can easily merge (a.k.a. I don't want to be the one to do it :) )

I want to run this patch through some builds with the right compilers, but then I'll check it in.

msg230722 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-06 03:31

New changeset f75b0470168b by Steve Dower in branch '2.7':
Issue #20160: broken ctypes calling convention on MSVC / 64-bit Windows (large structs). Patch by mattip
https://hg.python.org/cpython/rev/f75b0470168b

msg230723 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-06 03:31

New changeset cd36ba22602d by Steve Dower in branch '3.4':
Issue #20160: broken ctypes calling convention on MSVC / 64-bit Windows (large structs) Patch by mattip
https://hg.python.org/cpython/rev/cd36ba22602d

New changeset b701eb69260d by Steve Dower in branch 'default':
Issue #20160: broken ctypes calling convention on MSVC / 64-bit Windows (large structs) Patch by mattip
https://hg.python.org/cpython/rev/b701eb69260d

msg230724 - (view) Author: Steve Dower (steve.dower) * Date: 2014-11-06 03:32

Done. Thanks mattip!

msg230753 - (view) Author: Matthias Klose (doko) * Date: 2014-11-06 16:48

steve, please can we keep this issue open until this is forwarded and accepted upstream?

msg230763 - (view) Author: Steve Dower (steve.dower) * Date: 2014-11-06 20:38

Upstream as in libffi? I'm fairly sure they've fixed it already, but their current code bears little relation to what we have, so it's not easy to tell and I didn't look all that closely.

Who's doing the forwarding?

msg233746 - (view) Author: Robert Kuska (rkuska) * Date: 2015-01-09 11:57

This commit (probably) breaks aarch64 python build.

See https://bugzilla.redhat.com/show_bug.cgi?id=1174037

Build was done with libffi 3.1.6, I have also tried with latest upstream libffi version with same result.

(gdb) b ReturnRect
Function "ReturnRect" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (ReturnRect) pending.
(gdb) run test_win32.py
Starting program: /usr/bin/python test_win32.py
Missing separate debuginfos, use: debuginfo-install glibc-2.20.90-12.fc22.aarch64
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Breakpoint 1, ReturnRect (i=0, ar=..., br=0x59b750, cp=..., dr=..., er=0x59b750, fp=..., 
    gr=<error reading variable: Cannot access memory at address 0xffff000000000000>)
    at /usr/src/debug/Python-2.7.9/Modules/_ctypes/_ctypes_test.c:552
552	    if (ar.left + br->left + dr.left + er->left + gr.left != left * 5)
(gdb) p fp
$1 = {x = 4396722194992, y = 5879632}
(gdb) p cp
$2 = {x = 15, y = 25}
(gdb)

msg233754 - (view) Author: Steve Dower (steve.dower) * Date: 2015-01-09 14:06

This change only has an effect of you're building with Visual Studio and our fork of libffi. You seem to have an unrelated issue.

msg234079 - (view) Author: Robert Kuska (rkuska) * Date: 2015-01-15 15:28

FYI The bug was found in libffi. I have tried and rebuilt python also with *bundled libffi* with the *same result* (=test failure). There is more info in the bugzilla mentioned in my previous post along with the libffi patch.

msg234081 - (view) Author: Steve Dower (steve.dower) * Date: 2015-01-15 16:02

You're running on Fedora, not Windows, so this is not the right issue. You should open a new one and probably post on python-dev to get some attention (since there's no useful nosy lists right now and ctypes has no maintainer).

msg234093 - (view) Author: mattip (mattip) * Date: 2015-01-15 18:35

it seems the problem is that the bundled libffi version in Modules/_ctypes/libffi needs updating. The fix for aarch64 is a simple one-liner (tested on 2.7 in a aarch64 vm), for both HEAD and 2.7 (attached), but perhaps a better fix would be to update the bundled libffi?

msg234120 - (view) Author: Robert Kuska (rkuska) * Date: 2015-01-16 09:44

I have created #23249.

msg239117 - (view) Author: Bob (Bob) Date: 2015-03-24 12:02

I was looking into http://lists.cs.uiuc.edu/pipermail/llvmbugs/2012-September/025152.html 'Use of libclang python bindings on Windows 7 64 bit fails with memory access violation'

It appears to be 90% fixed with this patch, but I believe there is still a problem when structs are passed back though a callback function.

Could this be the correct addition to fix it?
in ffi_prep_incoming_args_SYSV() _ctypes\libffi_msvc\ffi.c(line 377)

      /* because we're little endian, this is what it turns into.   */

+#if _WIN64
+      if ((*p_arg)->type == FFI_TYPE_STRUCT && z > 8)
+          {
+          z = 8;
+          *p_argv = *(void**)argp;
+          }
+      else
+          {
+          *p_argv = (void*)argp;
+          }
+#else
      *p_argv = (void*)argp;
+#endif

msg239146 - (view) Author: Steve Dower (steve.dower) * Date: 2015-03-24 15:53

It could be, though I admit I don't know ctypes or libffi that well.

Should be easy enough to set up a repro for this (we'll add a function to _ctypes_test eventually that calls back and passes a struct) - I'll get to it sooner or later, but if someone else gets there first that'd be great.

msg239164 - (view) Author: mattip (mattip) * Date: 2015-03-24 18:08

"Bob" is relating to a dead issue, the email was from 2012 and this issue was fixed in Nov 2014. His fix is wrong and unnecessary, which he would have discovered had he run the tests that were added with that patch.

This issue was only left open in order to allow someone to resync libffi with the upstream version

In the mean time this is the second time someone has mistakenly related to this issue, so could someone with super powers please close the issue. If needed (a failing test would be nice) then we could open a new issue for "sync libffi with upstream"

msg239174 - (view) Author: Bob (Bob) Date: 2015-03-24 20:11

ok, forgive me for asking a newbe question ot two, but when you say 'fixed in nov 2014' are you refering to the release mentioned below or something else, possibly something that didn't make it into 2.7.9?  

I dont really understand what 'resync libffi with the upstream version' means, but I assume that if this is done my python-clang issue might be fixed?  So having a test is important, I'll see what I can do.

I'm working with the 2.7.9 release source, because I need a fix that can be dropped into the 2.7.9 official release folder so that people here can use python-clang. The source I have for 2.7.9 does appear to have the release below in it, including the changes to test_win32.py (which still passes with my fix)

msg239180 - (view) Author: mattip (mattip) * Date: 2015-03-24 20:55

This is the workflow for developing a patch for python
https://docs.python.org/devguide
This is the workflow for adding a patch to an issue
https://docs.python.org/devguide/patch.html

If there is an issue with python, please demonstrate it in a way that we can follow, and when you submit a patch please upload it in a way that we can see what version of the original file you are changing.

libffi is a library used in python to allow foreign function interfaces in python. It lives here https://github.com/atgreen/libffi The code from libffi was imported into python at version 3.1, (Modules/_ctypes/libffi) but since the official compiler used for windows is msvc, a forked version of libffi, called libffi_msvc lives in parallel to the official libffi in the python source tree. Clang should be using the libffi code or a libffi provided with clang, not the libffi_msvc version since it is specific for msvc. If the libffi inside python is faulty, someone should create an issue to import a newer version from the "upstream" source at https://github.com/atgreen/libffi

In addition, you reference an email from 2012, but now relate to an issue with python 2.7.9 which was released much after that date. What is the problem you see, how does it manifest itself? You should create a separate issue for that behaviour

This issue here was specifically for using libffi with the microsoft compiler to support ctypes, and was fixed for the 2.7.9 release. A test was added in the general ctypes test directory to make sure small and large structures are correctly handled in ctypes. While the test is in a file named win32, the test is run on all platforms, thus exposing a problem with aarch64 on redhat systems in Jan 2015. Their issue was that the libffi in python, and the one used on their platform, had an issue with aarch64. But the upstream libffi does support aarch64, so again, this issue is not the place to report that failure.

In summary, f there is a problem using clang with libffi, perhaps open a new issue that clearly shows how clang+libffi+python fail to do something, it may be enough to simply not use the msvc version of libffi

msg239186 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-03-24 21:53

This https://mail.python.org/pipermail/python-dev/2014-December/137631.html seems relevant.  Follow up here https://mail.python.org/pipermail/python-dev/2015-March/138731.html

msg239249 - (view) Author: Bob (Bob) Date: 2015-03-25 12:27

What I see is that structs lager that 8 bytes are not passed correctly to a callback funtion.

I've attached a patchfile that includes my fix and a test to demonstrate the problem.

msg240643 - (view) Author: John Ehresman (jpe) * Date: 2015-04-13 16:25

I've confirmed that the test included in the 3/25/15 patch fails without the change to ffi.c.  I think the 11/5/14 change fixed the bug for calling into a C function and converting the return value, but did not address the callback case.  The new patch seems to fix the callback case.

msg270916 - (view) Author: Patrick Stewart (Patrick Stewart) Date: 2016-07-21 12:08

This is still a problem, and the suggested fix seems to work. There is another error a few lines above where it is allocating 4 bytes for a pointer. I've attached a new patch with both fixes, but without Bob's tests.
This issue is a duplicate of 17310

msg271908 - (view) Author: Patrick Stewart (Patrick Stewart) Date: 2016-08-03 14:40

There's some confusion above about clang - that's completely irrelevant, the problem was using ctypes to call into libclang, not using clang to compile anything. The problem applies to any callback function that returns a struct larger than 8 bytes with any MSVC 64bit build of ctypes.

msg271919 - (view) Author: Vinay Sajip (vinay.sajip) * Date: 2016-08-03 18:33

I've uploaded a new patch which adds a test. I've confirmed that *without* the patch to Modules/_ctypes/libffi_msvc/ffi.c, the test passes on win32 and fails on amd64. *With* the patch to Modules/_ctypes/libffi_msvc/ffi.c, the test passes on both win32 and amd64.

I'll apply this patch if I don't hear any objections or suggested improvements before Friday 2016-08-05.

msg272007 - (view) Author: Steve Dower (steve.dower) * Date: 2016-08-05 03:08

Looks good to me.

Sorry for not getting back to this after I said I would. Every time I remembered I didn't have the time available...

msg272008 - (view) Author: Steve Dower (steve.dower) * Date: 2016-08-05 03:10

I'd also suggest that this is potentially exploitable (if the Python code calling into ctypes is already vulnerable - I don't think you could reasonably manufacture an exploit simply from this bug) and it should probably go into all active security branches. I think that's what the current selection represents, but I don't remember where 3.3 is at right now.

msg272024 - (view) Author: Vinay Sajip (vinay.sajip) * Date: 2016-08-05 11:05

According to PEP 398, we should patch the source for security updates for 3.3 until September 2017, though no new binary release needs to be made. I'm not sure if expedited binary releases are needed for 3.4 and 3.5. I will look at applying the patch in 2.7 and 3.3 through to 3.6.

msg272025 - (view) Author: Larry Hastings (larry) * Date: 2016-08-05 12:24

3.4 is also in security-fixes-only mode, which also means it's in no-binary-installers mode.

Good luck making the case that "this bugfix, which took us more than 2.5 years to finalize, is so critical that the release team must immediately issue binary installers".

msg272028 - (view) Author: Steve Dower (steve.dower) * Date: 2016-08-05 13:27

Yeah, it's more a loose end than a real concern. Helps make the case for reintegrating current libffi builds, as IIRC they've had the fix for a long time, but we don't have anyone willing to do the integration work right now.

msg272029 - (view) Author: Patrick Stewart (Patrick Stewart) Date: 2016-08-05 13:40

Actually the current released version of libffi (3.2.1) is even more severely broken on win64, you can't return structs at all. (patch here https://github.com/patstew/MINGW-packages/blob/9c3910fa32c45448826a2241c3fba3bf6abf9428/mingw-w64-libffi/fix_return_size.patch). They've rewritten it all in git, so hopefully the next version will work.

msg272056 - (view) Author: Roundup Robot (python-dev) Date: 2016-08-05 20:19

New changeset 09475e6135d0 by Vinay Sajip in branch '2.7':
Issue #20160: Handled passing of large structs to callbacks correctly.
https://hg.python.org/cpython/rev/09475e6135d0

msg272059 - (view) Author: Roundup Robot (python-dev) Date: 2016-08-05 20:45

New changeset 4d33bccb59a8 by Vinay Sajip in branch '3.3':
Issue #20160: Handled passing of large structs to callbacks correctly.
https://hg.python.org/cpython/rev/4d33bccb59a8

New changeset 190ebf99bf45 by Vinay Sajip in branch '3.4':
Issue #20160: Merged fix from 3.3.
https://hg.python.org/cpython/rev/190ebf99bf45

New changeset 24b114d77ec8 by Vinay Sajip in branch '3.5':
Issue #20160: Merged fix from 3.4.
https://hg.python.org/cpython/rev/24b114d77ec8

New changeset ec9b4d93662d by Vinay Sajip in branch 'default':
Closes #20160: Merged fix from 3.5.
https://hg.python.org/cpython/rev/ec9b4d93662d

msg276554 - (view) Author: Rafał Chłodnicki (Rafał Chłodnicki) Date: 2016-09-15 13:59

Only a part of this fix was backported to 3.3 branch (in http://bugs.python.org/issue20160#msg272059). The other important part was only backported to 3.4 and up in http://bugs.python.org/issue20160#msg230723 .

Should this still be fixed?

msg276556 - (view) Author: Christian Heimes (christian.heimes) * Date: 2016-09-15 14:05

3.3 no longer receives any updates, neither security nor feature updates. 3.3.7 was the final release of 3.3.

msg276558 - (view) Author: Christian Heimes (christian.heimes) * Date: 2016-09-15 14:12

Small correction, 3.3 will get security updates until 2017. This bug is clearly not a security issue. The patch should not have landed in 3.3 in the first patch.

https://www.python.org/dev/peps/pep-0398/#id7

msg276561 - (view) Author: Vinay Sajip (vinay.sajip) * Date: 2016-09-15 14:58

> This bug is clearly not a security issue.

I'm not sure it's all that clear - the bug could cause a crash (observed in practice - not theoretical), which perhaps could be exploited. See Steve Dower's message msg272008 in this thread. That's why I added the patch in 3.3.

msg276565 - (view) Author: Steve Dower (steve.dower) * Date: 2016-09-15 15:26

It may be exploitable, but I doubt it can be exploited without providing arbitrary Python code.

Anyone still running Python 3.3 on Windows must be building it from source. If they haven't noticed crashes from this issue yet, they aren't using the feature, and if they have they've probably already applied the patch.

I think it's Georg's call.

msg276569 - (view) Author: Rafał Chłodnicki (Rafał Chłodnicki) Date: 2016-09-15 15:52

In case it makes any difference, my query was prompted by this issue being noticed in Sublime Text editor. Its author bundles Python 3.3 so the bug reproduced there and it took a while to figure out what is the problem.

Now that the Sublime Text issue was analyzed and authors are aware of the bug and patch, they will probably patch the fixes manually so it doesn't matter that much for that particular case.

Personally I think the bug is quite serious, but at the same time it is quite specific use case.

msg288169 - (view) Author: Vinay Sajip (vinay.sajip) * Date: 2017-02-20 00:16

New changeset a86339b83fbd0932e0529a3c91935e997a234582 by GitHub in branch 'master':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168)
https://github.com/python/cpython/commit/a86339b83fbd0932e0529a3c91935e997a234582

msg322962 - (view) Author: STINNER Victor (vstinner) * Date: 2018-08-02 14:47

New changeset 3243f8c1fb16b6de73f1d7a30f5d09047553bce3 by Victor Stinner in branch '2.7':
bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64 (GH-168) (GH-8625)
https://github.com/python/cpython/commit/3243f8c1fb16b6de73f1d7a30f5d09047553bce3

History Date User Action Args 2022-04-11 14:57:56adminsetgithub: 64359 2018-08-02 14:47:30vstinnersetnosy: + vstinner
messages: + msg322962
2018-08-02 14:02:57vstinnersetpull_requests: + pull_request8131 2018-02-19 04:59:53eric.araujosetpull_requests: - pull_request5529 2018-02-19 03:50:22miss-islingtonsetpull_requests: + pull_request5529 2017-02-20 00:16:35vinay.sajipsetmessages: + msg288169 2017-02-19 06:44:53vinay.sajipsetpull_requests: + pull_request132 2016-09-15 15:52:59Rafał Chłodnickisetmessages: + msg276569 2016-09-15 15:26:29steve.dowersetnosy: + georg.brandl
messages: + msg276565
2016-09-15 14:58:49vinay.sajipsetmessages: + msg276561 2016-09-15 14:12:14christian.heimessetmessages: + msg276558 2016-09-15 14:05:55christian.heimessetnosy: + christian.heimes
messages: + msg276556
2016-09-15 13:59:59Rafał Chłodnickisetnosy: + Rafał Chłodnicki
messages: + msg276554
2016-08-05 20:45:14python-devsetstatus: open -> closed
resolution: fixed
messages: + msg272059

stage: patch review -> resolved

2016-08-05 20:19:10python-devsetmessages: + msg272056 2016-08-05 13:40:03Patrick Stewartsetmessages: + msg272029 2016-08-05 13:27:57steve.dowersetmessages: + msg272028 2016-08-05 12:24:21larrysetmessages: + msg272025 2016-08-05 11:05:45vinay.sajipsetmessages: + msg272024
versions: + Python 3.3 2016-08-05 03:10:49steve.dowersetmessages: + msg272008 2016-08-05 03:08:18steve.dowersetmessages: + msg272007 2016-08-03 18:33:34vinay.sajipsetassignee: steve.dower -> vinay.sajip 2016-08-03 18:33:18vinay.sajipsetfiles: + patch-vms-1.diff
resolution: remind -> (no value)
messages: + msg271919

stage: resolved -> patch review

2016-08-03 14:40:52Patrick Stewartsetmessages: + msg271908 2016-08-03 14:27:39vinay.sajiplinkissue17310 superseder 2016-08-03 14:25:09vinay.sajipsetversions: + Python 3.6 2016-08-03 14:24:05vinay.sajipsetnosy: + vinay.sajip
2016-07-21 12:08:32Patrick Stewartsetfiles: + ffi_msvc.patch
nosy: + Patrick Stewart
messages: + msg270916 2016-02-05 07:41:00BreamoreBoysetnosy: - BreamoreBoy
2016-02-04 13:45:45Christoph Sarnowskisetnosy: + Christoph Sarnowski
2015-04-13 16:25:46jpesetnosy: + jpe
messages: + msg240643
2015-03-25 12:27:41Bobsetfiles: + large_struct_callback.patch

messages: + msg239249

2015-03-24 21:53:46BreamoreBoysetnosy: + BreamoreBoy
messages: + msg239186
2015-03-24 20:55:44mattipsetmessages: + msg239180 2015-03-24 20:11:35Bobsetmessages: + msg239174 2015-03-24 18:08:17mattipsetmessages: + msg239164 2015-03-24 15:53:02steve.dowersetmessages: + msg239146 2015-03-24 12:02:31Bobsetnosy: + Bob
messages: + msg239117
2015-01-16 09:44:33rkuskasetmessages: + msg234120 2015-01-16 03:29:47berker.peksaglinkissue11835 superseder 2015-01-15 18:35:17mattipsetfiles: + libffi.patch

messages: + msg234093

2015-01-15 16:02:47steve.dowersetmessages: + msg234081 2015-01-15 15:28:23rkuskasetmessages: + msg234079 2015-01-09 14:06:38steve.dowersetmessages: + msg233754 2015-01-09 11:57:23rkuskasetnosy: + rkuska
messages: + msg233746
2014-11-06 20:38:18steve.dowersetmessages: + msg230763 2014-11-06 16:48:48dokosetstatus: closed -> open

nosy: + doko
messages: + msg230753

resolution: fixed -> remind

2014-11-06 03:32:28steve.dowersetstatus: open -> closed
resolution: fixed
messages: + msg230724

stage: resolved

2014-11-06 03:31:27python-devsetmessages: + msg230723 2014-11-06 03:31:17python-devsetnosy: + python-dev
messages: + msg230722
2014-11-05 05:44:00steve.dowersetassignee: steve.dower
messages: + msg230659
versions: + Python 3.5, - Python 3.3 2014-11-04 04:25:04steve.dowersetnosy: + steve.dower
messages: + msg230573
2014-05-14 08:32:39mattipsetfiles: + issue_20160_tip.patch

messages: + msg218508

2014-05-14 08:30:14mattipsetfiles: + issue_20160_python2_7.patch

nosy: + mattip
messages: + msg218507

keywords: + patch

2014-01-08 15:53:50meador.ingesetnosy: + meador.inge
2014-01-07 10:39:34larrysetnosy: + larry
messages: + msg207523
2014-01-07 09:37:54mark.dickinsoncreate