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

add function to os module for getting path to default shell #60557

Closed
cjerdonek opened this issue Oct 29, 2012 · 52 comments
Closed

add function to os module for getting path to default shell #60557

cjerdonek opened this issue Oct 29, 2012 · 52 comments
Labels
3.8 stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@cjerdonek
Copy link
Member

cjerdonek commented Oct 29, 2012

BPO 16353
Nosy @gpshead, @ncoghlan, @pitrou, @vstinner, @tiran, @ned-deily, @ezio-melotti, @merwok, @bitdancer, @asvetlov, @cjerdonek, @4kir4, @xdegaye, @Fak3, @serhiy-storchaka, @eryksun, @aixtools, @csabella
Files
  • issue16353.diff
  • issue16353_added_doc_tag.diff
  • issue16353.diff
  • issue16353.diff
  • os.get_shell_executable.patch: os.get_shell_executable() implementation, docs, tests.
  • os.get_shell_executable-3.patch: exclude '' completely
  • default_shell.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 2019-10-22.23:47:05.614>
    created_at = <Date 2012-10-29.01:19:20.218>
    labels = ['3.8', 'type-feature', 'library']
    title = 'add function to os module for getting path to default shell'
    updated_at = <Date 2019-10-22.23:47:05.613>
    user = 'https://github.com/cjerdonek'

    bugs.python.org fields:

    activity = <Date 2019-10-22.23:47:05.613>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-10-22.23:47:05.614>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2012-10-29.01:19:20.218>
    creator = 'chris.jerdonek'
    dependencies = []
    files = ['27849', '27855', '27897', '27898', '36196', '37140', '42953']
    hgrepos = []
    issue_num = 16353
    keywords = ['patch']
    message_count = 52.0
    messages = ['174093', '174569', '174572', '174574', '174576', '174578', '174580', '174582', '174583', '174586', '174607', '174619', '174621', '174623', '174624', '174628', '174629', '174631', '174646', '174652', '174689', '174690', '174885', '174886', '174929', '174930', '174932', '174933', '174935', '174962', '175002', '175003', '175006', '203160', '224477', '224508', '224509', '224512', '224514', '230711', '230713', '230720', '230754', '230755', '230761', '266135', '270099', '271418', '271439', '335137', '335138', '355182']
    nosy_count = 21.0
    nosy_names = ['gregory.p.smith', 'ncoghlan', 'pitrou', 'vstinner', 'christian.heimes', 'ned.deily', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'asvetlov', 'chris.jerdonek', 'neologix', 'akira', 'xdegaye', 'Roman.Evstifeev', 'serhiy.storchaka', 'eryksun', 'lyapun', 'WanderingLogic', 'Michael.Felt', 'cheryl.sabella']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue16353'
    versions = ['Python 3.8']

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Oct 29, 2012

    This issue is to add a function to the os module for getting the path to the default shell (e.g. os.getdefaultshell()).

    In bpo-16255, it was reported that on Android, the path to the default shell is "/system/bin/sh" rather than "/bin/sh" which it is on other Unix systems.

    Such a function in the os module would implement the necessary detection logic which could then be used, for example, by the subprocess module when invoking Popen() with shell=True (e.g. for bpo-16255).

    @cjerdonek cjerdonek added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Oct 29, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Nov 2, 2012

    Is it the "system default shell" or the user's default shell that we want here?

    That is, shouldn't we look up pwd.getpwuid(os.getuid()).pw_shell ?
    (but only when os.getuid() == os.geteuid()?)

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 2, 2012

    The system default shell is what I had in mind when filing this issue (i.e. what Popen() uses by default or, in the case of Android, what Popen() should use).

    @pitrou
    Copy link
    Member

    pitrou commented Nov 2, 2012

    The system default shell is what I had in mind when filing this issue
    (i.e. what Popen() uses by default or, in the case of Android, what
    Popen() should use).

    Well, the question then becomes whether Popen() shouldn't use the user's
    default shell instead? :)
    It probably doesn't make much of a difference in most cases, though.
    Also, there's the problem that some special users may have a void shell
    entry (or /bin/false or /sbin/nologin), to prevent a human from logging
    in as them.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 2, 2012

    I guess to return sh if supported, cmd.exe for Windows.
    The reason is: other shells can have different calling agreements (I mean rules to process command line).
    subprocess intended to use by library writers, so we need solid well known basis.

    There are another option — provide two functions: first for shell in terms of subprocess requirements and second will return actual user shell.
    I like it.

    @tjguk
    Copy link
    Member

    tjguk commented Nov 2, 2012

    On 02/11/2012 21:00, Andrew Svetlov wrote:

    I guess to return sh if supported, cmd.exe for Windows.

    FWIW the canonical approach on Windows is to return whatever %COMSPEC%
    points to.

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 2, 2012

    Well, the question then becomes whether Popen() shouldn't use the user's default shell instead? :)

    That's a good question, too. :) I was thinking just in terms of supporting the status quo. Maybe two functions would be useful? (as suggested also by Andrew)

    I do think the question of changing Popen()'s behavior should be decided independently of this issue though. In other words, even if we change Popen() in the future, I think exposing the system default shell would still be worthwhile.

    @gpshead
    Copy link
    Member

    gpshead commented Nov 2, 2012

    That is, shouldn't we look up pwd.getpwuid(os.getuid()).pw_shell ?
    (but only when os.getuid() == os.geteuid()?)

    No, you can't use the users shell from the pwd module. That can be
    any crazy program. Not a functional /bin/sh for use in making
    commands when shell=True in subprocess which is where this feature
    request came from.

    *system* default, not the user's. subprocess MUST use /bin/sh or
    equivalent, ignoring any per-user setting.

    @bitdancer
    Copy link
    Member

    bitdancer commented Nov 2, 2012

    The "system default shell" (which should always be a /bin/sh workalike, I think) should always be the default. Any other shell should be something that has to be specified explicitly. At least, that's the way most posix programs work, I think. As Chris said, different shells can use different syntax, so if you want to write portable code you want to be able to expect /bin/sh by default.

    So at a minimum we want a function for the system default shell. Anything else is a bonus :)

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 2, 2012

    BTW, according to PEP-11 (http://www.python.org/dev/peps/pep-0011/)
    Python 3.4 will remove code for "Windows systems where COMSPEC points to command.com". There are only winner: cmd.exe as well known shell good as default, command.com is gone.

    cmd.exe can be assumed as default shell as well as sh for posix platforms.

    About subprocess: I like to add something to point str (or pathlib path when accepted) as shell, but it's definitely another issue.

    @lyapun
    Copy link
    Mannequin

    lyapun mannequin commented Nov 3, 2012

    Diff with implementation, test and doc.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 3, 2012

    I would to prefer *get_default_shell* as function name (it's a bit shorter).

    Also please add ".. versionadded:: 3.4" tag to docs.

    @lyapun
    Copy link
    Mannequin

    lyapun mannequin commented Nov 3, 2012

    I would to prefer *get_default_shell* as function name (it's a bit shorter).

    Hm... I'm not sure, because someone can imagine that function returns some object or something like this... 'path' indicates that function returns something like string.

    Also please add ".. versionadded:: 3.4" tag to docs.

    Oh, sorry, added.

    @tiran
    Copy link
    Member

    tiran commented Nov 3, 2012

    which('sh') isn't correct here. 'which' searches in all PATH environ parts. However the shell must be looked up in CS_PATH only.

    From man sh(1posix):

    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.

    'getconf PATH' queries confstr(_CS_PATH).

    I suggest that you modify the POSIX part to:

      path = os.confstr("CS_PATH")
      which('sh', path=path)

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 3, 2012

    +++ b/Lib/shutil.py Sat Nov 03 13:32:05 2012 +0200
    +
    +def get_default_shell_path():

    Why is the patch putting the function in the shutil module? The function should go in the os module as the title and comments of this issue state. shutil seems misplaced to me.

    For example, the description of shutil in the docs is, "The shutil module offers a number of high-level operations on files and collections of files. In particular, functions are provided which support file copying and removal." In contrast, the description of the os module is, "This module provides a portable way of using operating system dependent functionality." The default shell is operating system dependent functionality.

    @tiran
    Copy link
    Member

    tiran commented Nov 3, 2012

    I'm with Chris. The information should be stored in the os module. I suggest os.shell var or os.get_shell() function like this:

    def get_shell():
        for path in confstr("CS_PATH").split(pathsep):
            sh = sep.join((path, "sh"))
            try:
                mode = stat(sh).st_mode
            except OSError:
                pass
            else:
                if st.S_ISREG(mode):
                    return sh
        raise FileNotFound("sh")

    According to all examples S_ISREG() is sufficient here.

    On Windows the function should use the env var COMSPEC instead of hard coding "cmd.exe".

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 3, 2012

    Is it ok to import *which* functions from shutil in *os* module?
    There is only reason to put function into shutil.

    But I like Christian's sketch.

    Also, what reason to get shell name from COMSPEC? What should we do if COMSPEC points to some another shell, not cmd.exe?

    @bitdancer
    Copy link
    Member

    bitdancer commented Nov 3, 2012

    I think it would not be ok to import shutil in os, so I'm glad there is an alternative.

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 3, 2012

    Also, what reason to get shell name from COMSPEC? What should we do if COMSPEC points to some another shell, not cmd.exe?

    FWIW, the subprocess module does this (with surrounding code linked after):

    comspec = os.environ.get("COMSPEC", "cmd.exe")

    http://hg.python.org/cpython/file/ed091424f230/Lib/subprocess.py#l1060

    @tiran
    Copy link
    Member

    tiran commented Nov 3, 2012

    The os module can't import shutil as it would create a circular import. Also shutil.which() does lots of stat calls and we don't want additional stat calls in a core module like os. My implementation requires just one stat() call.

    COMSPEC can point to an alternative command implementation. That's the point of COMSPEC. Perhaps some people remember 4DOS and 4NT.

    @merwok
    Copy link
    Member

    merwok commented Nov 3, 2012

    Any interest in doing like os.get_terminal_size/shutil.get_terminal_size with the os function being basic (i.e. current patch) and the shutil version querying the env var SHELL in addition?

    @merwok
    Copy link
    Member

    merwok commented Nov 3, 2012

    Please use ``sh`` or maybe :command:`sh` (check the sphinx doc) instead of `sh`.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 5, 2012

    Christian,
    Is there os.confstr supported by MaxOS X?
    Is there using of environ['PATH'] makes sense as good callback if former is not present?

    About COMSPEC. From my point of view it's useful if we need default path.
    Or if we have Win9x, where shell is command.com (not supported by 3.4)

    I guess to hardcode cmd.exe for Windows for the same reason as we hardcode sh for Unix systems.

    I agree to moving code to os if we no need to use shutil.which.
    The function name is still the question: getdefaultshell, getshell, get_shell etc.

    Adding shutil.getdefaultshell function is interesting suggestion. It function intended to return user shell (from SHELL env var or COMSPEC for Windows) and not used in subprocess if shell=True (but can be passed as executable param if need).

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 5, 2012

    Any interest in doing like os.get_terminal_size/shutil.get_terminal_size

    If functions with two different behaviors are needed, I think the two functions should probably have different names (e.g. get_shell() and get_user_shell()). Otherwise, it may create confusion as to which to call (or which is being called when reading code).

    Moreover, it doesn't seem like the following guidance in the docs for which get_terminal_size() to use would hold in parallel for the shell variants: "shutil.get_terminal_size() is the high-level function which should normally be used, os.get_terminal_size is the low-level implementation."

    @lyapun
    Copy link
    Mannequin

    lyapun mannequin commented Nov 5, 2012

    Updated patch.
    Moved function to os and used Christian Heimes implementation.
    Updated doc, and test.
    Also renamed function to get_shell.

    Test passes on mac os and windows.

    @tiran
    Copy link
    Member

    tiran commented Nov 5, 2012

    I've tested confstr("CS_PATH") on Linux, Mac OS X, Solaris, HP-UX and BSD. It works and the path to sh is always included.

    Taras:
    You don't have to perform the platform inside the get_shell() function. I suggest that you define the function depending on the value of name.

    if name == "posix":
        def get_shell():
            ...
    elif name in {"nt", "ce"}:
        def get_shell():
            ...

    @lyapun
    Copy link
    Mannequin

    lyapun mannequin commented Nov 5, 2012

    Updated patch in regards to Christian Heimes remark.

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 5, 2012

    Is someone able to test the patch on Android (the impetus for this issue)?

    @cjerdonek
    Copy link
    Member Author

    cjerdonek commented Nov 5, 2012

    Some minor comments.

    +.. function :: get_shell()
    +
    + Return the path to default shell.

    Three leading spaces needed. Also, "Return the path to the default shell."

    • For unix system returns path to sh, for windows returns path to cmd.exe.

    "On Unix systems returns the path to sh, and on Windows returns the path to cmd.exe." I would also document and test FileNotFound (e.g. by temporarily changing os.confstr).

    + """Return the path to default shell for Unix."""

    the default shell

    + """Return the path to default shell for Windows."""

    the default shell

    +class TestGetShell(unittest.TestCase):
    + def test_get_shell(self):
    + shell = os.get_shell()
    + param = "/c" if sys.platform == 'win32' else "-c"
    + s = subprocess.Popen([shell, param, "echo test"],
    + stdout=subprocess.PIPE)

    Alignment should be:

    + s = subprocess.Popen([shell, param, "echo test"],
    + stdout=subprocess.PIPE)

    @tiran
    Copy link
    Member

    tiran commented Nov 6, 2012

    Meh, Python 2.6 from SL4A (scripting languages for Android) doesn't have os.confstr(). I just tried it in a Android 2.3.3 emulator. Python 2.6 on Linux has the function. I propose we fall back to PATH env and use /bin as last resort.

    try:
        cspath = confstr("CS_PATH")
    except (ValueError, NameError):
        # installation doesn't have confstr() or doesn't know about CS_PATH
        # fall back to PATH environ and use /bin as last resort
        cspath = environ.get("PATH", "/bin")
    for path in cspath.split(pathsep):
        ...
    

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 6, 2012

    Confirm it for SL4A Python 3.2

    @lyapun
    Copy link
    Mannequin

    lyapun mannequin commented Nov 6, 2012

    Andrew, do you mean that Christian Heimes last snippet working on Android?

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 6, 2012

    I mean the last available SL4A doesn't have os.confstr
    Fallback should work, os.environ['PATH'] contains '/system/bin' where 'sh' is living.

    @tiran
    Copy link
    Member

    tiran commented Nov 17, 2013

    3.4 beta1 will be released next weekend. I'll try to submit a patch in the next couple of days.

    @tiran tiran self-assigned this Nov 17, 2013
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 1, 2014

    Should it be function? Why not use just a variable initialized at import time? The path to the default shell shouldn't change during the time of program execution.

    if sys.platform == 'win32':
        default_shell = 'cmd.exe'
    else:
        default_shell = '/bin/sh'

    @eryksun
    Copy link
    Contributor

    eryksun commented Aug 1, 2014

    The path to the default shell shouldn't change during the time
    of program execution.

    On Windows the ComSpec environment variable could change during program execution.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 1, 2014

    Taking into consideration msg174586 we can ignore the ComSpec environment
    variable and hardcode "cmd.exe" on Windows, as we ignore the SHELL environment
    variable on Posix systems.

    @eryksun
    Copy link
    Contributor

    eryksun commented Aug 1, 2014

    Defaulting to just "cmd.exe" can execute an arbitrary program named "cmd.exe" in the application directory or current directory.

    When CreateProcess needs to find the shell to execute a batch file (.bat or .cmd), it doesn't search for "cmd.exe" -- at least not in NT 6. If ComSpec isn't set, it defaults to "%SystemRoot%\system32\cmd.exe". If both ComSpec and SystemRoot are unset, executing a batch file via CreateProcess fails with ERROR_DIRECTORY.

    I think Python should use the same default path if it's ignoring ComSpec:

        default_shell = path.join(environ["SystemRoot"], "system32", "cmd.exe")

    A KeyError shouldn't be a problem here. CPython won't even run if SystemRoot isn't set to the correct path:

    C:\>set SystemRoot=
    
    C:\>py -3
    Fatal Python error: Failed to initialize Windows random API (CryptoGen)
    

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Aug 1, 2014

    Should it be function? Why not use just a variable initialized at
    import time? The path to the default shell shouldn't change during
    the time of program execution.

    if sys.platform == 'win32':
    default_shell = 'cmd.exe'
    else:
    default_shell = '/bin/sh'

    Andriod uses /system/bin/sh (and /sbin/sh if we believe some adb
    source on the internet). See bpo-16255: "subrocess.Popen needs
    /bin/sh but Android only has /system/bin/sh"

    POSIX recommends [1] getconf PATH (os.confstr('CS_PATH')):

    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.

    For example, to determine the location of the standard sh utility:

        $ command -v sh

    i.e. shell executable is shutil.which('sh', os.defpath) on POSIX.
    os.defpath could be tweaked on Android.

    To avoid dependency on shutil something like msg174628 could be
    adopted:

      if sys.platform == "win32": # use sys.platform to accommodate java
          def get_shell_executable():
              """Return path to the command processor specified by %ComSpec%.
          Default: %SystemRoot%\\system32\\cmd.exe
          """
          return (environ.get('ComSpec') or
                  path.join(environ.get('SystemRoot', r'C:\\Windows'),
                            r'system32\\cmd.exe'))
    

    else: # POSIX
    def get_shell_executable():
    """Return path to the standard system shell executable.

          Default: '/bin/sh'
          """
          for dirpath in defpath.split(pathsep):
              sh = dirpath + sep + 'sh' #NOTE: interpret '' as '/'
              if access(sh, F_OK | X_OK) and path.isfile(sh):
                  #NOTE: allow symlinks e.g., /bin/sh on Ubuntu may be dash
                  return sh
          return '/bin/sh' #XXX either OS or python are broken if we got here
    

    Windows part is based on msg224512. If r'C:\Windows' or '/bin/sh' code
    is reached then it means that either OS or python are
    broken/misconfigured.

    system(), popen() are described on POSIX as [2]:

    # fork()
    execl(shell path, "sh", "-c", command, (char *)0);

    subprocess module that replaces system, popen, spawn*p* calls may use
    os.get_shell_executable() to implement shell=True.

    os.defpath is [3]:

    The default search path used by exec*p* and spawn*p* if the
    environment doesn’t have a 'PATH' key. Also available via os.path.

    In the absense of os.confstr('CS_PATH') on Android (msg175006),
    os.defpath seems appropriate than os.environ['PATH'] to expand 'sh'
    basename to the full path to be used by subprocess later.

    os.get_shell() name might imply that a shell object is returned that
    can run commands like for example os.popen() returns a file-like
    object that is not true (the intent is to return a path -- a simple
    string).

    os.get_shell_executable() is similar to sys.executable that returns
    path to python executable.

    os.get_shell_executable() is not necessary for every python run
    therefore it is better to keep it a function to avoid unnecessary stat
    calls on startup. Though the result should not change during the
    program run.

    On Windows the ComSpec environment variable could change during
    program execution.

    Does it? The docs for os.environ say [4]:

    This mapping is captured the first time the os module is imported,
    typically during Python startup as part of processing
    site.py. Changes to the environment made after this time are not
    reflected in os.environ, except for changes made by modifying
    os.environ directly.

    I've uploaded a patch with the described implementation (plus docs, tests).

    [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html
    [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/popen.html
    [3] https://docs.python.org/3.4/library/os.html#os.defpath
    [4] https://docs.python.org/3.4/library/os.html#os.environ

    @WanderingLogic
    Copy link
    Mannequin

    WanderingLogic mannequin commented Nov 5, 2014

    Unfortunately os.defpath seems to be hardcoded. And hardcoded to the wrong value on every system I have looked at, including Linux.

    Lib/posixpath.py sets defpath=':/bin:/usr/bin' which is not what getconf CS_PATH returns on my Linux (the extra ':' at the beginning means "include the cwd"[1] which is almost certainly not what anyone wants.) The hardcoded value '/bin:/usr/bin' would also be wrong for Solaris and AIX installations that are trying to be POSIX (I think they use /usr/xpg4/bin/sh).

    Meanwhile Lib/macpath.py sets defpath=':', which is also almost certainly wrong. (I don't have a Mac and have never programmed one, but StackOverflow[2] indicates that one should use path_helper (which in turn gets the default value by reading it from the files in /etc/paths.d))[3] So it seems likely that this patch breaks Popen() on MacOS (unless, perhaps, a path of ':' means something special on Mac?).

    And Lib/ntpath.py sets defpath='.;C:\\bin', which doesn't resemble a path that even works (as pointed out in now closed http://bugs.python.org/issue5717). (The correct value may be '%SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;%SYSTEMROOT%\System32\WindowsPowerShell\v1.0\'.)

    So I don't know where to go next. I'm happy to cook up a different patch, but I have no idea what it should be. Here are some possibilities:

    1. Kick the can down the road: file a new bug against os.defpath and (somehow) fix Lib/*path.py so they do something more reasonable in more cases. One of which might be to have posixpath.py try to call os.confstr() (and then, perhaps, special-code something in Modules/posixmodule.c in the case that HAVE_CONFSTR is not defined.)

    2. Modify @akira's patch to call os.confstr('CS_PATH') instead of os.defpath, and then deal with the fact that very few systems actually implement confstr() (perhaps by special-coding something in Modules/posixmodule.c as described above.)

    3. Modify this patch to fall back to PATH if sh can't be found on os.defpath (or os.confstr('CS_PATH') fails).

    [1] man bash on Linux, search for the description of the PATH variable. (e.g., http://man7.org/linux/man-pages/man1/bash.1.html)
    [2] http://stackoverflow.com/questions/9832770/where-is-the-default-terminal-path-located-on-mac
    [3] https://developer.apple.com/library/mac/documentation/Darwin/Reference/Manpages/man8/path_helper.8.html
    [4] http://superuser.com/questions/124239/what-is-the-default-path-environment-variable-setting-on-fresh-install-of-window

    @ned-deily
    Copy link
    Member

    ned-deily commented Nov 5, 2014

    Matt, ignore Lib/macpath.py. It is not used on OS X systems other than in the rare case that someone explicitly needs to parse obsolete Classic Mac OS (Mac OS 9 or earlier) style path names. OS X uses Lib/posixpath.py.

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Nov 6, 2014

    Matt Frank added the comment:

    Unfortunately os.defpath seems to be hardcoded. And hardcoded to the
    wrong value on every system I have looked at, including Linux.

    os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
    used on Linux) sysdeps/posix/spawni.c I don't know whether it is
    possible to change CS_PATH without recompiling every statically linked
    executable on a system, sysdeps/unix/confstr.h:

      #define CS_PATH "/bin:/usr/bin"

    Though this issue is about the path to the standard shell sh/cmd.exe.
    It is not about os.defpath.

    The patch [1] has tests. Have you tried to run them?
    The tests *pass* at least on one system ;)

    [1] http://bugs.python.org/review/16353/#ps12569

    To run the tests, download the patch into your python checkout:

      $ curl -O \
      http://bugs.python.org/file36196/os.get_shell_executable.patch

    and apply it:

      $ patch -p1< os.get_shell_executable.patch

    to run only test_os.py:

      $ ./python -mtest test_os

    Lib/posixpath.py sets defpath=':/bin:/usr/bin' which is _not_ what
    getconf CS_PATH` returns on my Linux (the extra ':' at the beginning
    means "include the cwd" which is almost certainly not what anyone
    wants.)

    os.get_shell_executable() does not use cwd. Neither the documentation
    nor the code in the patch suggest that.

    The hardcoded value '/bin:/usr/bin' would also be wrong for
    Solaris and AIX installations that are trying to be POSIX (I think
    they use /usr/xpg4/bin/sh).

    os.defpath is the default search path used by exec*p* and spawn*p*
    i.e., if os.defpath is incorrect for these system then os.exec*p* and
    os.spawn*p* functions could also be broken.

    I expect that Windows, OS X, Linux work as is. If the tests fail on
    Solaris, AIX then os.defpath should be tweaked on these systems if it
    hasn't been already. Note: os.defpath is a very conservative value: it
    should be set at python's installation time at the very
    latest. Henceforth it should remain the same.

    And Lib/ntpath.py sets defpath='.;C:\\bin', which doesn't resemble a
    path that even works (as pointed out in now closed
    http://bugs.python.org/issue5717). (The correct value may be
    %SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;%SYSTEMROOT%\System32\WindowsPowerShell\v1.0\'.)

    Please, look at the patch. Neither the documentation nor the code
    suggest that os.defpath is used on Windows.

    So I don't know where to go next. I'm happy to cook up a different
    patch, but I have no idea what it should be. Here are some
    possibilities:

    1. Kick the can down the road: file a new bug against os.defpath and
      (somehow) fix Lib/*path.py so they do something more reasonable in
      more cases. One of which might be to have posixpath.py try to call
      os.confstr() (and then, perhaps, special-code something in
      Modules/posixmodule.c in the case that HAVE_CONFSTR is not defined.)

    2. Modify @akira's patch to call os.confstr('CS_PATH') instead of
      os.defpath, and then deal with the fact that very few systems actually
      implement confstr() (perhaps by special-coding something in
      Modules/posixmodule.c as described above.)

    Note I'm the author of http://bugs.python.org/issue16353#msg224514
    message that references the POSIX recommendation about getconf PATH
    and explicitly mentions os.confstr('CS_PATH').

    I don't remember exactly the motivation to use os.defpath instead of
    os.confstr('CS_PATH').

    A guess: the result is the same (a lone `:` is ignored in the patch) but
    os.defpath is easier to modify for uncommon systems where os.confstr
    might not be available.

    I expect that the tests in the patch will pass on all stable buildbots
    [2] successfully without any modifications (with os.defpath as is).

    [2] http://buildbot.python.org/all/waterfall?category=3.x.stable

    Other systems should probably configure os.defpath appropriately (it
    should include the path to the standard shell).

    1. Modify this patch to fall back to PATH if sh can't be found on
      os.defpath (or os.confstr('CS_PATH') fails).

    The standard shell should not depend on PATH envvar. The tests show that
    os.get_shell_executable() works even with an empty environment. The
    motivation is the same as why getconf PATH exists in the first place.

    The documentation for os.get_shell_executable() (from the patch):

    Return a path to the standard system shell executable -- sh
    program on POSIX (default: '/bin/sh') or %ComSpec% command
    processor on Windows (default: %SystemRoot%\system32\cmd.exe).

    Availability: Unix, Windows

    The intent is that os.get_shell_executable() returns the same *shell
    path* as used by system(3) on POSIX i.e., (ignoring signals,
    etc) os.system(command) could be implemented:

      subprocess.call(command, shell=True, executable=os.get_shell_executable())

    (presumably subprocess can use get_shell_executable() internally instead
    of /bin/sh but it is another issue)

    @WanderingLogic
    Copy link
    Mannequin

    WanderingLogic mannequin commented Nov 6, 2014

    In msg174930 Christian Heimes (christian.heimes) wrote:

    I've tested confstr("CS_PATH") on Linux, Mac OS X, Solaris, HP-UX
    and BSD. It works and the path to sh is always included.

    In msg230713 Ned Deily(ned.deily) wrote:

    ignore Lib/macpath.py.
    [...]
    OS X uses Lib/posixpath.py.

    These two messages have convinced me that the correct approach is to kick the can down the road. I will file a new issue and patch to fix os.defpath (by initializing os.defpath by calling confstr("CS_PATH")). Then I will file a new issue and patch that will define a reasonable os.confstr() for platforms where HAVE_CONFSTR is not defined.

    @WanderingLogic
    Copy link
    Mannequin

    WanderingLogic mannequin commented Nov 6, 2014

    In msg230720 Akira Li (akira) wrote:

    os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
    used on Linux) sysdeps/posix/spawni.c I don't know whether it is
    possible to change CS_PATH without recompiling every statically linked
    executable on a system, sysdeps/unix/confstr.h:

    #define CS_PATH "/bin:/usr/bin"

    Though this issue is about the path to the standard shell sh/cmd.exe.
    It is not about os.defpath.

    I understand this issue is about the path to the standard shell.
    My argument is that os.defpath is the wrong tool for finding that
    path, and os.confstr('CS_PATH') is the correct tool, as you originally
    suggested in msg224514.

    The patch [1] has tests. Have you tried to run them?
    The tests *pass* at least on one system ;)

    [...]

    os.get_shell_executable() does not use cwd. Neither the documentation
    nor the code in the patch suggest that.

    I ran the tests (and the entire Python test suite) before I wrote my
    first message. But you didn't test what happens when there is a bogus
    'sh' somewhere along the path. You also have a bug in your path
    searching loop that interprets an empty element on the path as "/".
    Here's the current failure mode:

    Go to root on your Posix system. With "su" or "sudo" create an
    executable file named sh. (for example "cd /; sudo touch sh; sudo
    chmod +x sh"). Then go back to your python interpreter (with the
    patch applied) and run "os.get_shell_executable()". The returned
    result will be '/sh'.

    The loop _should_ be treating empty path elements as current working
    directory. (In the glibc sysdeps/posix/spawni.c file you pointed to
    look around line 281, by the comment that says "/* Two adjacent
    colons, or a colon at the beginning or the end of 'PATH' means to
    search the current directory.*/")

    But if you fix the loop to iterate over the path correctly (including
    'cwd') then you will find that putting an executable named 'sh' in the
    current working directory will make os.get_shell_executable() return
    the "sh" in the current working directory (because os.defpath says it
    should).

    Note also, that glibc's spawni() uses two different "default paths".
    One is for if the user didn't specify their PATH variable (that's the
    one where they use ":/bin:/usr/bin") and a completely different (built
    in path is used in the spawn*p* version) if it turns out that the file
    is a script that needs to run under the system sh. (On line 290 they
    call maybe_script_execute(), which calls script_execute(), which uses
    _PATH_BSHELL.)

    os.defpath is the default search path used by exec*p* and spawn*p*
    i.e., if os.defpath is incorrect for these system then os.exec*p* and
    os.spawn*p* functions could also be broken.

    I'll look into it.

    I don't remember exactly the motivation to use os.defpath instead of
    os.confstr('CS_PATH').

    The motivation was in msg224514. You wrote:

    In the absense of os.confstr('CS_PATH') on Android (msg175006),
    os.defpath seems appropriate than os.environ['PATH'] to expand 'sh'
    basename to the full path to be used by subprocess later.

    But os.defpath doesn't work on Android either (because it is hardcoded
    to ':/bin:/usr/bin').

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Nov 6, 2014

    Matt Frank added the comment:

    In msg230720 Akira Li (akira) wrote:
    > os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
    > used on Linux) sysdeps/posix/spawni.c I don't know whether it is
    > possible to change CS_PATH without recompiling every statically linked
    > executable on a system, sysdeps/unix/confstr.h:
    >
    > #define CS_PATH "/bin:/usr/bin"
    >
    > Though this issue is about the path to the standard shell sh/cmd.exe.
    > It is not about os.defpath.

    I understand this issue is about the path to the standard shell.
    My argument is that os.defpath is the wrong tool for finding that
    path, and os.confstr('CS_PATH') is the correct tool, as you originally
    suggested in msg224514.

    I'll repeat os.defpath definition that I've cited in the same message
    msg224514:

    The default search path used by exec*p* and spawn*p* if the
    environment doesn’t have a 'PATH' key. Also available via os.path.

    1. os.defpath should work (contain the path to the standard shell)
      everywhere where os.confstr("CS_PATH") works.

    2. And it might be easier to customize e.g., on systems where there is
      no os.confstr or where changing CS_PATH involves recompiling crucial
      system processes.

    If these two statements are not true then os.defpath could be dropped.

    > The patch [1] has tests. Have you tried to run them?
    > The tests *pass* at least on one system ;)
    >
    > [...]
    >
    > os.get_shell_executable() does not use cwd. Neither the documentation
    > nor the code in the patch suggest that.

    I ran the tests (and the entire Python test suite) before I wrote my
    first message. But you didn't test what happens when there is a bogus
    'sh' somewhere along the path. You also have a bug in your path
    searching loop that interprets an empty element on the path as "/".
    Here's the current failure mode:

    Go to root on your Posix system. With "su" or "sudo" create an
    executable file named sh. (for example "cd /; sudo touch sh; sudo
    chmod +x sh"). Then go back to your python interpreter (with the
    patch applied) and run "os.get_shell_executable()". The returned
    result will be '/sh'.
    But if you fix the loop to iterate over the path correctly (including
    'cwd') then you will find that putting an executable named 'sh' in the
    current working directory will make os.get_shell_executable() return
    the "sh" in the current working directory (because os.defpath says it
    should).

    Comment in the patch explicitly says that '' is interpreted as '/'. The
    intent is to exclude the current working directory, thinking that /sh
    executable can create only root. And root can do anything to the machine
    anyway.

    I agree. It is a misfeature. I've uploaded a new patch that excludes ''
    completely. The previous code tried to be too cute.

    The loop _should_ be treating empty path elements as current working
    directory. (In the glibc sysdeps/posix/spawni.c file you pointed to
    look around line 281, by the comment that says "/* Two adjacent
    colons, or a colon at the beginning or the end of 'PATH' means to
    search the current directory.*/")

    yes. Empty path element stands for the current working directory e.g.,

       $ PATH= python
       $ PATH=: python
       $ PATH=:/ python
       $ PATH=/: python

    run ./python on my system.

    The current working directory is intentionally excluded (in the original
    and in the current patches) otherwise os.get_exec_path(env={}) would be
    used.

    Note also, that glibc's spawni() uses two different "default paths".
    One is for if the user didn't specify their PATH variable (that's the
    one where they use ":/bin:/usr/bin") and a completely different (built
    in path is used in the spawn*p* version) if it turns out that the file
    is a script that needs to run under the system sh. (On line 290 they
    call maybe_script_execute(), which calls script_execute(), which uses
    _PATH_BSHELL.)

    glibc hardcodes the path (like subprocess module does currently):

      #define	_PATH_BSHELL	"/bin/sh"

    os.get_shell_executable() is an enhancement that allows
    (Unix) systems to configure the path via os.defpath.

    But os.defpath doesn't work on Android either (because it is hardcoded
    to ':/bin:/usr/bin').

    As I've mentioned in msg224514 before posting my original patch:

    • "Andriod uses /system/bin/sh"
    • "os.defpath could be tweaked on Android"

    i.e., yes. Default os.defpath doesn't work there and it should be
    configured on Android to include the path to the standard shell.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented May 23, 2016

    Following Serhiy suggestion at msg224477, this tentative patch adds the is_android attribute to the posix module and the default_shell attribute to the os module. No documentation for the moment. No test case is needed I think, as the changes in bpo-16255 will test any solution found here.

    An alternative to posix.is_android is sys.platform set to 'linux-android' on Android. These are the locations in the standard library that do not test for "if sys.platform.startswith('linux')":

    Lib/distutils/tests/test_dist.py|386 col 16| if sys.platform in ('linux', 'darwin'):
    Lib/test/test_logging.py|527 col 12| if sys.platform in ('linux', 'darwin'):
    Lib/test/test_resource.py|135 col 26| @unittest.skipUnless(sys.platform == 'linux', 'test requires Linux')
    Lib/test/test_socket.py|898 col 16| or sys.platform in ('linux', 'darwin')):
    Lib/test/test_socket.py|2256 col 23| @skipWithClientIf(sys.platform not in {"linux"},
    Lib/test/test_socket.py|4508 col 22| @unittest.skipUnless(sys.platform == 'linux', 'Linux specific test')
    Lib/test/test_sysconfig.py|388 col 26| @unittest.skipUnless(sys.platform == 'linux', 'Linux-specific test')

    @tiran tiran removed their assignment Jun 12, 2016
    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Jul 10, 2016

    bpo-27472 adds test.support.unix_shell as the path to the default shell.

    @aixtools
    Copy link
    Contributor

    aixtools commented Jul 26, 2016

    An interesting read, but I am lost in what the goal is.

    e.g., on AIX, which I know well, the system default is /bin/ksh (aka /usr/bin/ksh). However, /bin/sh (/usr/bin/sh) is available as well.

    My expectation is that on Linux the default shell is /bin/bash, and like AIX /bin/sh is also available.

    /bin/sh, /bin/ksh, /bin/bash are all (potentially) default shells. However, something every posix system is "sh" aka borne shell.

    Aside: windows is different, and I expect has a different syntax.

    So, if the goal is to GET the pathname of the so-called "default" shell -again, interesting - BUT - what does that buy me?

    For Popen or now subprocess I would want a consistent shell syntax, i.e., borne shell on posix, cmd.exe on windows, and whatever is correct on platforms that do not fit in these two.

    Hence, on posix platforms where /bin/sh does not exist (hard)code the correct path for that platform.

    FYI: CS_PATH does not exist on AIX

    IMHO: (as others have stated) having . in PATH is bad for security, being dependent on PATH opens other security concerns as well.

    IMHO2: KISS principle. After several years there is still no consenus, so make it simple - the popen and subprocess shell is one of /bin/sh, cmd.exe, or some other "hard-coded" shell.

    (Although I thought that subprocess called "program" directly. Python is still new, so if not applicable for subprocess - please ignore

    rereading, direct call would be when shell=False I expect.

    Anyway, my two cents.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Jul 27, 2016

    After several years there is still no consenus, so make it simple - the popen and subprocess shell is one of /bin/sh, cmd.exe, or some other "hard-coded" shell.

    I agree, the last patch that is being reviewed in bpo-16255 attempts to do that.

    @csabella
    Copy link
    Contributor

    csabella commented Feb 9, 2019

    3.7 added sys.getandroidapilevel() under bpo-28740. As Xavier mentioned in msg271439, that was used for subprocess.Popen in bpo-16255 and also merged as part of 3.7.

    I'm not sure if there is still interest in adding a default_shell to the os module using sys.androidapilevel() and the last batch that Xavier uploaded in 2016.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 9, 2019

    I think the functionality of getting either the system-wide default shell or the current user's default shell can be useful regardless of whether the specific subprocess issue was fixed.

    @pitrou pitrou added the 3.8 label Feb 9, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Oct 22, 2019

    This issue has been fixed. The subprocess module now uses '/system/bin/sh' by default on Android thanks to Xavier de Gaye!

    @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.8 stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests