classification
Title: os.fspath is certain to crash when exception raised in __fspath__
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Decorater, SilentGhost, brett.cannon, ethan.furman, python-dev, r.david.murray, serhiy.storchaka, xiang.zhang, ztane
Priority: normal Keywords: patch

Created on 2016-07-14 09:28 by xiang.zhang, last changed 2016-08-16 19:53 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
fix_fspath_crash.patch xiang.zhang, 2016-07-14 09:28 review
fix_fspath_crash_v2.patch xiang.zhang, 2016-07-14 13:13 review
fix_fspath_crash_v3.patch xiang.zhang, 2016-07-14 17:43 review
Messages (22)
msg270386 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 09:28
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.
msg270387 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 09:29
BTW, the code is also listed in PEP519. So maybe that also needs to be fixed.
msg270390 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-14 10:16
Could you add tests?
msg270392 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 10:26
Ahh, adding tests is easy. But this seems to be just a careless omission, so maybe tests is not a must.
msg270393 - (view) Author: Decorater (Decorater) * Date: 2016-07-14 10:34
This does also happen in make_zip too as it does not known about the current changes. https://bpaste.net/show/ff826604a5f0
msg270396 - (view) Author: Decorater (Decorater) * Date: 2016-07-14 10:44
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.
msg270397 - (view) Author: Antti Haapala (ztane) * Date: 2016-07-14 10:45
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?
msg270398 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 10:48
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.
msg270402 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-14 11:42
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).
msg270408 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 13:13
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.
msg270416 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2016-07-14 15:31
> 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.
msg270417 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2016-07-14 15:38
> 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.
msg270420 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 16:06
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.
msg270428 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-07-14 17:30
The reordering of the tests is unnecessary. Because you both changed the code *and* moved a test you have to be very careful to notice the change, otherwise it just looks like you cut-and-pasted the code to another location. Had you left the test method where it was and just simplified the code then it would be very obvious what you changed and reviewing it would be much easier.
msg270430 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-14 17:43
Thanks Brett, also others. Restore the moving around.
msg270491 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-15 17:41
New changeset 35bf83ff0828 by Brett Cannon in branch 'default':
Issue #27512: Don't segfault when os.fspath() calls an object whose
https://hg.python.org/cpython/rev/35bf83ff0828
msg270492 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-07-15 17:51
Thanks for the patch, Xiang! Only thing I changed were braces around the `if` body in the C code and made the mock __fspath__() class raise an exception itself.
msg270495 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-07-15 18:08
Looks like the buildbots aren't happy:

http://buildbot.python.org/all/builders/s390x%20SLES%203.x/builds/1396/steps/test/logs/stdio
http://buildbot.python.org/all/builders/s390x%20Debian%203.x/builds/1374
msg270497 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-15 18:27
New changeset bb7686a555cc by Brett Cannon in branch 'default':
Fix a failing test introduced as part of issue #27512
https://hg.python.org/cpython/rev/bb7686a555cc
msg270498 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-07-15 18:27
Thanks for catching that, David.
msg272877 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-08-16 19:42
Brett, Misc/NEWS entry needs a # before issue number.
msg272878 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-16 19:53
Thanks, I'll fix it at some point.
History
Date User Action Args
2016-08-16 19:53:11brett.cannonsetmessages: + msg272878
2016-08-16 19:42:54SilentGhostsetnosy: + SilentGhost
messages: + msg272877
2016-07-15 18:27:45brett.cannonsetstatus: open -> closed

messages: + msg270498
2016-07-15 18:27:00python-devsetmessages: + msg270497
2016-07-15 18:08:58r.david.murraysetstatus: closed -> open
nosy: + r.david.murray
messages: + msg270495

2016-07-15 17:51:17brett.cannonsetstatus: open -> closed
resolution: fixed
stage: resolved
2016-07-15 17:51:07brett.cannonsetmessages: + msg270492
2016-07-15 17:41:57python-devsetnosy: + python-dev
messages: + msg270491
2016-07-14 17:43:49xiang.zhangsetfiles: + fix_fspath_crash_v3.patch

messages: + msg270430
2016-07-14 17:30:58brett.cannonsetassignee: brett.cannon
messages: + msg270428
2016-07-14 16:06:17xiang.zhangsetmessages: + msg270420
2016-07-14 15:38:54ethan.furmansetmessages: + msg270417
2016-07-14 15:31:26ethan.furmansetmessages: + msg270416
2016-07-14 13:13:12xiang.zhangsetfiles: + fix_fspath_crash_v2.patch

messages: + msg270408
2016-07-14 11:42:22serhiy.storchakasetmessages: + msg270402
2016-07-14 10:48:34xiang.zhangsetmessages: + msg270398
2016-07-14 10:45:50ztanesetnosy: + ztane
messages: + msg270397
2016-07-14 10:44:04Decoratersetmessages: + msg270396
2016-07-14 10:34:13Decoratersetnosy: + Decorater
messages: + msg270393
2016-07-14 10:26:49xiang.zhangsetmessages: + msg270392
2016-07-14 10:16:34serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg270390
2016-07-14 09:29:44xiang.zhangsetmessages: + msg270387
2016-07-14 09:28:13xiang.zhangcreate