classification
Title: subrocess.Popen needs /bin/sh but Android only has /system/bin/sh
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: xdegaye Nosy List: Chi Hsuan Yen, Roman.Evstifeev, WanderingLogic, asvetlov, brousch, chris.jerdonek, eric.araujo, ezio.melotti, gregory.p.smith, haypo, pitrou, python-dev, r.david.murray, rpetrov, skrah, xdegaye
Priority: normal Keywords: patch

Created on 2012-10-16 21:00 by brousch, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
popen-no-hardcode-bin-sh.patch WanderingLogic, 2014-11-06 19:08 review
unix_shell_16255.patch xdegaye, 2016-07-19 16:41 review
unix_shell_16255_2.patch xdegaye, 2016-07-21 13:08 review
unix_shell_16255_3.patch xdegaye, 2016-12-12 16:00 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (26)
msg173093 - (view) Author: Ben Rousch (brousch) Date: 2012-10-16 21:00
The subprocess.Popen function uses /bin/sh in Unix environments. Android is detected as a Unix environemnt, but has moved that executable to /system/bin/sh. This can be worked around by adding a parameter "executable='/system/bin/sh'" to the call, but it is impractical to do this for every call to Popen in every library and codebase. For subprocess.Popen to work on Android, Popen needs to be able to detect if /bin/sh is not there and try /system/bin/sh instead.
msg173100 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-16 21:24
Android really should not be breaking the standards that way.  We do want to support Android, but have you submitted a bug report to them?
msg173107 - (view) Author: Roumen Petrov (rpetrov) * Date: 2012-10-16 22:31
...Applications should note that the standard PATH to the shell cannot be assumed to be either /bin/sh or /usr/bin/sh, and should be determined by interrogation of the PATH returned by getconf PATH , ensuring that the returned pathname is an absolute pathname and not a shell built-in....
msg173109 - (view) Author: Ben Rousch (brousch) Date: 2012-10-16 23:16
@rpetrov thanks for finding that - I was having trouble hunting down what the standard is. I think you're quoting from http://pubs.opengroup.org/onlinepubs/009695399/utilities/sh.html
msg173115 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-17 00:06
We should do that, then, if /bin/sh doesn't exist.
msg173117 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-10-17 00:45
I wouldn't blame android for this; I doubt Android claims to support whatever standard you are holding it to.

It seems simple enough for us to make the default configurable (a public module level constant that anyone can override in their code after importing the module to change the default) or to otherwise query the system for where the default shell is at import time.
msg173120 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-17 01:12
Well, posix; but I was wrong about what posix required, as Roumen pointed out.
msg173361 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-19 22:44
Is there some document describing procedure for building/installing/running_tests at Android device?

I have Samsung Galaxy Tab and would to work on the issue.
But my android experience is limited to installing Scripting Layer for Android from Google Play and running python3 scripts.
msg173362 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-19 22:59
> It seems simple enough for us to make the default configurable

This seems good to do in any case (as opposed to having the string be "hard-coded"), and we can test it independently of Android.  There is a recently added test in the 3.x branches that can be modified: test method "test_executable_replaces_shell" which checks that the executable argument replaces the default shell for non-Windows.

I would suggest implementing Gregory's two suggested measures as two separate patches.
msg173363 - (view) Author: Ben Rousch (brousch) Date: 2012-10-19 23:29
> Is there some document describing procedure for building/installing/running_tests at Android device?

I'm using the android-python27 project, but you can also reproduce it under SL4A:

Start SL4A
Menu -> View -> Interpreters
Python 2.6.2 or Python 3

>>> from subprocess import Popen
>>> p = Popen("ls", shell=True)
*Fails* OSError: [Error 2] No such file or directory
>>> p = Popen("ls" shell=True, executable="/system/bin/sh")
*Works*
msg173477 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-21 20:12
It occurs to me that logic for detecting the shell might make sense for being part of the os module, e.g. os.getdefaultshell().
msg173658 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-10-24 09:08
Chris, agree with you.
msg174094 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-29 01:27
I created issue 16353 for adding a function to the os module for getting the path to the default shell.

The current issue 16255 could be addressed in 3.4 by calling such a function from the subprocess module.  For earlier versions (since enhancements are not allowed), the current issue could perhaps be addressed by backporting the logic in such a function directly into the subprocess module (i.e. without adding a new function to the os module).
msg224453 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-07-31 23:14
Why not simply use $SHELL?
msg224474 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2014-08-01 08:50
$SHELL is up to the user and not guaranteed to be anything remotely /bin/sh
compatible.
On Jul 31, 2014 4:14 PM, "Antoine Pitrou" <report@bugs.python.org> wrote:

>
> Antoine Pitrou added the comment:
>
> Why not simply use $SHELL?
>
> ----------
> nosy: +pitrou
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue16255>
> _______________________________________
>
msg230757 - (view) Author: Matt Frank (WanderingLogic) * Date: 2014-11-06 19:08
Assuming issue16353 is fixed using http://bugs.python.org/file36196/os.get_shell_executable.patch the appropriate way to find the path to the default shell is by calling os.get_shell_executable().

This is the 1-liner patch that uses os.get_shell_executable() in Popen() instead of the hard-coded string "/bin/sh".
msg264332 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-04-26 22:19
It seems that Android is the only known platform that deviates from
/bin/sh. So perhaps we should simply set a variable to either
/bin/sh or /system/bin/sh rather than waiting for #16353 to settle.
msg266084 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-05-22 15:51
The list of locations where '/bin/sh' is hard coded in the standard library:

Lib/distutils/tests/test_build_scripts.py|68 col 31| ("#!/bin/sh\n"
Lib/distutils/tests/test_install_scripts.py|57 col 38| write_script("shell.sh", ("#!/bin/sh\n"
Lib/distutils/tests/test_spawn.py|34 col 37| self.write_file(exe, '#!/bin/sh\nexit 1')
Lib/distutils/tests/test_spawn.py|45 col 37| self.write_file(exe, '#!/bin/sh\nexit 0')
Lib/subprocess.py|611 col 24| >>> check_output(["/bin/sh", "-c",
Lib/subprocess.py|1450 col 26| args = ["/bin/sh", "-c"] + args
Lib/test/test__osx_support.py|46 col 24| f.write("#!/bin/sh\n/bin/echo OK\n")
Lib/test/test__osx_support.py|58 col 24| f.write("#!/bin/sh\n/bin/echo ExpectedOutput\n")
Lib/test/test__osx_support.py|149 col 28| f.write("#!/bin/sh\n/bin/echo " + c_output)
Lib/test/test__osx_support.py|205 col 24| f.write("#!/bin/sh\nexit 255")
Lib/test/test_os.py|673 col 42| @unittest.skipUnless(os.path.exists('/bin/sh'), 'requires /bin/sh')
Lib/test/test_os.py|677 col 24| with os.popen("/bin/sh -c 'echo $HELLO'") as popen:
Lib/test/test_os.py|681 col 42| @unittest.skipUnless(os.path.exists('/bin/sh'), 'requires /bin/sh')
Lib/test/test_os.py|684 col 14| "/bin/sh -c 'echo \"line1\nline2\nline3\"'") as popen:
Lib/test/test_subprocess.py|1563 col 31| fobj.write("#!/bin/sh\n")
Lib/test/test_subprocess.py|1611 col 31| fobj.write("#!/bin/sh\n")
Lib/test/test_subprocess.py|1629 col 15| sh = '/bin/sh'
msg266089 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-05-22 16:30
I've also opened a feature request here:

https://code.google.com/p/android/issues/detail?id=210812
msg270047 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-07-09 15:18
> The list of locations where '/bin/sh' is hard coded in the standard library:

I have entered issue 27472.
msg270834 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-07-19 16:41
The attached patch fixes the issue. There is at least already an existing test case: test_subprocess.MiscTests.test_getoutput calls subprocess.getstatusoutput() that runs a Popen instance with shell=True.
msg270865 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-07-20 11:37
Apparently sysconfig.get_config_var() is already called in site.py anyway,
so in principle I'm fine with using it here.

In the stdlib it seems to be the first use outside site.py though, and
site.py can be disabled by using "python -S", so perhaps we should be
extra careful, set UNIX_SHELL unconditionally to "/bin/sh" and move
the Android conditional inside a try/except.


But perhaps I'm being irrational here.  Victor, what do you think?
msg270919 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-07-21 13:08
New patch that does not call sysconfig.get_config_var() on platforms that have '/bin/sh'.
msg283017 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-12 16:00
New patch using sys.getandroidapilevel() that is only defined for Android.
getandroidapilevel() is only available in 3.7, so only 3.7 is being fixed.
msg283116 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-13 16:16
For some reason the following notifications have not all been received (yet):
    remote: added 1 changesets with 2 changes to 2 files
    remote: buildbot: change(s) sent successfully
    remote: sent email to roundup at report@bugs.python.org
    remote: notified python-checkins@python.org of incoming changeset 96a9992d1003

But the buildbots have processed the build request.
Closing the issue.
msg283117 - (view) Author: Roundup Robot (python-dev) Date: 2016-12-13 16:20
New changeset 96a9992d1003 by Xavier de Gaye in branch 'default':
Issue #16255: subrocess.Popen uses /system/bin/sh on Android as the shell,
https://hg.python.org/cpython/rev/96a9992d1003
History
Date User Action Args
2017-03-31 16:36:16dstufftsetpull_requests: + pull_request905
2016-12-13 16:20:30python-devsetnosy: + python-dev
messages: + msg283117
2016-12-13 16:16:50xdegayesetstatus: open -> closed
resolution: fixed
messages: + msg283116

stage: patch review -> resolved
2016-12-12 16:00:24xdegayesetfiles: + unix_shell_16255_3.patch
versions: + Python 3.7, - Python 3.6
messages: + msg283017

assignee: xdegaye
dependencies: - add function to os module for getting path to default shell
2016-07-21 13:08:43xdegayesetfiles: + unix_shell_16255_2.patch

messages: + msg270919
2016-07-20 11:37:24skrahsetmessages: + msg270865
2016-07-19 16:49:14xdegayesetnosy: + haypo
2016-07-19 16:41:20xdegayesetfiles: + unix_shell_16255.patch

messages: + msg270834
stage: patch review
2016-07-09 15:18:28xdegayesetmessages: + msg270047
2016-05-22 16:30:05skrahsetmessages: + msg266089
2016-05-22 15:58:14xdegayelinkissue26865 dependencies
2016-05-22 15:51:10xdegayesetnosy: + xdegaye
messages: + msg266084
2016-04-27 10:37:46Roman.Evstifeevsetnosy: + Roman.Evstifeev
2016-04-26 22:19:42skrahsetnosy: + skrah

messages: + msg264332
versions: + Python 3.6, - Python 3.4
2015-11-25 15:12:44Chi Hsuan Yensetnosy: + Chi Hsuan Yen
2015-08-23 22:30:27r.david.murraylinkissue24919 superseder
2014-11-06 19:08:33WanderingLogicsetfiles: + popen-no-hardcode-bin-sh.patch
keywords: + patch
messages: + msg230757
2014-11-05 17:56:13WanderingLogicsetnosy: + WanderingLogic
2014-08-01 08:50:40gregory.p.smithsetmessages: + msg224474
2014-07-31 23:14:01pitrousetnosy: + pitrou
messages: + msg224453
2013-08-19 16:52:27asvetlovsetversions: - Python 2.7, Python 3.2, Python 3.3
2013-08-19 16:52:03asvetlovsetdependencies: + add function to os module for getting path to default shell
2012-11-02 18:59:49ezio.melottisetnosy: + ezio.melotti
2012-10-29 01:27:47chris.jerdoneksetmessages: + msg174094
2012-10-24 09:08:41asvetlovsetmessages: + msg173658
2012-10-21 20:12:49chris.jerdoneksetmessages: + msg173477
2012-10-19 23:29:36brouschsetmessages: + msg173363
2012-10-19 22:59:21chris.jerdoneksetmessages: + msg173362
2012-10-19 22:44:21asvetlovsetmessages: + msg173361
2012-10-19 16:18:49eric.araujosetnosy: + eric.araujo
2012-10-17 01:12:12r.david.murraysetmessages: + msg173120
2012-10-17 00:45:41gregory.p.smithsetmessages: + msg173117
2012-10-17 00:22:38chris.jerdoneksetnosy: + asvetlov, chris.jerdonek
2012-10-17 00:06:31r.david.murraysetversions: + Python 3.2, Python 3.3, Python 3.4
nosy: + gregory.p.smith

messages: + msg173115

type: crash -> behavior
2012-10-16 23:16:29brouschsetmessages: + msg173109
2012-10-16 22:31:07rpetrovsetnosy: + rpetrov
messages: + msg173107
2012-10-16 21:24:06r.david.murraysetnosy: + r.david.murray
messages: + msg173100
2012-10-16 21:00:02brouschcreate