classification
Title: a SystemError and a crash in PyCData_setstate() when __dict__ is bad
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, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-08-30 19:29 by Oren Milman, last changed 2017-09-27 06:40 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3254 merged Oren Milman, 2017-08-31 15:14
PR 3255 merged Oren Milman, 2017-08-31 15:57
PR 3743 merged python-dev, 2017-09-25 08:09
PR 3781 merged serhiy.storchaka, 2017-09-27 05:59
Messages (10)
msg301034 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-30 19:29
The following code causes PyCData_setstate() (in Modules/_ctypes/_ctypes.c) to
raise a SystemError:

import ctypes
class BadStruct(ctypes.Structure):
    def __dict__(self):
        pass

BadStruct().__setstate__({}, b'foo')

this is because PyCData_setstate() assumes that the __dict__ attribute is a dict.


while we are here, I wonder whether we should change the format given to 
PyArg_ParseTuple() to "!Os#", so that the following would raise a TypeError:

import ctypes
class MyStruct(ctypes.Structure):
    pass

MyStruct().__setstate__(42, b'foo')

AttributeError: 'int' object has no attribute 'keys'


what do you think?
msg301035 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-30 19:31
typo - change the format to "O!s#"
msg301036 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-30 20:58
And the following example results in segmentation fault.

import ctypes
class BadStruct(ctypes.Structure):
    @property
    def __dict__(self):
        raise AttributeError

BadStruct().__setstate__({}, b'')

As for change the format given to PyArg_ParseTuple(), I think it can be done only in master. It is better to make a separate PR for this.
msg301807 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-09-10 10:34
just in case it was missed - I have opened two PRs for this issue.
msg302840 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-24 09:21
New changeset 4facdf523aa6967487a9425f124da9661b59fd43 by Serhiy Storchaka (Oren Milman) in branch 'master':
bpo-31311: Impove error reporting in case the first argument to PyCData_setstate() isn't a dictionary. (#3255)
https://github.com/python/cpython/commit/4facdf523aa6967487a9425f124da9661b59fd43
msg302841 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-24 09:49
I thought about this issue so long because I can't find good cause for __dict__ to be not a dict. Defining the __dict__ method or property doesn't affect instance dictionary and attributes lookup. It just breaks the __setstate__ method. Without having good example of overriding __dict__ I have no good test cases and don't know what a way of handling this error is better.

There is yet one disadvantage of the current implementation. The instance's dict can be lazy. It can be created only on demand, when instance's attribute is set or the __dict__ attribute is read. PyObject_GetAttrString(myself, "__dict__") creates it if it was not created. It would be more efficient to use _PyObject_GetDictPtr(). But this is a separate issue, 3.7 only.
msg302914 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-09-25 07:20
> But this is a separate issue, 3.7 only.
I don't think i understand what this issue would include.

Anyway, i updated the PR according to your comments.
msg302919 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-25 08:09
New changeset 57c2561c8c5663aef55b00e3f29cba575ff36ccd by Serhiy Storchaka (Oren Milman) in branch 'master':
bpo-31311: Fix a SystemError and a crash in ctypes._CData.__setstate__(), in case of a bad __dict__. (#3254)
https://github.com/python/cpython/commit/57c2561c8c5663aef55b00e3f29cba575ff36ccd
msg302928 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-25 08:49
New changeset a6bddb8e4397df30926eb1ade6420a2277174227 by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
[3.6] bpo-31311: Fix a SystemError and a crash in ctypes._CData.__setstate__(), in case of a bad __dict__. (GH-3254) (#3743)
https://github.com/python/cpython/commit/a6bddb8e4397df30926eb1ade6420a2277174227
msg303099 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-27 06:24
New changeset 81691b05484a68df9cebb1310dd7dcabb3c6eb0a by Serhiy Storchaka in branch '2.7':
[2.7] bpo-31311: Fix a SystemError and a crash in ctypes._CData.__setstate__(), in case of a bad __dict__. (GH-3254). (#3781)
https://github.com/python/cpython/commit/81691b05484a68df9cebb1310dd7dcabb3c6eb0a
History
Date User Action Args
2017-09-27 06:40:21serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 2.7, Python 3.6
2017-09-27 06:24:41serhiy.storchakasetmessages: + msg303099
2017-09-27 05:59:18serhiy.storchakasetpull_requests: + pull_request3768
2017-09-25 08:49:10serhiy.storchakasetmessages: + msg302928
2017-09-25 08:09:20python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request3730
2017-09-25 08:09:13serhiy.storchakasetmessages: + msg302919
2017-09-25 07:20:48Oren Milmansetmessages: + msg302914
2017-09-24 09:49:38serhiy.storchakasetmessages: + msg302841
2017-09-24 09:21:44serhiy.storchakasetmessages: + msg302840
2017-09-10 10:34:24Oren Milmansetmessages: + msg301807
2017-08-31 16:23:30Oren Milmansetcomponents: + ctypes, - Extension Modules
2017-08-31 15:57:38Oren Milmansetpull_requests: + pull_request3299
2017-08-31 15:14:06Oren Milmansetpull_requests: + pull_request3298
2017-08-31 13:04:55Oren Milmansettitle: SystemError raised by PyCData_setstate() in case __dict__ is not a dict -> a SystemError and a crash in PyCData_setstate() when __dict__ is bad
2017-08-30 20:58:44serhiy.storchakasettype: behavior -> crash

messages: + msg301036
nosy: + serhiy.storchaka
2017-08-30 19:31:55Oren Milmansetmessages: + msg301035
2017-08-30 19:29:41Oren Milmancreate