classification
Title: subprocess: add a helper/parameter to catch exec() OSError exception
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.10
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, gregory.p.smith, vstinner
Priority: normal Keywords: patch

Created on 2020-12-15 17:05 by vstinner, last changed 2021-09-21 22:01 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 23790 closed vstinner, 2020-12-15 23:20
Messages (10)
msg383078 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 17:05
While working on on bpo-42641 "Deprecate os.popen() function", I tried to replace os.popen() with a subprocess function, to avoid relying on an external shell. Example from test_posix:

        with os.popen('id -G 2>/dev/null') as idg:
            groups = idg.read().strip()
            ret = idg.close()

os.popen() uses a shell, and so returns an non-zero exit status if the "id" program is not available:

>>> import os; os.popen('nonexistent').close()
/bin/sh: nonexistent : commande introuvable
32512

whereas the subprocess module raises an OSError:

>>> import subprocess; proc=subprocess.run('nonexistent')
FileNotFoundError: [Errno 2] No such file or directory: 'nonexistent'

It's not convenient to have to write try/except OSError when replacing os.popen() with subprocess.run().

It would be convenient to have a subprocess API which avoid the need for try/except, if possible with keeping the ability to distinguish when exec() fails and when exec() completed but waitpid() returns a non-zero exit status (child process exit code is non-zero).

This issue becomes more interesting when subprocess uses os.posix_spawn(). The subprocess module only uses os.posix_spawn() if the libc implementation is known to report exec() failure: if os.posix_spawn() raises an OSError exception if exec() fails. See subprocess._use_posix_spawn() which uses os.confstr('CS_GNU_LIBC_VERSION') to check if the glibc 2.24+ is used.

Or maybe I simply missed a very obvious API in subprocess for this problem?
msg383103 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 23:25
I wrote PR 23790 which adds exec_raise=True parameter to subprocess.Popen. exec_raise=False avoids try/except OSError.

I dislike the "exec_raise" parameter name. The function is not related to exec, but more specific to OSError. A better name may include "oserror", like "catch_oserror"?

The shutil.rmtree() has a "onerror" parameter. I don't know if it's a good inspiration for this feature.
msg383104 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-12-15 23:28
The shell was isolating the user from the exec error by always existing and turning the exec error into a status return.

>>> subprocess.run('foo', shell=True)
/bin/sh: line 1: foo: command not found
CompletedProcess(args='foo', returncode=127)
>>> subprocess.getstatusoutput('foo')
(127, '/bin/sh: line 1: foo: command not found')

For things that don't actually _need_ the output as a pipe file (such as your .read() example), I recommend using .run() like that today and accessing the .stdout and .returncode attributes of the CompletedProcess.  Or use the old getstatusoutput() API if you don't mind stderr being included.

As far as new APIs go, yet another keyword only argument to subprocess.POpen's existing huge list that tells it to turn exec failures into a particular returncode value could be added as a feature.  Is this use case compelling enough to do that?

...

As far as distinguishing failures go, I suggest making such an API be to allow the user to specify a non-negative int to use as returncode - a unique int or one that is out of range such as a negative number or large number could be used to distinguish things if the user were so inclined or even an int subclass like True or an IntEnum value.  But lets keep it a non-negative int, not an arbitrary object.  Negative ints map to signals; using those would be confusing for CalledProcessError when a check constructs one.

```
>>> subprocess.run('foo', exec_fails_via_returncode=256)
CompletedProcess(args='foo', returncode=256)
```

With a default of exec_fails_via_returncode=None being the existing exception raising behavior.

That isn't joyful to use as an API.  So I don't expect most people would use it directly.  They'd have a wrapper function.  Ie: just another variation on getstatusoutput() that allows for use of a shell or stderr inclusion to be controlled.

The shell is fast.  I'd just use the shell when replacing os.popen uses in tests.
msg383105 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-12-15 23:31
I suggest just adding a couple options to getstatusoutput instead of wrangling new to-catch-or-not-to-catch APIs if code like your test_posix example is what you want to replace.
msg383107 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 23:54
https://docs.python.org/dev/library/subprocess.html#subprocess-replacements documentation suggests to replace os.popen(cmd, "w") with Popen(cmd, stdin=PIPE): without shell=True. My problem is that the replacement does change the behavior if the command does not exist.

--

I would like to avoid a shell (shell=True) to avoid any risk of shell injection vulnerability, but also to avoid bugs caused by the usage of a shell.

For example, "*" is a joker character. "*.py" is expanded to the list of filenames ending with ".py", or left unchanged if there is no file with a name ending with ".py". It's surprising if you are not used to a shell, and you expect "*" to be passed to the final command.

There are other weird cases with a shell, like bpo-26124 "shlex.quote and pipes.quote do not quote shell keywords".

See bpo-42641 "Deprecate os.popen() function" for other examples.
msg383108 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 00:42
I updated my PR 23790 to add many usage of the new parameter, to see which kind of code would benefit of it.

--

Use case: Get the output of a command, don't fail if the command doesn't exist. Example: get "gcc --version" output.

It's common to redirect stderr to /dev/null.

The advantage of having a parameter is the ability to continue using regular subprocess methods without having to bother with the "command not found" case:

   proc.communicate()
   proc.stdout.read()
   if proc.returncode: ...
   ...

--

Tools/scripts/patchcheck.py calls subprocess.check_output() and uses "except subprocess.CalledProcessError:" to ignore any command error. But it doesn't catch "OSError" if the command doesn't exist (ex: "git" in this case).

*Maybe* exec_raise=False should even be the default behavior for functions like check_output()? But changing the default behavior is always a challenge for the backward compatibility :-(
msg383109 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-12-16 01:01
We could also be thinking too low level here.  We don't have to re-use returncode for this or do it on POpen itself.  This could be done at the `run()` API level and CompletedProcess could be given state indicating success or failure of the exec itself.

ex: Add a `capture_oserror=` arg to `run()`.

```
>>> proc = subprocess.run(['foo'], capture_oserror=True)
>>> proc
CompletedProcess(args='foo', oserror=FileNotFoundError(2, 'No such file or directory'))
>>> if proc.failure:
...   
```

Add two properties to CompletedProcess:

```
@property
def success(self):
    return bool(self.oserror is None and not self.returncode)

@property
def failure(self):
    return bool(self.oserror or self.returncode)
```

to make using that an easy one liner.

Another thing that came up recently was someone wishing CompletedProcess had a `__bool__()` method.  I rejected that as it could break existing code already testing None vs CompletedProcess via truthiness.  But such a desire should be satisfied with the above .success and .failure properties.
msg383110 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-12-16 01:11
arguably run() with check=True could've turned the OSError into a CalledProcessError [can't do that now, someone surely depends on it].  So if both are supplied, perhaps it does that instead of returning a CompletedProcess with .oserror set?

the API surface we've accumulated in subprocess over the decades is huge. :(
msg383111 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-12-16 01:14
I suggesting changing the name to indicate that only OSError exceptions are suppressed, not SubprocessError exceptions. Maybe call it no_oserror.

As to the status code to use, if you want a a common code that can't interfere, it has to be either a negative value that can't be confused with a POSIX signal value or, if non-negative, it has to be at least 2**32, since a status code in Windows can be any non-negative 32-bit value.

> The shell is fast.  I'd just use the shell when replacing os.popen 
> uses in tests.

POSIX functions os.system() and os.popen() should be avoided if there's a chance of shell injection, but if you'll be using shell=True anyway, then what's the value in rewriting the code? os.popen() already uses subprocess.Popen() with shell=True.
msg402380 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-09-21 22:01
I failed finding time to finish to design this feature. I'm not convinced that it's really needed. I abandoned my idea of deprecating os.popen(): bpo-42641. I close the issue.
History
Date User Action Args
2021-09-21 22:01:35vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg402380

stage: patch review -> resolved
2020-12-16 01:14:30eryksunsetnosy: + eryksun
messages: + msg383111
2020-12-16 01:11:29gregory.p.smithsetmessages: + msg383110
2020-12-16 01:01:15gregory.p.smithsetmessages: + msg383109
2020-12-16 00:42:52vstinnersetmessages: + msg383108
2020-12-15 23:54:44vstinnersetmessages: + msg383107
2020-12-15 23:31:13gregory.p.smithsetmessages: + msg383105
2020-12-15 23:28:09gregory.p.smithsetmessages: + msg383104
2020-12-15 23:25:01vstinnersetmessages: + msg383103
2020-12-15 23:20:46vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request22647
2020-12-15 17:05:42vstinnercreate