msg270386 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
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) *  |
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) *  |
Date: 2016-07-14 10:16 |
Could you add tests?
|
msg270392 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-07-14 17:43 |
Thanks Brett, also others. Restore the moving around.
|
msg270491 - (view) |
Author: Roundup Robot (python-dev)  |
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) *  |
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) *  |
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)  |
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) *  |
Date: 2016-07-15 18:27 |
Thanks for catching that, David.
|
msg272877 - (view) |
Author: SilentGhost (SilentGhost) *  |
Date: 2016-08-16 19:42 |
Brett, Misc/NEWS entry needs a # before issue number.
|
msg272878 - (view) |
Author: Brett Cannon (brett.cannon) *  |
Date: 2016-08-16 19:53 |
Thanks, I'll fix it at some point.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:33 | admin | set | github: 71699 |
2016-08-16 19:53:11 | brett.cannon | set | messages:
+ msg272878 |
2016-08-16 19:42:54 | SilentGhost | set | nosy:
+ SilentGhost messages:
+ msg272877
|
2016-07-15 18:27:45 | brett.cannon | set | status: open -> closed
messages:
+ msg270498 |
2016-07-15 18:27:00 | python-dev | set | messages:
+ msg270497 |
2016-07-15 18:08:58 | r.david.murray | set | status: closed -> open nosy:
+ r.david.murray messages:
+ msg270495
|
2016-07-15 17:51:17 | brett.cannon | set | status: open -> closed resolution: fixed stage: resolved |
2016-07-15 17:51:07 | brett.cannon | set | messages:
+ msg270492 |
2016-07-15 17:41:57 | python-dev | set | nosy:
+ python-dev messages:
+ msg270491
|
2016-07-14 17:43:49 | xiang.zhang | set | files:
+ fix_fspath_crash_v3.patch
messages:
+ msg270430 |
2016-07-14 17:30:58 | brett.cannon | set | assignee: brett.cannon messages:
+ msg270428 |
2016-07-14 16:06:17 | xiang.zhang | set | messages:
+ msg270420 |
2016-07-14 15:38:54 | ethan.furman | set | messages:
+ msg270417 |
2016-07-14 15:31:26 | ethan.furman | set | messages:
+ msg270416 |
2016-07-14 13:13:12 | xiang.zhang | set | files:
+ fix_fspath_crash_v2.patch
messages:
+ msg270408 |
2016-07-14 11:42:22 | serhiy.storchaka | set | messages:
+ msg270402 |
2016-07-14 10:48:34 | xiang.zhang | set | messages:
+ msg270398 |
2016-07-14 10:45:50 | ztane | set | nosy:
+ ztane messages:
+ msg270397
|
2016-07-14 10:44:04 | Decorater | set | messages:
+ msg270396 |
2016-07-14 10:34:13 | Decorater | set | nosy:
+ Decorater messages:
+ msg270393
|
2016-07-14 10:26:49 | xiang.zhang | set | messages:
+ msg270392 |
2016-07-14 10:16:34 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg270390
|
2016-07-14 09:29:44 | xiang.zhang | set | messages:
+ msg270387 |
2016-07-14 09:28:13 | xiang.zhang | create | |