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

Brett Cannon added the comment: Thanks, I'll fix it at some point. -- ___ Python tracker ___ ___

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

SilentGhost added the comment: Brett, Misc/NEWS entry needs a # before issue number. -- nosy: +SilentGhost ___ Python tracker ___

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

Brett Cannon added the comment: Thanks for catching that, David. -- status: open -> closed ___ Python tracker ___

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

Roundup Robot added the comment: 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 -- ___ Python tracker

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

R. David Murray added the comment: 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 -- nosy: +r.david.murray status: closed ->

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

Changes by Brett Cannon : -- resolution: -> fixed stage: -> resolved status: open -> closed ___ Python tracker ___

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

Brett Cannon added the comment: 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. -- ___ Python tracker

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

Roundup Robot added the comment: 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 -- nosy: +python-dev ___ Python

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

Xiang Zhang added the comment: Thanks Brett, also others. Restore the moving around. -- Added file: http://bugs.python.org/file43723/fix_fspath_crash_v3.patch ___ Python tracker

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

Brett Cannon added the comment: 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

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

Xiang Zhang added the comment: 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

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

Ethan Furman added the comment: > 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.

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

Ethan Furman added the comment: > 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

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

Xiang Zhang added the comment: 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. -- Added file:

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

Serhiy Storchaka added the comment: 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). -- ___

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

Xiang Zhang added the comment: 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. --

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

Antti Haapala added the comment: 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? -- nosy: +ztane ___ Python tracker

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

Decorater added the comment: 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. -- ___ Python tracker

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

Decorater added the comment: This does also happen in make_zip too as it does not known about the current changes. https://bpaste.net/show/ff826604a5f0 -- nosy: +Decorater ___ Python tracker

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

Xiang Zhang added the comment: Ahh, adding tests is easy. But this seems to be just a careless omission, so maybe tests is not a must. -- ___ Python tracker

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

Serhiy Storchaka added the comment: Could you add tests? -- nosy: +serhiy.storchaka ___ Python tracker ___

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

Xiang Zhang added the comment: BTW, the code is also listed in PEP519. So maybe that also needs to be fixed. -- ___ Python tracker ___

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

New submission from Xiang Zhang: 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