python / cpython

The Python programming language
https://www.python.org
Other
62.61k stars 30.05k forks source link

subprocess: replacement shell on windows with executable="..." arg #84647

Open 3171633f-d6c6-4f82-bb53-fbbe90b14658 opened 4 years ago

3171633f-d6c6-4f82-bb53-fbbe90b14658 commented 4 years ago
BPO 40467
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @snoopyjc, @anishathalye

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: ```python assignee = None closed_at = None created_at = labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', 'library', 'OS-windows'] title = 'subprocess: replacement shell on windows with executable="..." arg' updated_at = user = 'https://github.com/anishathalye' ``` bugs.python.org fields: ```python activity = actor = 'eryksun' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)', 'Windows'] creation = creator = 'anishathalye' dependencies = [] files = [] hgrepos = [] issue_num = 40467 keywords = [] message_count = 3.0 messages = ['367844', '407644', '407647'] nosy_count = 7.0 nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'snoopyjc', 'anishathalye'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue40467' versions = ['Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11'] ```

3171633f-d6c6-4f82-bb53-fbbe90b14658 commented 4 years ago

On Windows, using subprocess.call() and specifying both shell=True and the executable='...' keyword arguments produces an undesirable result when the specified shell is a POSIX-like shell rather than the standard cmd.exe.

I think the documentation is unclear on the semantics of Popen() when both shell=True and executable= are specified on Windows. It does say the following about POSIX systems:

If shell=True, on POSIX the executable argument specifies a replacement shell for the default /bin/sh.

But the documentation doesn't say anything about Windows in this scenario, so I'm not sure if this is a bug, or if it's undefined behavior.

Concretely, here's an example program that fails due to this:

    import subprocess
    bash = 'C:\\Program Files\\Git\\usr\\bin\\bash.exe'
    subprocess.call('f() { echo test; }; f', shell=True, executable=bash)

It prints out this mysterious-looking error:

/c: /c: Is a directory

Tracing this into subprocess.py, it looks like it's because the executable bash (as specified) is being called with the argv that's approximately ['cmd.exe', '/c', 'f() { echo test; }; f'] (and the program being launched is indeed bash).

Bash doesn't expect a '/c' argument, it wants a '-c' there.

The problematic code in subprocess.py is here: https://github.com/python/cpython/blob/1def7754b7a41fe57efafaf5eff24cfa15353444/Lib/subprocess.py#L1407 If the '/c' is replaced with a '-c', the example program above works (bash doesn't seem to care that it's called with an argv[0] that doesn't make sense, though presumably that should be fixed too).

I'm not sure how this could be fixed. It's unclear when '/c' should be used, as opposed to '-c'. Doing it based on the contents of the executable= argument or the SHELL environment variable or COMSPEC might be fragile? I couldn't find much about this online, but I did find one project (in Ruby) that seems to have run into a similar problem. See https://github.com/kimmobrunfeldt/chokidar-cli/issues/15 and https://github.com/kimmobrunfeldt/chokidar-cli/pull/16.

At the very least, even if this isn't fixed / can't be fixed, it might be nice if it's possible to give some sort of useful warning/error when this happens -- perhaps say that specifying both shell=True and executable="..." isn't supported on Windows?

I ran into this issue while while debugging an issue in a project of mine. In case the additional context is useful, here is the discussion: https://github.com/anishathalye/dotbot/issues/219

2cfedb62-c387-4430-a0e4-94fc2399097a commented 2 years ago

Proposed solution:

                if comspec.endswith('sh.exe') or comspec.endswith('sh'):    # issue 40467
                    args = '{} -c "{}"'.format (comspec, args)              # issue 40467
                else:                                                       # issue 40467
                    args = '{} /c "{}"'.format (comspec, args)
eryksun commented 2 years ago

it might be nice if it's possible to give some sort of useful warning/error when this happens -- perhaps say that specifying both shell=True and executable="..." isn't supported on Windows?

The shell parameter is documented as follows for Windows:

On Windows with shell=True, the COMSPEC environment variable 
specifies the default shell. The only time you need to specify 
shell=True on Windows is when the command you wish to execute is 
built into the shell (e.g. dir or copy). You do not need
shell=True to run a batch file or console-based executable.

It wouldn't hurt to clarify that the COMSPEC shell has to support /c. This is required by CreateProcessW(), which uses COMSPEC to run BAT and CMD files.

The discussion about using executable with shell could be extended for Windows. But this would also require new behavior. For example:

Original:

    if shell:
        startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW
        startupinfo.wShowWindow = _winapi.SW_HIDE
        comspec = os.environ.get("COMSPEC", "cmd.exe")
        args = '{} /c "{}"'.format (comspec, args)

Proposed:

    if shell:
        startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW
        startupinfo.wShowWindow = _winapi.SW_HIDE
        if executable is not None:
            cmd = executable
        else:
            cmd = os.path.normpath(os.environ.get("COMSPEC", "cmd.exe"))
            if "\\" in cmd:
                executable = cmd
        args = '"{}" /c "{}"'.format(cmd, args)

if comspec.endswith('sh.exe') or comspec.endswith('sh'): args = '{} -c "{}"'.format (comspec, args)

sh is not a valid COMSPEC shell. To use sh automatically, subprocess would have to support and prefer the SHELL [1] environment variable in Windows -- and in POSIX for that matter.

--- [1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03