Incorrect name passed as arg[0] when shell=True and executable specified

[narnie]

BPO	9265
Nosy	@ericvsmith, @merwok, @bitdancer, @briancurtin, @skrah
Files	subprocess2.py: corrected code issue9265-2.patch

^{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 2010-07-20.11:57:18.885>
created_at = <Date 2010-07-15.05:20:52.756>
labels = ['type-bug', 'library']
title = 'Incorrect name passed as arg[0] when shell=True and executable specified'
updated_at = <Date 2010-07-20.11:57:18.883>
user = 'https://bugs.python.org/narnie'

bugs.python.org fields:

activity = <Date 2010-07-20.11:57:18.883>
actor = 'skrah'
assignee = 'none'
closed = True
closed_date = <Date 2010-07-20.11:57:18.885>
closer = 'skrah'
components = ['Library (Lib)']
creation = <Date 2010-07-15.05:20:52.756>
creator = 'narnie'
dependencies = []
files = ['18010', '18027']
hgrepos = []
issue_num = 9265
keywords = []
message_count = 20.0
messages = ['110346', '110359', '110360', '110361', '110363', '110364', '110365', '110371', '110372', '110375', '110377', '110380', '110434', '110676', '110678', '110747', '110758', '110763', '110852', '110890']
nosy_count = 6.0
nosy_names = ['eric.smith', 'eric.araujo', 'r.david.murray', 'brian.curtin', 'skrah', 'narnie']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue9265'
versions = ['Python 2.6', 'Python 3.1', 'Python 2.7']

[narnie]

Popen from the subprocess module when called with shell=True and executable=/bin/bash still runs in the /bin/sh shell.

The error has been found and corrected in the subprocess module and is as follows:

change:
lines 1018-1022 for python v 2.6
lines 1092-1096 for python v 2.7
lines 1046-1050 for python v 3.1
from this:

        if shell:
            args = ["/bin/sh", "-c"] + args

            if executable is None:
                executable = args[0]

    to this:
    
        if shell:
        if executable is None:
            args = ["/bin/sh", "-c"] + args
            executable = args[0]
        else:
            args = [executable, "-c"] + args

# if executable is None:
# executable = args[0]

Of course the last 2 lines can just be deleted but were left in to better highlight the change.

This change corrects the bug allowing shell=True and executable=/bin/bash to run in a bash shell or any other shell sub'd for /bin/bash.

Hope this will make it into the module.

I've included my temp fix which I have simply called subprocess2 for my purposes for v 2.6.

[merwok]

Thank you for the report. The docs indeed say “If shell=True, the executable argument specifies which shell to use”.

Could you provide your change as a diff file following the guidelines on http://www.python.org/dev/patches/ ? Diffs are easier to review than whole files. A unit test is needed too, to make sure the bug is fixed, would you like to write a patch for test_subprocess.py?

Thanks again!

[skrah]

I think this is invalid. Please run:

>>> from subprocess import Popen
>>> Popen("echo $SHELL", executable="/bin/bash", shell=True)
/bin/bash
<subprocess.Popen object at 0x7f7beb13f150>
>>>

[merwok]

The SHELL variable is not to be trusted, since e.g. dash (my system’s /bin/sh) in a bash will have SHELL=bash. I tested this command: "help set >/dev/null && echo ok". Bash prints ok but dash gives an error. This test worked for versions 2.4 up to 3.2.

Narnie, perhaps you have a /bin/sh configuration problem?

Is there a test for this in test_subprocess? If not, we should add one.

[bitdancer]

From looking at the code, it appears that what happens currently is that executable is used to run /bin/sh when shell=True. So I'm pretty sure there is a bug here. What isn't clear is whether or not fixing this bug will cause currently working code to break (that is, we may not be able to backport the fix.)

The patch as given in the message has at minimum some formatting problems, but in any case is not complete. Later logic in the subprocess code uses 'executable' to run the 'args' string, which means that with this fix you'd be using executable to execute executable. This might be benign in most cases, but it is certainly wrong and could lead to subtle bugs.

There should also be some doc updates as the current docs implicitly assume that '/bin/sh' is the shell when discussing shell=True cases. This assumption should be made explicit.

[skrah]

Isn't just the name of the executable wrong? /bin/bash is
executed all right, but the name is set to "/bin/sh".

Index: Lib/subprocess.py
===================================================================

--- Lib/subprocess.py   (revision 82816)
+++ Lib/subprocess.py   (working copy)
@@ -1091,6 +1091,8 @@
 
             if shell:
                 args = ["/bin/sh", "-c"] + args
+                if executable:
+                    args[0] = executable

In general though, I'd prefer to specify another shell by shell="bin/bash".

[bitdancer]

Stephan: that might be a cleaner API, rather than overloading the semantics of 'executable'. But that would be a separate feature request.

I see what you are saying about "just the name". I misread Éric's message as saying he'd confirmed the bug, but I guess he's saying he confirmed that it works as documented.

It does look like subprocess lacks a test for this case.

[skrah]

Here's a patch with a test case. Without the fix in subprocess.py,
the test prints:

======================================================================
FAIL: test_specific_shell (main.POSIXProcessTestCase)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "Lib/test/test_subprocess.py", line 650, in test_specific_shell
    self.assertEqual(p.stdout.read().strip(), sh)
AssertionError: '/bin/sh' != '/bin/bash'

I think trying bash and ksh is sufficient; "echo $0" works for both
(for csh it does not). David, does the patch look good?

Éric, I'm by no means a tracker expert, so I wonder why you set the
resolution to 'accepted' at such an early stage. Does 'accepted' mean
'This is a valid report.' or does it mean 'I think the patch is correct.'?

[bitdancer]

The test unfortunately is too fragile. There is no guarantee that those shells will exist with those paths on any given system. Maybe you could use 'which' to find the path to the alternate shell for use in the test, and skip it if you can't find said shell.[*] I don't think there's a need to test more than one alternate shell.

Does this combo (executable + shell=True) have a meaning on Windows?

[*] or make bpo-444582 a dependency for this bug :)

[merwok]

David: My message was ambiguous. Your second reading was correct :)

Stefan: $0 works with dash too (no reason why it should not, but I still tested :). “Accepted” means “valid bug report”; to say that the patch is good people use a message or commit directly.