msg156470 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2012-03-21 00:07 |
The documentation for sys.exit says, "The optional argument arg can be an integer giving the exit status (defaulting to zero), or another type of object". However, the arguments that are treated as exit statuses are actually "subtypes of int". So, a bool argument is fine: $ python2.7 -c "import sys; sys.exit(False)"; echo ?0Butalongargumentisnot:? 0 But a long argument is not: ?0Butalongargumentisnot: python2.7 -c "import sys; sys.exit(long(0))"; echo $? 0 1 The latter behaviour can be surprising since functions like os.spawnv may return the exit status of the executed process as a long on some platforms, so that if you try to pass on the exit code via code = os.spawnv(...) sys.exit(code) you may get a mysterious surprise: code is 0 but exit code is 1. It would be simple to change line 1112 of pythonrun.c from if (PyInt_Check(value)) to if (PyInt_Check(value) | |
PyLong_Check(value)) (This issue is not present in Python 3 because there is no longer a distinction between int and long.) |
|
msg156481 - (view) |
Author: Mark Dickinson (mark.dickinson) *  |
Date: 2012-03-21 08:27 |
> It would be simple to change line 1112 of pythonrun.c from > > if (PyInt_Check(value)) > > to > > if (PyInt_Check(value) | |
PyLong_Check(value)) Wouldn't you also have to deal with possible errors from the PyInt_AsLong call? E.g., after sys.exit(2**64), an OverflowException would be set. I don't know if not dealing with that exception (perhaps with PyErr_Clear) before exit might cause issues, though it seems to work in practice (with the -1 value indicating an error being turned into an exit code of 255). |
|
msg156484 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2012-03-21 09:55 |
> Wouldn't you also have to deal with possible errors from the PyInt_AsLong call? Good point. But I note that Python 3 just does exitcode = (int)PyLong_AsLong(value); so maybe it's not important to do error handling here. |
|
|
msg156513 - (view) |
Author: Mark Dickinson (mark.dickinson) *  |
Date: 2012-03-21 20:19 |
> so maybe it's not important to do error handling here. Hmm, seems it's not. And dealing with OverflowError is hardly likely to be a concern in practice anyway. +1 for the suggested fix. |
|
|
msg156726 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
Date: 2012-03-25 00:28 |
Patch in first message; patch file needed. I don't know if sys.exit is actually tested. This is really 2.7 specific. |
|
|
msg210242 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2014-02-04 15:39 |
Patch attached. I added a test case to Lib/test/test_sys.py. |
|
|
msg241884 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2015-04-23 20:18 |
I am -1 on the patch. (int)PyLong_AsLong(value) can silently convert non-zero error code to zero. I would leave 2.7 as is and limit allowable values to a range supported by the platform. Note that ANSI C standard only specifies two values: EXIT_SUCCESS and EXIT_FAILURE, that may be passed to exit(). |
|
|
msg241885 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2015-04-23 20:22 |
There are _PyInt_AsInt() and _PyLong_AsInt(). |
|
|
msg241886 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2015-04-23 20:26 |
The key issue here is not to report success for nonzero values. I consider the following a bug: $ python3 Python 3.4.2 (default, Oct 30 2014, 08:51:12) [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.54)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import sys >>> sys.exit(256) $ echo $? 0 |
|
|
msg241887 - (view) |
Author: Ryan Gonzalez (refi64) * |
Date: 2015-04-23 20:29 |
Now we're getting away from the original issue. This wasn't created to handle edge cases for sys.exit; is was created to make it accept long values under Python 2. |
|
|
msg241888 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2015-04-23 20:54 |
> The key issue here is not to report success for nonzero values. This is different issue. |
|
|
msg241892 - (view) |
Author: Ethan Furman (ethan.furman) *  |
Date: 2015-04-23 21:35 |
+1 for the fix. Alexander, create a new issue for the problem of converting non-zero values to zero. |
|
|
msg241894 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2015-04-23 22:31 |
"errors should not pass silently" The "fix" makes the problem worse. Why would anyone want to pass a long integer to exit? I bet the user who discovered this problem had something like 0L or 1L coming from a lazily written C extension. |
|
|
msg241896 - (view) |
Author: Ryan Gonzalez (refi64) * |
Date: 2015-04-23 23:17 |
> > "errors should not pass silently" > > The "fix" makes the problem worse. > > Why would anyone want to pass a long integer to exit? > > I bet the user who discovered this problem had something like 0L or 1L > coming from a lazily written C extension. Or the person is using a Python code generator that makes everything a long for portability reasons! Hy does this. int and long are supposed to be indistinguishable. Quote by Guido from the time when I figured out the re module's group function doesn't take longs: ...but any place where an int is accepted but a long is not, is a bug. The language has been on a long trip to making the two types interchangeable and this seems one of the last remnants. Notice the word *interchangeable*. Also, I find that the current behavior is even more confusing. |
|
|
msg241902 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2015-04-24 00:23 |
Passing anything other than one of the os.EX_* constants to sys.exit() is a bad idea. In most cases you can get away with 0 and ±1. Anything beyond 8 bit signed range is a gamble. Passing a computed integer value is even more problematic. With the current behavior, you at least get some diagnostic when a computed long finds its way to sys.axit(). With the proposed patch these errors will pass silently. |
|
|
msg241904 - (view) |
Author: Ethan Furman (ethan.furman) *  |
Date: 2015-04-24 00:50 |
Linux is not the only O/S that Python runs on. There should be no difference between int and long. Possible doc fix being tracked in . Gareth, please ignore my comments about adding guards on the return value -- it is up to the O/S to use or adjust whatever Python returns. |
|
|
msg241945 - (view) |
Author: Ethan Furman (ethan.furman) *  |
Date: 2015-04-24 15:15 |
I previously wrote: ------------------ > Gareth, please ignore my comments about adding guards on the return value -- it is up > to the O/S to use or adjust whatever Python returns. Let me clarify that a bit: we need to protect against overflow from to ; protecting against overflow from to <O/S returncode size> is a separate issue. |
|
|
msg241947 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2015-04-24 15:30 |
> Alexander, create a new issue for the problem of converting non-zero values to zero. See #24052. |
|
|
msg268225 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2016-06-11 16:26 |
Let's not allow the perfect to be the enemy of the good here. The issue I reported is a very specific one: in Python 2.7, if you pass a long to sys.exit, then the value of the long is not used as the exit code. This is bad because functions like os.spawnv that return exit codes (that you might reasonably want to pass on to sys.exit) can return them as long. My patch only proposes to address this one issue. In order to keep the impact as small as possible, I do not propose to make any other changes, or address any other problems. But in the comments here people have brought up THREE other issues: 1. Alexander Belopolsky expresses the concern that "(int)PyLong_AsLong(value) can silently convert non-zero error code to zero." This is not a problem introduced by my patch -- the current code is: exitcode = (int)PyInt_AsLong(value) which has exactly the same problem (because PyIntObject stores its value as a long). So this concern (even if valid) is not a reason to reject my patch. 2. Ethan Furman wrote: "we need to protect against overflow from to " But again, this is not a problem introduced by my patch. The current code says: exitcode = (int)PyInt_AsLong(value); and my patch does not change this line. The possibility of this overflow is not a reason to reject my patch. 3. Alexander says, "Passing anything other than one of the os.EX_* constants to sys.exit() is a bad idea" First, this is not a problem introduced by my patch. The existing code in Python 2.7 allows you to specify other exit codes. So this problem (if it is a problem) is not a reason to reject my patch. Second, this claim is surely not right -- when a subprocess fails it often makes sense to pass on the exit code of the subprocess, whatever that is. This is exactly the use case that I mentioned in my original report (that is, passing on the exit code from os.spawnv to sys.exit). |
|
|
msg286619 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2017-02-01 12:20 |
Is there any chance of making progress on this issue? Is there anything wrong with my patch? Did I omit any relevant point in my message of 2016-06-11 16:26? It would be nice if this were not left in limbo for another four years. |
|
|
msg286629 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
Date: 2017-02-01 13:41 |
Summary of overt and implied votes: Core Devs: +1 Mark Dickenson +1 Ethan Furman ?? Serhiy Storchaka -1 Alexander Belopolsky (at first, but unclear after) Other: +1 Ryan Gonzales, quoting Guido about equivalence of ints and longs. The sys.exit docstring says "If the status is an integer, it will be used as the system exit status." In 2.7, 'integer' clearly includes longs. Does anyone know the Windows equivalent of 'echo $?'? Gareth, I think the problem is that this is a minor 2.7-only, non-security issue and therefore a low priority for most Core Devs. |
|
|
msg286630 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2017-02-01 13:52 |
In Windows, under cmd.exe, you can use %errorlevel% |
|
|
msg286632 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2017-02-01 14:04 |
Count me as +1. The patch LGTM. See also . |
|
|
msg286672 - (view) |
Author: Mark Dickinson (mark.dickinson) *  |
Date: 2017-02-01 18:19 |
The test portion of the existing patch doesn't apply cleanly to 2.7 tip. Here's an updated patch that does, with slightly expanded tests and a Misc/NEWS entry. There was discussion above about two overflow cases: C long to C int, and C int to byte, but with this patch there's now a 3rd overflow case to consider, namely Python long to C long. In this case PyInt_AsLong sets an exception and returns -1, and since we don't check for exceptions that -1 then gets used as the exit code. I'm not sure whether that's the right thing to do or not, but it does match what Python 3 currently does, and that's good enough for me. |
|
|
msg286673 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2017-02-01 18:34 |
Thanks for the revised patch, Mark. The new tests look good. |
|
|
msg286814 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2017-02-02 19:32 |
New changeset 14682d00b09a by Mark Dickinson in branch '2.7': Issue #14376: sys.exit now accepts longs as well as ints. Thanks Gareth Rees. https://hg.python.org/cpython/rev/14682d00b09a |
|
|
msg286816 - (view) |
Author: Gareth Rees (gdr@garethrees.org) *  |
Date: 2017-02-02 19:41 |
Thank you, Mark (and everyone else who helped). |
|
|
msg286824 - (view) |
Author: Roundup Robot (python-dev)  |
Date: |
New changeset 12b49507177368204085bd6e2b464f45e3b569e2 by Mark Dickinson in branch '2.7': Issue #14376: sys.exit now accepts longs as well as ints. Thanks Gareth Rees. https://github.com/python/cpython/commit/12b49507177368204085bd6e2b464f45e3b569e2 |
|
|