Add shell command helpers to subprocess module

@rhettinger

BPO	13238
Nosy	@rhettinger, @pfmoore, @ncoghlan, @pitrou, @merwok, @alex, @bitdancer, @asvetlov, @ericsnowcurrently, @vadmium
Files	issue13238_shell_helpers.diff: Diff between shell_helper branch and default branch b267e72c8c10.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-04-09.15:57:34.015>
created_at = <Date 2011-10-21.06:00:12.522>
labels = ['type-feature', 'library']
title = 'Add shell command helpers to subprocess module'
updated_at = <Date 2015-04-09.15:57:34.014>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2015-04-09.15:57:34.014>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2015-04-09.15:57:34.015>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2011-10-21.06:00:12.522>
creator = 'ncoghlan'
dependencies = []
files = ['23542', '23694']
hgrepos = ['85']
issue_num = 13238
keywords = ['patch']
message_count = 38.0
messages = ['146061', '146062', '146064', '146065', '146069', '146077', '146085', '146148', '146150', '146153', '146154', '146155', '146156', '146210', '146211', '146299', '146345', '146364', '146367', '146555', '146556', '146567', '146570', '146589', '146603', '146609', '146705', '147291', '147292', '147686', '147708', '147739', '149216', '154332', '157250', '174017', '191069', '240346']
nosy_count = 12.0
nosy_names = ['rhettinger', 'paul.moore', 'ncoghlan', 'pitrou', 'eric.araujo', 'alex', 'r.david.murray', 'cvrebert', 'asvetlov', 'eric.snow', 'martin.panter', 'piotr.dobrogost']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue13238'
versions = ['Python 3.4']

I've been doing a few systems administration tasks with Python recently, and shell command invocation directly via the subprocess module is annoyingly clunky (even with the new convenience APIs).

Since subprocess needs to avoid the shell by default for security reasons, I suggest we add the following simple helper functions to shutil:

    def call(cmd, *args, **kwds):
        if args or kwds:
            cmd = cmd.format(*args, **kwds)
        return subprocess.call(cmd, shell=True)

    def check_call(cmd, *args, **kwds):
        if args or kwds:
            cmd = cmd.format(*args, **kwds)
        return subprocess.check_call(cmd, shell=True)

    def check_output(cmd, *args, **kwds):
        if args or kwds:
            cmd = cmd.format(*args, **kwds)
        return subprocess.check_output(cmd, shell=True)

Initially posted at:
http://code.activestate.com/recipes/577891-simple-invocation-of-shell-commands/

Of course, even if others agree in principle, documentation and tests are still needed before this change can go anywhere.

These feel like a shell injection waiting to happen to me.

And that's exactly the problem - a web developer's or security auditor's "shell injection" is a system administrator's "this language sucks".

These wrappers are the kind of thing you want for shell invocations when using Python as a replacement for a shell script or rewriting something that was originally written in Perl, but they're a terrible idea if anything you're interpolating came from an untrusted data source.

Currently, requiring "shell=True" in the arguments to the subprocess calls is considered a sufficient deterrent against people doing the wrong thing. I'm suggesting that requiring "import shutil" instead of "import subprocess" may be a similarly acceptable compromise that better serves the system administrators that choose to use Python for system automation tasks.

Perhaps a better idea would be to use different names, so it's clearer at the point of invocation that the shell is being invoked (and hence shell injection attacks are a potential concern). For example:

shell_call
check_shell_call
check_shell_output

That would make large applications easier to audit (just search for 'shell_') while still making life easier for sysadmins.

Is format() really the best choice here, considering that {}s already have a meaning in the shell?

Of the 3 available options (mod style, string.Template and str.format), yes, str.format is the best choice.

If people want the shell meaning of the braces, they can escape them by doubling them up in the command string.

Why not keeping these helpers in subprocess?

Initially, because I was suggesting the names shadow the subprocess convenience functions so they *had* to live in a different namespace.

However, even after changing the names to explicitly include "shell", I'd like to keep them away from the general subprocess functionality - these wrappers are more convenient for shell operations than the subprocess ones, but it's that very convenience that makes them potentially dangerous in larger applications that may be interpolating data that untrusted users can manipulate.

Since the intended audience is system administrators working on shell-like operations, the shell utility module seems like an appropriate place for them. Both the "import shutil" and the "shell" in the names would then serve as red flags on a code review or security audit.

Hum, in:

return_code = shellcmd.shell_call('ls -l {}', dirname)
listing = shellcmd.check_shell_output('ls -l {}', dirname)

...how do you know that dirname doesn't need some kind of escaping?
This is not only a security issue, but a bug. Even if security doesn't matter on your system, your script will still break and/or do unexpected things.

Also, I don't really understand how your recipe improves things. You're just saving one call to .format(). You would probably have the same saving by using the % operator.

It's a flow thing. This idea was kicked off by the process of translating a large Perl script to Python and paying attention to what the translation made *worse*.

One of the big things it made worse was the translation of "qx" (quoted executable) strings. In Perl, those are almost as neat and tidy as if you were writing directly in the shell:

qx|ls -l $dirname|

The thought process isn't "build this command and then execute it", it's "run this shell command".

Yes, you have to be careful that "dirname" is legal in the shell, but that usually isn't a big problem in practice, because dirname came from a previous listdir call, or you otherwise know that it's valid to interpolate it into the command (and if it isn't, then the bug lies in the way 'dirname' was populated, not in this operation).

Now, Python's never going to have implicit string interpolation, and that's fine - we have explicit interpolation instead. A direct translation of the above to idiomatic Python 2.7 code looks like the following:

subprocess.check_output("ls -l {}".format(dirname), shell=True)

Now, if I'm doing system administration tasks (like the script I was translating), then I'm going to be doing that kind of thing a *lot*. I'm also likely to be a sysadmin rather than a programmer, so rather than going "Oh, I can write a helper function for this", my natural reaction is going to be "I'm going to use a language that doesn't get in my way so much".

This proposal is aimed directly at making Python a better language for systems administration by making shell invocation almost as easy as it is in Perl:

    shutil.check_shell_output("ls -l {}", dirname)

Heck, if someone really wanted to, they could even do:

    qx = shutil.check_shell_output
    qx("ls -l {}", dirname)

However, this is also why I *don't* want these methods in subprocess - they put the burden on the user to think about their data as if they were writing shell scripts, because that data is going to get passed straight to the shell for execution without any additional escaping. That's a reasonable idea for a shell utility in shutil, it's not reasonable for a general purpose subprocess manipulation utility in subprocess.

Yes, you have to be careful that "dirname" is legal in the shell, but
that usually isn't a big problem in practice, because dirname came
from a previous listdir call, or you otherwise know that it's valid to
interpolate it into the command

I don't understand. os.listdir() doesn't escape filenames for you.

Having been bitten several times by almost-working shell commands which
would crash when one of the 10000 files being processed had a space in
it (ironically, I think that was in the Python source tree with some of
the old Mac directories), I think we really don't want to publish a
function which encourages people to pass unescaped arguments to the
shell.

That's a fair point, but I think it actually *improves* the argument for better helper functions, since we can have them automatically invoke shlex.quote() on all of the arguments:

    def _shell_format(cmd, args, kwds):
        args = map(shlex.quote, args)
        kwds = {k:shlex.quote(v) for k, v in kwds}
        return cmd.format(*args, **kwds)

    def _invoke_shell(func, cmd, args, kwds):
        return func(_shell_format(cmd, args, kwds), shell=True)

    def format_shell_call(cmd, *args, kwds):
        return _shell_format(cmd, args, kwds)

    def shell_call(cmd, *args, **kwds):
        return _invoke_shell(subprocess.call, cmds, args, kwds)

    def check_shell_call(cmd, *args, **kwds):
        return _invoke_shell(subprocess.check_call, cmds, args, kwds)

    def check_shell_output(cmd, *args, **kwds):
        return _invoke_shell(subprocess.check_output, cmds, args, kwds)

That's a fair point, but I think it actually *improves* the argument
for better helper functions

Agreed :)

[snip rationale about why shutil and not subprocess]
I’m convinced (with one nit: sh in the shutil name does not ring a security alarm for me, as I understand it as “shell-like conveniences in nice, dont-do-nasty-things-with-stings Python” :) but the shell in check_shell_call does warn).

Automatic call of shlex.quote is an argument in favor of the new helpers.

After working on the documentation updates to the subprocess module to emphasise the convenience functions, I've now changed my mind and think it makes sense to keep these additions in that module.

Now that the shell helpers will default to using shlex.quote() and require the "!u" conversion specifier to bypass that behaviour, and also will have distinct names from the ordinary non-shell based versions, my original concerns that lead to me wanting to keep them out of subprocess no longer apply. That means the general preference that subprocess be our "one stop shop" for explicit subprocess invocation can once again take precedence.

I'm actually planning to change the API a bit though, dropping "shell_format" and "shell_format_map" in favour of the following helper class:

    class ShellCommand:
        def __init__(self, command, *, **callkwds):
            self.command = command
            self.callkwds = callkwds
        def format(self, *args, **kwds):
            return _ShellFormatter().vformat(_fmt, args, kwds)
        def format_map(self, mapping):
            return _ShellFormatter().vformat(_fmt, (), mapping)
        def call(self, *args, **kwds):
            if args or kwds:
                cmd = _ShellFormatter().vformat(cmd, args, kwds)
            return subprocess.call(cmd, shell=True, **self.callkwds)
        def check_call(self, *args, **kwds):
            if args or kwds:
                cmd = _ShellFormatter().vformat(cmd, args, kwds)
            return subprocess.check_call(cmd, shell=True, **self.callkwds)
    def shell_output(cmd, *args, **kwds):
        if args or kwds:
            cmd = _ShellFormatter().vformat(cmd, args, kwds)
        data = subprocess.check_output(cmd, shell=True, universal_newlines=True)
        if data[-1:] == "\n":
            data = data[:-1]
        return data

The module level helpers would then just become re module style wrappers:

    def shell_call(command, *args, **kwds):
        return ShellCommand(command).shell_call(*args, **kwds)
    def check_shell_call(command, *args, **kwds):
        return ShellCommand(command).check_shell_call(*args, **kwds)
    def shell_output(command, *args, **kwds):
        return ShellCommand(command).shell_output(*args, **kwds)

Éric, to answer your question, with the planned version of the new API, a "*.py" value interpolated with "{!u}" should indeed pick up all of those files, since the wildcard will be passed unmodified to the underlying shell, and all shells I am aware of will accept "foo bar.py" as a match for "*.py".

   def \_\_init__(self, command, \*, \*\*callkwds):

Is the '*' marker needed?

       self.callkwds = callkwds

These aren’t used in the module-level functions. What is the use case?

If you forgive me for the nitpick, the docstrings have too much indenting.

a "*.py" value interpolated with "{!u}" should indeed pick up all of those files,
since the wildcard will be passed unmodified to the underlying shell
Great.

(This patch will also serve as a nice example of creating a string formatter. I’ve seen the PyCon video about them, but I thought I was missing a piece since I didn’t see how you hook the format function to your custom formatter; it looks like the answer is that you don’t.)

I like Nick's original idea for a handful of convenience functions but want to caution against adding a bunch of tools that start guessing at what you want. Adding automatic wildcard expansion, shell quoting/splitting and whatnot can make the module more arcane and harder to learn and remember.

Please adopt a conservation approach adding the minimal set that has been shown to be necessary. We can take these tools away if they turn out to have been a bad idea. It is much easier to add cruft than to take it away. In the itertools module, a number of tools started their life as just a recipe in the docs and only became builtin after having proved their worth and having proved their API.

Similar to what I did with walkdir, I plan to publish a shellcommand module on PyPI to give people a chance to look at the API and experiment with it without having to patch shutil in the standard library.

The only aspect I'm 100% sold on at the moment for the stdlib version is the use of a custom formatter to implement automatic invocation of shlex.quote(). It makes doing the right thing to avoid shell injection attacks (and whitespace problems in filenames) the path of least resistance: if you use shell_call/shell_check_call/shell_output and let them do the interpolation, then your call will be safe by default and the only thing you have to avoid is using an explicit conversion specifier that will bypass the quoting (i.e. '!a', '!r', '!s' or the custom '!u'). Since fear of encouraging shell injection vulnerabilities has historically been a big issue when it comes to shell invocation from the subprocess module, I think this part is mandatory. Once you do that, then '!u' is needed as a consequence, since sometimes you *will* want to pass unquoted values through in order to use standard numeric formatting options, or just to allow metacharacters to be interpreted by the shell, etc.

Interface wise, I'm still leaning towards just the _ShellFormatter class being private (due to its thread safety limitations), with ShellCommand and the three module level helpers making up the initial public API.

This isn't just speculation - by making the choice to use the *args and **kwds slots of the helper functions for string interpolation, it means they're no longer available for arguments to the Popen constructor.

In my own use case, one of the big things I want to do is invoke shell_call() with stdout and stderr redirected to a log file. Without ShellCommand exposed to let you customise the arguments to Popen, you end up having to essentially reimplement everything the helpers otherwise provide. With ShellCommand, you're able to still handle those less common use cases *without* having to reinvent the wheel on the formatting front:

    cmd = ShellCommand("cat {}", stdout=logfile, stderr=STDOUT)
    cmd.shell_call(cmd_arg)

For simple use cases where you *don't* want to tinker with the Popen args, though, the module level helpers will be the preferred option:

    data = shell_output("cat {}", cmd_arg).decode("utf-8")

The PyPI package to prototype the API details is now available:

http://pypi.python.org/pypi/shell_command
http://shell-command.readthedocs.org
https://bitbucket.org/ncoghlan/shell_command/

For a different take on this concept:
http://julialang.org/manual/running-external-programs/

Bumping the target version for this issue to 3.4.

There's a lot of active development in the shell convenience function space at the moment, including my shell_command, Vinay Sajip's sarge, Kenneth Reitz's envoy and the possibility of doing something inspired by Julia's approach to this problem.

I'm not convinced enough that shell_command gets the security trade-offs right to push for this in the two months we have left before 3.3b1 (plus I have other things I want to work on). In the meantime, "pip install shell_command" is fairly straightforward (or similar for sarge, envoy or one of the other shell helper libraries).

Even more libraries in this vein:
http://plumbum.readthedocs.org/en/latest/
http://amoffat.github.com/sh/

There's also https://pypi.python.org/pypi/sarge

One other thing I *often* want to do when scripting commands is to capture output, but provide some form of "progress" reporting (something like a dot per line of output, usually). That's annoyingly verbose with subprocess.

Having reviewed this issue, I don't see any point to leaving it open. If any additional shell command helpers are added to the stdlib, it should be an ab-initio discussion based on what exists in the field, and start on python-ideas. But given current sentiments about the stdlib, I doubt this is something we are likely to do in any case.

Navigation Menu

Repository navigation

Add shell command helpers to subprocess module #57447

13 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Add shell command helpers to subprocess module #57447

Description

Activity

ncoghlan commented on Oct 21, 2011

alex commented on Oct 21, 2011

ncoghlan commented on Oct 21, 2011

ncoghlan commented on Oct 21, 2011

cvrebert commented on Oct 21, 2011

ncoghlan commented on Oct 21, 2011

merwok commented on Oct 21, 2011

ncoghlan commented on Oct 21, 2011

pitrou commented on Oct 21, 2011

ncoghlan commented on Oct 22, 2011

pitrou commented on Oct 22, 2011

ncoghlan commented on Oct 22, 2011

pitrou commented on Oct 22, 2011

merwok commented on Oct 23, 2011

13 remaining items

ncoghlan commented on Nov 8, 2011

ncoghlan commented on Nov 8, 2011

merwok commented on Nov 15, 2011

rhettinger commented on Nov 15, 2011

ncoghlan commented on Nov 16, 2011

ncoghlan commented on Dec 11, 2011

ncoghlan commented on Feb 26, 2012

ncoghlan commented on Apr 1, 2012

cvrebert commented on Oct 28, 2012

pfmoore commented on Jun 13, 2013

bitdancer commented on Apr 9, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions