classification
Title: assertion failure in imp.create_dynamic(), when spec.name is not a string
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, brett.cannon, eric.snow, ncoghlan, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-08-31 16:25 by Oren Milman, last changed 2017-09-19 12:54 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3257 merged Oren Milman, 2017-08-31 22:02
PR 3653 merged python-dev, 2017-09-19 11:39
Messages (8)
msg301050 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-31 16:25
The following code causes an assertion failure in get_encoded_name(), which is
called by _PyImport_LoadDynamicModuleWithSpec() (in Python/importdl.c):

import imp

class BadSpec:
    name = 42
    origin = 'foo'

imp.create_dynamic(BadSpec())


this is because _PyImport_LoadDynamicModuleWithSpec() assumes that spec.name is
a string.


should we fix this (even though imp is deprecated)?
msg301056 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-08-31 19:32
I'm about to go on vacation so I might not be right of mind to comment, but I think throwing a TypeError is valid if it's triggering an assertion error that is already there.

P.S. Thanks for all the fuzz testing you're doing, Oren!
msg301057 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-31 19:40
do you mean that we should fix it to raise a TypeError?

the assertion is there, but not explicitly. 
get_encoded_name() calls PyUnicode_FindChar(), which calls
PyUnicode_READY(), which does assert(_PyUnicode_CHECK).

so i get:
>>> import imp
>>>
>>> class BadSpec:
...     name = 42
...     origin = 'foo'
...
>>> imp.create_dynamic(BadSpec())
Assertion failed: PyUnicode_Check(op), file ..\Objects\unicodeobject.c, line 380
msg301061 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-08-31 20:45
Yes, I'm saying that instead of hitting the C-level assertion error an explicit TypeError should be raised (or some other change like calling str() on the object). Either way, a C-level assertion from valid Python code is bad. :)
msg301109 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-01 14:40
On Thu, Aug 31, 2017 at 1:32 PM, Brett Cannon <report@bugs.python.org> wrote:
> I think throwing a TypeError is valid if it's triggering an assertion error that is already there.

+1

> P.S. Thanks for all the fuzz testing you're doing, Oren!

Also a big +1. :)
msg302516 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-19 11:39
New changeset 9974e1bcf3d0cec9b38b39b39b7ec8a1ebd9ef54 by Serhiy Storchaka (Oren Milman) in branch 'master':
bpo-31315: Fix an assertion failure in imp.create_dynamic(), when spec.name is not a string. (#3257)
https://github.com/python/cpython/commit/9974e1bcf3d0cec9b38b39b39b7ec8a1ebd9ef54
msg302518 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-19 12:51
New changeset 99a51d4e5b154a7b8d971090fecc1e34769a3ca1 by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
[3.6] bpo-31315: Fix an assertion failure in imp.create_dynamic(), when spec.name is not a string. (GH-3257) (#3653)
https://github.com/python/cpython/commit/99a51d4e5b154a7b8d971090fecc1e34769a3ca1
msg302519 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-19 12:54
Thank you Oren!
History
Date User Action Args
2017-09-19 12:54:46serhiy.storchakasetmessages: + msg302519
2017-09-19 12:54:26serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: - Python 2.7
2017-09-19 12:51:24serhiy.storchakasetmessages: + msg302518
2017-09-19 11:39:56python-devsetkeywords: + patch
pull_requests: + pull_request3646
2017-09-19 11:39:50serhiy.storchakasetmessages: + msg302516
2017-09-02 09:36:01serhiy.storchakasetstage: patch review
versions: + Python 2.7, Python 3.6
2017-09-01 14:40:34eric.snowsetmessages: + msg301109
2017-08-31 22:02:03Oren Milmansetpull_requests: + pull_request3301
2017-08-31 20:45:09brett.cannonsetmessages: + msg301061
2017-08-31 19:40:44Oren Milmansetmessages: + msg301057
2017-08-31 19:32:58brett.cannonsetmessages: + msg301056
2017-08-31 17:31:42serhiy.storchakasetnosy: + brett.cannon, ncoghlan, eric.snow, serhiy.storchaka
2017-08-31 16:25:55Oren Milmancreate