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

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

Open
anishathalye mannequin opened this issue May 1, 2020 · 3 comments
Open

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

anishathalye mannequin opened this issue May 1, 2020 · 3 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows stdlib Python modules in the Lib dir topic-subprocess Subprocess issues. type-bug An unexpected behavior, bug, or error

Comments

@anishathalye
Copy link
Mannequin

anishathalye mannequin commented May 1, 2020

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:

assignee = None
closed_at = None
created_at = <Date 2020-05-01.13:34:17.815>
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 = <Date 2021-12-04.09:14:59.402>
user = 'https://github.com/anishathalye'

bugs.python.org fields:

activity = <Date 2021-12-04.09:14:59.402>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'Windows']
creation = <Date 2020-05-01.13:34:17.815>
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']

@anishathalye
Copy link
Mannequin Author

anishathalye mannequin commented May 1, 2020

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:

args = '{} /c "{}"'.format (comspec, args)

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:
anishathalye/dotbot#219

@anishathalye anishathalye mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir OS-windows type-bug An unexpected behavior, bug, or error labels May 1, 2020
@snoopyjc
Copy link
Mannequin

snoopyjc mannequin commented Dec 4, 2021

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
Copy link
Contributor

eryksun commented Dec 4, 2021

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

@eryksun eryksun added 3.10 only security fixes 3.11 only security fixes and removed 3.7 (EOL) end of life labels Dec 4, 2021
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@vstinner vstinner added the topic-subprocess Subprocess issues. label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows stdlib Python modules in the Lib dir topic-subprocess Subprocess issues. type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants