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

[Brett Cannon]

Brett Cannon added the comment:

Thanks, I'll fix it at some point.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[SilentGhost]

SilentGhost added the comment:

Brett, Misc/NEWS entry needs a # before issue number.

--
nosy: +SilentGhost

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Brett Cannon]

Brett Cannon added the comment:

Thanks for catching that, David.

--
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[R. David Murray]

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 -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Brett Cannon]

Changes by Brett Cannon :


--
resolution:  -> fixed
stage:  -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Brett Cannon]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Brett Cannon]

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 it was and just simplified the code then it would be 
very obvious what you changed and reviewing it would be much easier.

--
assignee:  -> brett.cannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

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 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ethan Furman]

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.

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ethan Furman]

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 does the
same as the old code, and then wonder why the change was made.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

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: http://bugs.python.org/file43717/fix_fspath_crash_v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Antti Haapala]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Decorater]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Decorater]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Could you add tests?

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

Xiang Zhang added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Xiang Zhang]

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 dumped)

Attach a patch to fix this.

--
components: Extension Modules
files: fix_fspath_crash.patch
keywords: patch
messages: 270386
nosy: brett.cannon, ethan.furman, xiang.zhang
priority: normal
severity: normal
status: open
title: os.fspath is certain to crash when exception raised in __fspath__
type: crash
versions: Python 3.6
Added file: http://bugs.python.org/file43713/fix_fspath_crash.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com