zipimport.zipimporter.get_data() crashes when path.replace() returns a non-str

[orenmn]

BPO	31291
Nosy	@Yhg1s, @brettcannon, @serhiy-storchaka, @orenmn
PRs	bpo-31291: fix an assertion failure in zipimport.zipimporter.get_data() #3226 [3.6] bpo-31291: Fixed an assertion failure in zipimport.zipimporter.… #3243

^{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 = None
closed_at = <Date 2017-08-30.11:09:04.820>
created_at = <Date 2017-08-28.08:44:26.426>
labels = ['extension-modules', '3.7', 'type-crash']
title = 'zipimport.zipimporter.get_data() crashes when path.replace() returns a non-str'
updated_at = <Date 2017-08-30.11:09:04.819>
user = 'https://github.com/orenmn'

bugs.python.org fields:

activity = <Date 2017-08-30.11:09:04.819>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2017-08-30.11:09:04.820>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2017-08-28.08:44:26.426>
creator = 'Oren Milman'
dependencies = []
files = []
hgrepos = []
issue_num = 31291
keywords = []
message_count = 9.0
messages = ['300944', '300962', '300963', '300965', '300966', '300967', '300972', '300995', '301021']
nosy_count = 4.0
nosy_names = ['twouters', 'brett.cannon', 'serhiy.storchaka', 'Oren Milman']
pr_nums = ['3226', '3243']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue31291'
versions = ['Python 3.6', 'Python 3.7']

[orenmn]

on Windows, assuming the file 'foo.zip' exists, the following would cause an
assertion failure in Modules/zipimport.c in zipimport_zipimporter_get_data_impl():

import zipimport

class BadStr(str):
    def replace(self, old, new):
        return 42

zipimport.zipimporter('foo.zip').get_data(BadStr('bar'))

this is because zipimport_zipimporter_get_data_impl() assumes that
BadStr('bar').replace('/', '\\') is a string.

[brettcannon]

I don't think this is a bad thing. This is duck-typing at work to be flexible where the type doesn't really matter, just the provided interface. If someone happens to provide an object that doesn't do the right thing then that's their fault for using the wrong object and the resulting exception that will occur from a bad path is acceptable.

I appreciate the work to do a PR, Oren, but I'm closing this issue as "rejected". (And if it makes you feel any better, you helped find a bug in the Bedevere bot. :) .

[orenmn]

I understand that our goal is to make Python better, not to make me happier :)

anyway, I haven't checked, but I am quite sure that similar code might
crash the interpreter on a release build of Python.
(just wanted to clarify that, as you used the term 'exception'.)

[brettcannon]

If it will crash the interpreter then that's different. If that turns out to be the case then please come back here and we can rework the issue and PR to prevent that by being better about checking return values or resulting exception cases.

[orenmn]

just checked, and indeed on my Windows 10 the original code I posted here
crashes the interpreter.

The patch in the PR undermines duck-typing, and that's why I added a comment
there, stating I wasn't sure about the patch.
an alternate solution would be to simply check whether the return value of
pathname.replace('/', '\') is a str.

do you think I would update the PR to do that?

[brettcannon]

Could you paste in what the crash looks like? E.g. is it a segfault, SystemError, etc.?

And you can tweak the PR or just open a new one if you would rather that fixes the crash itself without taking away duck typing.

[serhiy-storchaka]

Oren already fixed several similar problems and he is aware of difference between a crash and an exception.

The problem is Windows specific. The current code uses Unicode C API with the result of calling the replace method. This can cause a segfault (or may be an assertion failure in debug build).

The proposed solution LGTM. It undermines duck-typing, but I think this doesn't break any existing code. The duck-typing was not intentional and is Windows-only.