Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subrocess.Popen needs /bin/sh but Android only has /system/bin/sh #60459

Closed
brousch mannequin opened this issue Oct 16, 2012 · 26 comments
Closed

subrocess.Popen needs /bin/sh but Android only has /system/bin/sh #60459

brousch mannequin opened this issue Oct 16, 2012 · 26 comments
Assignees
Labels
3.7 stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@brousch
Copy link
Mannequin

brousch mannequin commented Oct 16, 2012

BPO 16255
Nosy @gpshead, @pitrou, @vstinner, @ezio-melotti, @merwok, @bitdancer, @asvetlov, @skrah, @cjerdonek, @xdegaye, @Fak3, @yan12125
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • popen-no-hardcode-bin-sh.patch
  • unix_shell_16255.patch
  • unix_shell_16255_2.patch
  • unix_shell_16255_3.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 = 'https://github.com/xdegaye'
    closed_at = <Date 2016-12-13.16:16:50.952>
    created_at = <Date 2012-10-16.21:00:02.467>
    labels = ['3.7', 'type-bug', 'library']
    title = 'subrocess.Popen needs /bin/sh but Android only has /system/bin/sh'
    updated_at = <Date 2017-03-31.16:36:16.098>
    user = 'https://bugs.python.org/brousch'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:16.098>
    actor = 'dstufft'
    assignee = 'xdegaye'
    closed = True
    closed_date = <Date 2016-12-13.16:16:50.952>
    closer = 'xdegaye'
    components = ['Library (Lib)']
    creation = <Date 2012-10-16.21:00:02.467>
    creator = 'brousch'
    dependencies = []
    files = ['37139', '43795', '43809', '45860']
    hgrepos = []
    issue_num = 16255
    keywords = ['patch']
    message_count = 26.0
    messages = ['173093', '173100', '173107', '173109', '173115', '173117', '173120', '173361', '173362', '173363', '173477', '173658', '174094', '224453', '224474', '230757', '264332', '266084', '266089', '270047', '270834', '270865', '270919', '283017', '283116', '283117']
    nosy_count = 16.0
    nosy_names = ['gregory.p.smith', 'pitrou', 'vstinner', 'ezio.melotti', 'eric.araujo', 'rpetrov', 'r.david.murray', 'asvetlov', 'skrah', 'chris.jerdonek', 'xdegaye', 'python-dev', 'Roman.Evstifeev', 'brousch', 'WanderingLogic', 'yan12125']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue16255'
    versions = ['Python 3.7']

    @brousch
    Copy link
    Mannequin Author

    brousch mannequin commented Oct 16, 2012

    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.

    @brousch brousch mannequin added type-crash A hard crash of the interpreter, possibly with a core dump stdlib Python modules in the Lib dir labels Oct 16, 2012
    @bitdancer
    Copy link
    Member

    bitdancer commented Oct 16, 2012

    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?

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Oct 16, 2012

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

    @brousch
    Copy link
    Mannequin Author

    brousch mannequin commented Oct 16, 2012

    @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

    @bitdancer
    Copy link
    Member

    bitdancer commented Oct 17, 2012

    We should do that, then, if /bin/sh doesn't exist.

    @bitdancer bitdancer added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 17, 2012
    @gpshead
    Copy link
    Member

    gpshead commented Oct 17, 2012

    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.

    @bitdancer
    Copy link
    Member

    bitdancer commented Oct 17, 2012

    Well, posix; but I was wrong about what posix required, as Roumen pointed out.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 19, 2012

    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.

    @cjerdonek
    Copy link
    Member

    cjerdonek commented Oct 19, 2012

    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.

    @brousch
    Copy link
    Mannequin Author

    brousch mannequin commented Oct 19, 2012

    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*

    @cjerdonek
    Copy link
    Member

    cjerdonek commented Oct 21, 2012

    It occurs to me that logic for detecting the shell might make sense for being part of the os module, e.g. os.getdefaultshell().

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 24, 2012

    Chris, agree with you.

    @cjerdonek
    Copy link
    Member

    cjerdonek commented Oct 29, 2012

    I created bpo-16353 for adding a function to the os module for getting the path to the default shell.

    The current bpo-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).

    @pitrou
    Copy link
    Member

    pitrou commented Jul 31, 2014

    Why not simply use $SHELL?

    @gpshead
    Copy link
    Member

    gpshead commented Aug 1, 2014

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


    @WanderingLogic
    Copy link
    Mannequin

    WanderingLogic mannequin commented Nov 6, 2014

    Assuming bpo-16353 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".

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 26, 2016

    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 bpo-16353 to settle.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented May 22, 2016

    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'

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 22, 2016

    I've also opened a feature request here:

    https://code.google.com/p/android/issues/detail?id=210812

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Jul 9, 2016

    The list of locations where '/bin/sh' is hard coded in the standard library:

    I have entered bpo-27472.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Jul 19, 2016

    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.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jul 20, 2016

    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?

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Jul 21, 2016

    New patch that does not call sysconfig.get_config_var() on platforms that have '/bin/sh'.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 12, 2016

    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.

    @xdegaye xdegaye mannequin added the 3.7 label Dec 12, 2016
    @xdegaye xdegaye mannequin self-assigned this Dec 12, 2016
    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 13, 2016

    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.

    @xdegaye xdegaye mannequin closed this as completed Dec 13, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 13, 2016

    New changeset 96a9992d1003 by Xavier de Gaye in branch 'default':
    Issue bpo-16255: subrocess.Popen uses /system/bin/sh on Android as the shell,
    https://hg.python.org/cpython/rev/96a9992d1003

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants