[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[STINNER Victor]

STINNER Victor  added the comment:

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.

--
resolution:  -> rejected
stage: patch review -> 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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[Eryk Sun]

Eryk Sun  added the comment:

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.

--
nosy: +eryksun

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[STINNER Victor]

STINNER Victor  added the comment:

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 :-(

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[STINNER Victor]

STINNER Victor  added the comment:

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.

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[STINNER Victor]

STINNER Victor  added the comment:

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.

--

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[STINNER Victor]

Change by STINNER Victor :


--
keywords: +patch
pull_requests: +22647
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/23790

___
Python tracker 

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

[issue42648] subprocess: add a helper/parameter to catch exec() OSError exception

[STINNER Victor]

New submission from STINNER Victor :

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?

--
components: Library (Lib)
messages: 383078
nosy: gregory.p.smith, vstinner
priority: normal
severity: normal
status: open
title: subprocess: add a helper/parameter to catch exec() OSError exception
versions: Python 3.10

___
Python tracker 

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