Incorrect use of PyObject_IsInstance

[serhiy-storchaka]

BPO	24257
Nosy	@rhettinger, @bitdancer, @serhiy-storchaka
Files	typecheck.patch typecheck_2.patch

^{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/serhiy-storchaka'
closed_at = <Date 2015-05-22.08:24:25.805>
created_at = <Date 2015-05-21.08:40:46.663>
labels = ['extension-modules', 'interpreter-core', 'type-crash']
title = 'Incorrect use of PyObject_IsInstance'
updated_at = <Date 2015-05-22.08:24:25.804>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2015-05-22.08:24:25.804>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-05-22.08:24:25.805>
closer = 'serhiy.storchaka'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2015-05-21.08:40:46.663>
creator = 'serhiy.storchaka'
dependencies = []
files = ['39450', '39453']
hgrepos = []
issue_num = 24257
keywords = ['patch']
message_count = 6.0
messages = ['243739', '243752', '243754', '243756', '243797', '243820']
nosy_count = 4.0
nosy_names = ['rhettinger', 'r.david.murray', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue24257'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[serhiy-storchaka]

PyObject_IsInstance() is used incorrectly for testing if Python object is an instance of specified builtin type before direct access to internals of object. This is not correct, because PyObject_IsInstance() checks the __class__ attribute that can be modified and even can be dynamic property. Correct way is to check static type. Proposed patch replaces PyObject_IsInstance() with PyObject_TypeCheck() if this is appropriate.

See also similar issues bpo-24102 and bpo-24091.

[bitdancer]

Is there any chance that these changes will break working code, or is it the case that if the current check passes incorrectly one will always get a segfauilt or other error?

[rhettinger]

is it the case that if the current check passes incorrectly
one will always get a segfauilt or other error?

Yes, that is the case. All four of these checks precede a reference to an structure member that depends on being an exact type or subtype. So, yes they are all necessary to prevent segfaults or other undefined behavior.

[serhiy-storchaka]

Yes, it is the case that if the current check passes incorrectly one will always get a segfauilt or other error.

Added tests for types.SimpleNamespace and sqlite3.Cursor. It is not easy to reproduce a bug for StopIterator (not sure it is reproducible), but the code looks definitely erroneous in any case.

[rhettinger]

Serhiy, go ahead and apply your patch. The existing code is clearly wrong.

[python-dev]

New changeset bccaba8a5482 by Serhiy Storchaka in branch '2.7':
Issue bpo-24257: Fixed segmentation fault in sqlite3.Row constructor with faked
https://hg.python.org/cpython/rev/bccaba8a5482

New changeset c7b9645a6f35 by Serhiy Storchaka in branch '3.4':
Issue bpo-24257: Fixed incorrect uses of PyObject_IsInstance().
https://hg.python.org/cpython/rev/c7b9645a6f35

New changeset a5101529a8a9 by Serhiy Storchaka in branch 'default':
Issue bpo-24257: Fixed incorrect uses of PyObject_IsInstance().
https://hg.python.org/cpython/rev/a5101529a8a9