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.

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!

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

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.

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.

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".

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.

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.'?

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 issue 444582 a dependency for this bug :)

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.

Hm, /bin/sh should actually be removed from the list. It might be a
symlink to csh, for example.

I know this isn't an exhaustive test. If /bin/bash or /bin/ksh don't
exist, the test is skipped. I thought this is good enough, since the
majority of systems have one of those, so the test is guaranteed to
fail on one of the buildbots should the changes to subprocess.py be
removed in the future.

If a system only has /bin/csh, I wouldn't know how to write the test.

Ah, I misread the test the first time.

Well, I'd prefer a test that always did a real test on posix, but I suppose this has to be good enough since there could be systems that Python otherwise supports that *only* have /bin/sh.  Actually I suppose one could create a local symlink to /bin/sh and call through that...but I don't think this issue warrants going to any more effort, so I think the patch is fine as is.

You could argue that /bin/sh is good to have in the list since it makes sure that arg[0] doesn't get changed inappropriately in the default case.  I'd be more comfortable if the test generated a skip message if the only one of the shells that is found is /bin/sh, but I'm not going to insist on it.  (This will be true, for example, on FreeBSD (at least on 6, which is the install I can easily check...maybe you could add /usr/local/bin/bash as one of your candidates?  Most FreeBSDs will end up having that one installed, though not of course all of them).

I vote we don't worry about the csh only case :)

Ok, here's a more comprehensive test. Comments:

  1) Instead of emulating 'which' one could use find_executable
     from distutils.spawn. But this feels wrong.

  2) Skip the test if the primary issue cannot be tested.

  3) Exercise the test for /bin/sh, if it isn't a symlink. Then
     this part will be executed on some systems, e.g. FreeBSD.

  4) The check os.path.isfile('/bin/sh') isn't necessary, but good
     practice nonetheless.



Should this go into 2.6 and 3.1 as well?

Hello,

I am impressed by the activity here. I am a new programmer and cutting my teeth with python. I wish I knew how to do a patch and write unit tests, but I'm just not there yet.

I can show a test case in this fashion where it fails.

First I prove that /bin/sh is /bin/sh (er, dash) and /bin/bash is /bin/bash. Then it can be seen that when executable is set as /bin/bash and there is a shell error, that it returns a shell error from /bin/sh, not from /bin/bash.

I see there are patches here that should fix it, but for those wanting to see the buggy code in action, here it is using the fact that echo was mistyped as echos to raise a shell error (but shell=True so it doesn't raise an OSError exception).
_____________________________

in shell terminal:
$ file /bin/bash
/bin/bash: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.15, stripped
$ file /bin/sh
/bin/sh: symbolic link to `dash'
$ file /bin/dash
/bin/dash: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.15, stripped

in python interactive interpreter

>>> shell = Popen('echos', shell=True, executable="/bin/bash", stdin=PIPE, stderr=PIPE, stdout=PIPE)
>>> shell.communicate()
('', '/bin/sh: echos: command not found\n')
>>> shell = Popen('echos', shell=True, executable="/bin/bash", stdin=PIPE, stderr=PIPE, stdout=PIPE)
>>> shell.communicate()
('', '/bin/bash: echos: command not found\n')

Thank you for working on this so quickly. This is truly an amazing community.

I'm sorry, but the last lines in the interpreter reflect my code changes which seemed to not be sufficient, but it shows that just for this case at least, it corrects it and allows it to be run in /bin/bash shells (much of my shell code is bash since I'm linux only and this is usually available and I make use of c-like math or incrementors like $(( x++ )) in the shell code (I know, I know, I should make my code more portable such that it can run on /bin/sh, but I'm lazy).

Stefan: patch looks good to me in principle, but it looks like you made it against 2.7, and we commit to py3k first now.  The test does not work correctly on py3k because of a bytes/string issue, but I'm not clear on why the problem happens (I haven't looked in to it, I just observe that the test fails).  And yes I think this should go into 2.6 and 3.1 as well.

Narnie: if only we had enough manpower to handle all bugs as rapidly.  Please keep learning and help us where you can :)

As for your test...I'm not clear if you are saying Stefan's patch fixes or problem or not, but please observe this:

>>> os.execv('/bin/bash', ['/bin/sh', 'echos', 'foo'])
/bin/sh: echos: No such file or directory

That is, it is /bin/bash that is running, but it thinks its name is '/bin/sh' because that is what we passed in arg0 in the execv call.
Similarly you could do:

>>> os.execv('/bin/bash', ['my_funny_walk', 'echos', 'foo'])
my_funny_walk: echos: No such file or directory

So, popen is *calling* the correct shell, it's just that it is giving it the wrong arg[0] name, which Stefan's patch fixes.

Hmm.  I didn't intentionally remove the older patch, but it's probably not worth the effort to restore it.

David, thanks for all the comments! - The string/bytes issue was caused
by the fact that p.stdout is only opened in text mode when universal_newlines=True.


Committed fix in r82971, r82972, r82973 and r82974. I'll close this issue
if all buildbots are ok.

David, working as fast at learning Python as I can and loving it!

Stefan, thanks for the patches.

Thanks guys,
Narnie

The trustworthy buildbots look good, so I'm closing this.