os.fspath is certain to crash when exception raised in __fspath__

[zhangyangyu]

BPO	27512
Nosy	@brettcannon, @bitdancer, @ethanfurman, @serhiy-storchaka, @ztane, @zhangyangyu, @AraHaan
Files	fix_fspath_crash.patch fix_fspath_crash_v2.patch fix_fspath_crash_v3.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/brettcannon'
closed_at = <Date 2016-07-15.18:27:45.271>
created_at = <Date 2016-07-14.09:28:13.506>
labels = ['extension-modules', 'type-crash']
title = 'os.fspath is certain to crash when exception raised in __fspath__'
updated_at = <Date 2016-08-16.19:53:11.705>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2016-08-16.19:53:11.705>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2016-07-15.18:27:45.271>
closer = 'brett.cannon'
components = ['Extension Modules']
creation = <Date 2016-07-14.09:28:13.506>
creator = 'xiang.zhang'
dependencies = []
files = ['43713', '43717', '43723']
hgrepos = []
issue_num = 27512
keywords = ['patch']
message_count = 22.0
messages = ['270386', '270387', '270390', '270392', '270393', '270396', '270397', '270398', '270402', '270408', '270416', '270417', '270420', '270428', '270430', '270491', '270492', '270495', '270497', '270498', '272877', '272878']
nosy_count = 9.0
nosy_names = ['brett.cannon', 'r.david.murray', 'SilentGhost', 'ethan.furman', 'python-dev', 'serhiy.storchaka', 'ztane', 'xiang.zhang', 'Decorater']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue27512'
versions = ['Python 3.6']

[zhangyangyu]

When any exception is raised inside __fspath__, operations involving it like os.fspath, open are certain to crash.

>>> import os
>>> class Test:
...     def __fspath__(self):
...             raise RuntimeError
... 
>>> os.fspath(Test())
Segmentation fault (core dumped)

Attach a patch to fix this.

[zhangyangyu]

BTW, the code is also listed in PEP-519. So maybe that also needs to be fixed.

[serhiy-storchaka]

Could you add tests?

[zhangyangyu]

Ahh, adding tests is easy. But this seems to be just a careless omission, so maybe tests is not a must.

[AraHaan]

This does also happen in make_zip too as it does not known about the current changes. https://bpaste.net/show/ff826604a5f0

[AraHaan]

nevermind the above traceback from it is a issue with the code that makes it try to copy the redist file from the v14 C++ runtime.

[ztane]

I believe tests is that they should *especially* be in place for any previously found "careless omissions". If it has been done before, who is to say that it wouldn't happen again?

[zhangyangyu]

I think such omissions are hard to predict and you won't know where is the omission until you encounter it. So if you don't know, how do you know what to put in the tests? And if it is encountered and fixed, such errors should not happen again.

[serhiy-storchaka]

I think should be tests for following cases:

1. __fspath__ doesn't exist;
2. __fspath__ is not callable or calling __fspath__() raises an exception;
3. the result of __fspath__() is not valid (wrong type).

[zhangyangyu]

The test requests for the whole fspath are reasonable. I just don't want to add tests for these two lines.

1. and 3) have already been tested. I add 2) and reorganize the existing tests a little.

[ethanfurman]

I think such omissions are hard to predict and you won't know where is
the omission until you encounter it. So if you don't know, how do you
know what to put in the tests?

True.

And if it is encountered and fixed, such errors should not happen again.

False.

Such errors *will* happen again -- they are called regressions. In order to avoid those we add tests, and those tests serve several purposes:

• confirm the problem has been fixed (not every bug is a core-dump)
• confirm that changes in the future don't reintroduce the bug
• communicate past difficulties and possible gotchas to new developers

In short: add tests; patches are not complete without them.

[ethanfurman]

I ... reorganize the existing tests a little.

Please don't.

Moving tests around makes it harder for reviewers to see what
actually changed.

Rewriting tests to do the same thing as before also takes more
work from reviewers as we have to verify the new code does the
same as the old code, and then wonder why the change was made.

[zhangyangyu]

I have to explain for myself.

First, I know tests *are* important. This is not the first time
uploading patches here. I don't want to add tests for this case
is because such codes: call something and test for failure appears
all the places in the code base. This case is nothing special. For
this reason, I think simply add the missing code back is enough.
But then Serhiy presents his opinion that some test cases should be
added, I think it's quite reasonable because it's testing the whole
fspath behaviour, not just this case.

Second, maybe I cannot agree with your opinions in your last message.
I think changes are welcome if they *are* reasonable. The unreasonable
aspects including what you have stated. For this patch, I don't know
which part is unreasonable. Moving test_fsencode_fsdecode after
test_pathlike makes the first 3 tests test for valid arguments.
Extracting bad pathlike tests into a single test does not sounds like
a bad idea. And it's small, right? So it's not hard to figure out
what the change is made and what is actually changed.