classification
Title: Incorrect use of PyObject_IsInstance
Type: crash Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: python-dev, r.david.murray, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-05-21 08:40 by serhiy.storchaka, last changed 2015-05-22 08:24 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
typecheck.patch serhiy.storchaka, 2015-05-21 08:40 review
typecheck_2.patch serhiy.storchaka, 2015-05-21 15:25 review
Messages (6)
msg243739 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-21 08:40
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 issue24102 and issue24091.
msg243752 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-21 14:02
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?
msg243754 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-21 14:57
> 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.
msg243756 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-21 15:25
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.
msg243797 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-22 01:15
Serhiy, go ahead and apply your patch.  The existing code is clearly wrong.
msg243820 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-22 08:14
New changeset bccaba8a5482 by Serhiy Storchaka in branch '2.7':
Issue #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 #24257: Fixed incorrect uses of PyObject_IsInstance().
https://hg.python.org/cpython/rev/c7b9645a6f35

New changeset a5101529a8a9 by Serhiy Storchaka in branch 'default':
Issue #24257: Fixed incorrect uses of PyObject_IsInstance().
https://hg.python.org/cpython/rev/a5101529a8a9
History
Date User Action Args
2015-05-22 08:24:25serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-05-22 08:14:05python-devsetnosy: + python-dev
messages: + msg243820
2015-05-22 01:15:15rhettingersetassignee: serhiy.storchaka
messages: + msg243797
2015-05-21 15:25:23serhiy.storchakasetfiles: + typecheck_2.patch

messages: + msg243756
2015-05-21 14:57:22rhettingersetnosy: + rhettinger
messages: + msg243754
2015-05-21 14:02:04r.david.murraysetnosy: + r.david.murray
messages: + msg243752
2015-05-21 08:40:46serhiy.storchakacreate