classification
Title: assertion failures in ctypes
Type: crash Stage: resolved
Components: ctypes Versions: Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, vinay.sajip, vstinner
Priority: normal Keywords: patch

Created on 2016-09-13 15:14 by Oren Milman, last changed 2017-09-28 14:33 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
testBugsOrPatches.py Oren Milman, 2016-09-13 15:14 an ugly script that verifies the bugs or the patches
issue28129_ver1.diff Oren Milman, 2016-09-13 15:15 proposed patches diff file - ver1 review
CPythonTestOutput.txt Oren Milman, 2016-09-13 15:16 test output of CPython without my patches (tested on my PC)
patchedCPythonTestOutput_ver1.txt Oren Milman, 2016-09-13 15:17 test output of CPython with my patches (tested on my PC) - ver1
Pull Requests
URL Status Linked Edit
PR 386 merged python-dev, 2017-03-01 23:57
PR 403 closed christian.heimes, 2017-03-02 22:01
PR 3799 merged vstinner, 2017-09-28 14:09
PR 3800 merged vstinner, 2017-09-28 14:11
Messages (8)
msg276288 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-09-13 15:14
------------ current state ------------
In Modules\_ctypes\_ctypes.c, there are six functions with assertions that might fail:
    1. CDataType_from_buffer
    2. CDataType_from_buffer_copy
    3. PyCPointerType_set_type
    4. PyCPointerType_from_param
    5. PyCSimpleType_from_param
    6. _validate_paramflags
The following is true for each of these functions:
    - It assumes its first argument is a subclass (or an instance of a subclass) of some abstract ctype, which means it (the first argument) has a storage dict.
    - Thus, it asserts its first argument has a storage dict.
    - However, its first argument might be some abstract ctype (and not a subclass (or an instance of a subclass) of that abstract ctype), in which case the assertion fails.

In Modules\_ctypes\cfield.c, there are two functions with assertions that might fail:
    1. PyCField_set
    2. PyCField_get
These functions are the C implementations of the __set__ and __get__ functions (respectively) of the CFeild type. Each of them asserts its instance argument is a CDataObject, which might not be true.


------------ proposed changes ------------
Replace each of these assertions with an if statement that raises an exception in case of an invalid argument.


------------ diff ------------
The proposed patches diff file is attached.


------------ tests ------------
I wrote an ugly script to verify the assertion failures on CPython without my patches, and to test the patches on CPython with my patches. The script is attached, but it would probably fail on a non-Windows machine.

I built the patched CPython for x86, and played with it a little. Everything seemed to work as usual.

In addition, I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output.
The outputs of both runs are attached.
msg288778 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-02 00:00
The fix for issue #25659 already replaced the assertions in
CDataType_from_buffer and CDataType_from_buffer_copy with if statements (my
bad for missing that issue when I opened this one).
In addition, that fix added some tests, so I also added some, and created a
pull request.

I run the test module again, and on my Windows 10, the same tests failed with
and without my patches. However, on my Ubuntu 16.04 VM, none of the tests
failed.
msg290342 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-24 23:13
New changeset 1bea762d9ec823544c530d567330a47f64d93d4f by Victor Stinner (orenmn) in branch 'master':
bpo-28129: fix ctypes crashes (#386)
https://github.com/python/cpython/commit/1bea762d9ec823544c530d567330a47f64d93d4f
msg303235 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-09-28 13:37
Shouldn't we close this issue?
msg303239 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-28 14:11
> Shouldn't we close this issue?

Oh, I forgot this issue.

Python 2.7 and 3.6 are also impacted and still accept bug fixes. I proposed backports.
msg303243 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-28 14:31
New changeset 8b83687bdf66d2d10afb78e46bcede399deaefde by Victor Stinner in branch '2.7':
bpo-28129: fix ctypes crashes (#386) (#3800)
https://github.com/python/cpython/commit/8b83687bdf66d2d10afb78e46bcede399deaefde
msg303244 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-28 14:32
New changeset 7d6ddb96b34b94c1cbdf95baa94492c48426404e by Victor Stinner in branch '3.6':
bpo-28129: fix ctypes crashes (#386) (#3799)
https://github.com/python/cpython/commit/7d6ddb96b34b94c1cbdf95baa94492c48426404e
msg303245 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-28 14:33
Ok, now the issue can be closed :-) I backported Oren Milman's fix to Python 2.7 and 3.6 as well.

Oren Milman: thank you very much for your fix!
History
Date User Action Args
2017-09-28 14:33:09vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg303245

stage: patch review -> resolved
2017-09-28 14:32:13vstinnersetmessages: + msg303244
2017-09-28 14:31:43vstinnersetmessages: + msg303243
2017-09-28 14:11:44vstinnersetmessages: + msg303239
versions: + Python 2.7, Python 3.6
2017-09-28 14:11:15vstinnersetpull_requests: + pull_request3786
2017-09-28 14:09:36vstinnersetstage: patch review
pull_requests: + pull_request3785
2017-09-28 13:37:40Oren Milmansetmessages: + msg303235
2017-03-24 23:13:47vstinnersetnosy: + vstinner
messages: + msg290342
2017-03-02 22:01:32christian.heimessetpull_requests: + pull_request335
2017-03-02 00:00:57Oren Milmansetmessages: + msg288778
2017-03-01 23:57:07python-devsetpull_requests: + pull_request322
2017-02-22 19:45:31vinay.sajipsetnosy: + vinay.sajip
2016-09-13 15:17:09Oren Milmansetfiles: + patchedCPythonTestOutput_ver1.txt
2016-09-13 15:16:33Oren Milmansetfiles: + CPythonTestOutput.txt
2016-09-13 15:15:06Oren Milmansetfiles: + issue28129_ver1.diff
keywords: + patch
2016-09-13 15:14:14Oren Milmancreate