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

@birkenfeld

BPO	20160
Nosy	@birkenfeld, @doko42, @vsajip, @mdickinson, @vstinner, @larryhastings, @tiran, @meadori, @mattip, @zooba, @rkuska
PRs	Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. #168 [2.7] bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168) #8625
Files	issue_20160_python2_7.patch: patch against 2.7 issue_20160_tip.patch: patch against tip libffi.patch large_struct_callback.patch ffi_msvc.patch patch-vms-1.diff: Patch for callback on amd64 and test for the same.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/vsajip'
closed_at = <Date 2016-08-05.20:45:14.356>
created_at = <Date 2014-01-07.09:37:54.034>
labels = ['ctypes', 'type-crash']
title = 'broken ctypes calling convention on MSVC / 64-bit Windows (large structs)'
updated_at = <Date 2018-08-02.14:47:30.534>
user = 'https://github.com/mdickinson'

bugs.python.org fields:

activity = <Date 2018-08-02.14:47:30.534>
actor = 'vstinner'
assignee = 'vinay.sajip'
closed = True
closed_date = <Date 2016-08-05.20:45:14.356>
closer = 'python-dev'
components = ['ctypes']
creation = <Date 2014-01-07.09:37:54.034>
creator = 'mark.dickinson'
dependencies = []
files = ['35248', '35249', '37716', '38687', '43807', '43993']
hgrepos = []
issue_num = 20160
keywords = ['patch']
message_count = 44.0
messages = ['207520', '207523', '218507', '218508', '230573', '230659', '230722', '230723', '230724', '230753', '230763', '233746', '233754', '234079', '234081', '234093', '234120', '239117', '239146', '239164', '239174', '239180', '239186', '239249', '240643', '270916', '271908', '271919', '272007', '272008', '272024', '272025', '272028', '272029', '272056', '272059', '276554', '276556', '276558', '276561', '276565', '276569', '288169', '322962']
nosy_count = 17.0
nosy_names = ['georg.brandl', 'jpe', 'doko', 'vinay.sajip', 'mark.dickinson', 'vstinner', 'larry', 'christian.heimes', 'meador.inge', 'python-dev', 'mattip', 'steve.dower', 'rkuska', 'Bob', 'Christoph Sarnowski', 'Patrick Stewart', 'Rafa\xc5\x82 Ch\xc5\x82odnicki']
pr_nums = ['168', '8625']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue20160'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4', 'Python 3.5', 'Python 3.6']

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 bpo-17310 may be related.

Credit for this discovery should go to Freek Mank.

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

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

and here is the promised patch for tip

Patch looks good to me, but given this issue, bpo-11835, bpo-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.

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.

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

New changeset cd36ba22602d by Steve Dower in branch '3.4':
Issue bpo-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 bpo-20160: broken ctypes calling convention on MSVC / 64-bit Windows (large structs) Patch by mattip
https://hg.python.org/cpython/rev/b701eb69260d

Done. Thanks mattip!

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

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".

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.

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.

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

New changeset 4d33bccb59a8 by Vinay Sajip in branch '3.3':
Issue bpo-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 bpo-20160: Merged fix from 3.3.
https://hg.python.org/cpython/rev/190ebf99bf45

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

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

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?

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

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

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.

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.

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.

New changeset a86339b by GitHub in branch 'master':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168)
a86339b

New changeset 3243f8c by Victor Stinner in branch '2.7':
bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64 (GH-168) (GH-8625)
3243f8c

Navigation Menu

Repository navigation

broken ctypes calling convention on MSVC / 64-bit Windows (large structs) #64359

23 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

broken ctypes calling convention on MSVC / 64-bit Windows (large structs) #64359

Description

Activity

mdickinson commented on Jan 7, 2014

larryhastings commented on Jan 7, 2014

mattip commented on May 14, 2014

mattip commented on May 14, 2014

zooba commented on Nov 4, 2014

zooba commented on Nov 5, 2014

python-dev commented on Nov 6, 2014

python-dev commented on Nov 6, 2014

zooba commented on Nov 6, 2014

doko42 commented on Nov 6, 2014

23 remaining items

larryhastings commented on Aug 5, 2016

zooba commented on Aug 5, 2016

PatrickStewart commented on Aug 5, 2016

python-dev commented on Aug 5, 2016

python-dev commented on Aug 5, 2016

RafaChodnicki commented on Sep 15, 2016

tiran commented on Sep 15, 2016

tiran commented on Sep 15, 2016

vsajip commented on Sep 15, 2016

zooba commented on Sep 15, 2016

RafaChodnicki commented on Sep 15, 2016

vsajip commented on Feb 20, 2017

vstinner commented on Aug 2, 2018

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions