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._execute_child doesn't accept a single PathLike argument for args #76142

Closed
rowillia mannequin opened this issue Nov 6, 2017 · 34 comments
Closed

subprocess._execute_child doesn't accept a single PathLike argument for args #76142

rowillia mannequin opened this issue Nov 6, 2017 · 34 comments
Assignees
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@rowillia
Copy link
Mannequin

rowillia mannequin commented Nov 6, 2017

BPO 31961
Nosy @gpshead, @ned-deily, @bitdancer, @njsmith, @serhiy-storchaka, @eryksun, @Phaqui, @rowillia, @csabella
PRs
  • bpo-31961: subprocess.run() now accepts path-like args #4329
  • Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" #5912
  • bpo-31961: Fix support of path-like executables in subprocess. #5914
  • [3.7] Revert "bpo-31961: subprocess now accepts path-like args" (GH-5912) #5931
  • 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/gpshead'
    closed_at = <Date 2019-08-02.22:39:24.649>
    created_at = <Date 2017-11-06.17:27:25.843>
    labels = ['3.8', 'type-feature', 'library']
    title = "subprocess._execute_child doesn't accept a single PathLike argument for args"
    updated_at = <Date 2019-08-02.22:39:24.649>
    user = 'https://github.com/rowillia'

    bugs.python.org fields:

    activity = <Date 2019-08-02.22:39:24.649>
    actor = 'steve.dower'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2019-08-02.22:39:24.649>
    closer = 'steve.dower'
    components = ['Library (Lib)']
    creation = <Date 2017-11-06.17:27:25.843>
    creator = 'Roy Williams'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31961
    keywords = ['patch']
    message_count = 34.0
    messages = ['305663', '305669', '305710', '305802', '311247', '311250', '311604', '311611', '311639', '311645', '311675', '311683', '311695', '311697', '311698', '311715', '311730', '311733', '311734', '311737', '311775', '311948', '312859', '312958', '312960', '312962', '312966', '313033', '313035', '313038', '330496', '331062', '342778', '343814']
    nosy_count = 9.0
    nosy_names = ['gregory.p.smith', 'ned.deily', 'r.david.murray', 'njs', 'serhiy.storchaka', 'eryksun', 'Phaqui', 'Roy Williams', 'cheryl.sabella']
    pr_nums = ['4329', '5912', '5914', '5931']
    priority = None
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31961'
    versions = ['Python 3.8']

    @rowillia
    Copy link
    Mannequin Author

    rowillia mannequin commented Nov 6, 2017

    Repro:

    from pathlib import Path
    import subprocess
    
    subprocess.run([Path('/bin/ls')])  # Works Fine
    subprocess.run(Path('/bin/ls'))  # Fails

    The problem seems to originate from here:
    https://github.com/python/cpython/blob/master/Lib/subprocess.py#L1237-L1240

    This file auspiciously avoids importing pathlib, which I'm guessing is somewhat intentional? Would the correct fix be to check for the existence of a __fspath__ attribute as per https://www.python.org/dev/peps/pep-0519/ ?

    @rowillia rowillia mannequin added 3.8 only security fixes 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels Nov 6, 2017
    @rowillia
    Copy link
    Mannequin Author

    rowillia mannequin commented Nov 6, 2017

    Ignore my comment re: pathlib, it looks like PathLike is defined in os and not pathlib.

    @phaqui
    Copy link
    Mannequin

    phaqui mannequin commented Nov 7, 2017

    I was able to make a test that reproduces your code, and expectedly fails. Also implemented a fix for it. See a temporary diff here: https://pastebin.com/C9JWkg0i

    However, there is also a specific MS Windows version of _execute_child() (a phrase only computer-folks can say out loud without feeling very...wrong...). It uses a different method to extract and parse arguments, and it should be corrected and tested under windows, before I submit a PR for this.

    Also, my test messes up the formatting. Instead of saying "....ok", they both say "... total 0\nok". I have no idea why.

    @phaqui
    Copy link
    Mannequin

    phaqui mannequin commented Nov 8, 2017

    While researching this, I discovered that on MS Windows

    >> subprocess.run([pathlike_object, additional_arguments])

    did not run like it did on Posix. My PR includes this problem and it's fix.

    @gpshead
    Copy link
    Member

    gpshead commented Jan 30, 2018

    New changeset dd42cb7 by Gregory P. Smith (Anders Lorentsen) in branch 'master':
    bpo-31961: subprocess now accepts path-like args (GH-4329)
    dd42cb7

    @gpshead gpshead removed the 3.8 only security fixes label Jan 30, 2018
    @gpshead gpshead self-assigned this Jan 30, 2018
    @gpshead
    Copy link
    Member

    gpshead commented Jan 30, 2018

    Thanks for the PR! I accepted it as this makes sense as a feature.

    <meme> pathlib all the things! </meme>

    I think the code be polished up a bit to not rely on catching TypeError but this gets the feature in before the 3.7 feature freeze (just). If further changes are needed or bugs are found, they can be addressed during the 3.7 betas. :)

    @gpshead gpshead closed this as completed Jan 30, 2018
    @gpshead gpshead added the type-feature A feature request or enhancement label Jan 30, 2018
    @serhiy-storchaka
    Copy link
    Member

    This made tests failing on Windows. See bpo-32764.

    @serhiy-storchaka
    Copy link
    Member

    Actually this feature looks wrong to me. The args argument is either a sequence containing a program name and arguments, or a command line string. In the first case supporting path-like objects makes sense, and this was supported. But the command line is not a path-like object. It is a concatenation of quoted program name and arguments. Neither operation for path-like objects makes sense for a command line.

    I think this change should be reverted.

    @gpshead
    Copy link
    Member

    gpshead commented Feb 5, 2018

    Don't revert something just because you found a bug, we can fix it. fwiw, the PR passed appveyor's Windows run: https://ci.appveyor.com/project/python/cpython/build/3.7build11551

    So if there's a bug, we're missing some kind of test coverage.

    @serhiy-storchaka
    Copy link
    Member

    The bug itself can be easily fixed. But I think this PR shouldn't be merged at first place. Not all functions should accept path-like objects for arbitrary arguments. Only if the argument semantically is a path, a path-like object should be accepted. Several similar propositions already were rejected for this reason.

    @gpshead
    Copy link
    Member

    gpshead commented Feb 5, 2018

    got any pointers to those? I want to familiarize myself with existing arguments for/against such a feature in subprocess to decide.

    @serhiy-storchaka
    Copy link
    Member

    I don't remember all details, here is what I have found.

    bpo-28230 and bpo-28231 -- the support of path-like object was added only for external names (paths of archives, paths of added files, paths for extraction), but not for internal names.

    bpo-29447 -- the initial idea was rejected, and it seems to me that the tempfile module doesn't need any changes. Path-like objects already are accepted for the dir argument, but they shouldn't be accepted for arguments like pre or suf which are not paths.

    bpo-29448 is only tangentially related. It's rejection is based on bpo-29447.

    @njsmith
    Copy link
    Contributor

    njsmith commented Feb 5, 2018

    I think I agree with Serhiy here... the whole difference between run([a]) and run(a) is that in the first case 'a' is treated as a path, and in the second it isn't. This distinction long predates pathlib.

    Say I have an oddly named binary with an embedded space in its name:

    bin = Path("/bin/ls -l")

    # Check that this weird file actually exists:
    assert bin.exists()

    # runs this weird file
    subprocess.run([bin])
    
    # Currently an error; if this is implemented, would run
    # /bin/ls, and pass it the -l argument. Refers to something
    # completely different than our .exists() call above.
    subprocess.run(bin)

    @gpshead
    Copy link
    Member

    gpshead commented Feb 5, 2018

    I agree with both of you at this point.

    TODO: Revert the PR and backport that to the 3.7 branch (i'll take care of it)

    @gpshead gpshead added 3.8 only security fixes release-blocker labels Feb 5, 2018
    @phaqui
    Copy link
    Mannequin

    phaqui mannequin commented Feb 5, 2018

    runs this weird file

    subprocess.run([bin])

    Currently an error; if this is implemented, would run

    /bin/ls, and pass it the -l argument. Refers to something

    completely different than our .exists() call above.

    I do not understand where it is stated that this is the behavior. My understanding was that if run() is passed a single argument, it will interpret the string "/bin/ls -l" to mean "run the /bin/ls program, and pass it one argument -l, whereas if instead run() is given a list, it will literally treat the first element as the program to run, regardless of how many spaces or whatever else it contains ... And as such, if bin is a Path-like object, this issue tries to resolve that
    run([bin]) works as excepted, but run(bin) fails horribly.

    Sure, I can understand that for strings it may not feel natural how these two things are interpreted differently, but if bin is a Path-like object, it at least feels very natural to me that then it should be run as the program name itself (as "paths" does not have arguments).

    @serhiy-storchaka
    Copy link
    Member

    Anders, the problem is that running subprocess.run(executable) instead of subprocess.run([executable]) (where executable is a path to a program) is a bug. For example it doesn't work if the path contains spaces. And PR 4329 encourages making this bug.

    @phaqui
    Copy link
    Mannequin

    phaqui mannequin commented Feb 6, 2018

    What do you mean "is a bug", and "the PR would encourage this"? Can't it be fixed?

    Are you saying that just because it is a bug now, we should be discouraged from making it work in the way you'd expect it to work?

    If exe is a path, these two lines of code should do exactly the same, right? It seems unintuitive to me if they produce different results.

    run(exe)
    run([exe])

    @gpshead
    Copy link
    Member

    gpshead commented Feb 6, 2018

    Nathaniel's specific description of a problem is wrong. A Path with embedded spaces is treated no different than one without by default.

    Where things change, and what needs fixing, is when shell=True is passed. In that case a PathLike object should not be allowed as it will be turned into a flat string passed to the shell for parsing.

    # I've created an executable bash script called "x/mybin -h" for this that just does: echo "hello from mybin $*"

    >>> run("x/mybin -h", shell=True)
    /bin/sh: 1: x/mybin: not found
    CompletedProcess(args='x/mybin -h', returncode=127)
    >>> run("x/mybin -h")
    hello from mybin 
    CompletedProcess(args='x/mybin -h', returncode=0)
    >>> run(Path("x/mybin -h"), shell=True)
    /bin/sh: 1: x/mybin: not found
    CompletedProcess(args=PosixPath('x/mybin -h'), returncode=127)
    >>> run([Path("x/mybin -h")], shell=True)
    /bin/sh: 1: x/mybin: not found
    CompletedProcess(args=[PosixPath('x/mybin -h')], returncode=127)
    >>> run(Path("x/mybin -h"))
    hello from mybin 
    CompletedProcess(args=PosixPath('x/mybin -h'), returncode=0)
    >>> run([Path("x/mybin -h")])
    hello from mybin

    @serhiy-storchaka
    Copy link
    Member

    Oh, my mistake. I thought the case when args is a string and shell=False is deprecated.

    @bitdancer
    Copy link
    Member

    "I thought the case when args is a string and shell=False is deprecated."

    IMO it ought to be :) See bpo-7839 for background.

    @eryksun
    Copy link
    Contributor

    eryksun commented Feb 7, 2018

    If a PathLike args value is supported in Windows, then it must be processed through list2cmdline, in case it needs to be quoted. Also, the preferred usage is to pass args as a list when shell is false. This common case shouldn't be penalized as a TypeError. Also, passing executable as PathLike should be supported, as is already the case in the POSIX implementation. For example.

    _execute_child:

        if executable is not None and not isinstance(executable, str):
            executable = os.fsdecode(executable)
    
        if not isinstance(args, str):
            try:
                args = list2cmdline(args)
            except TypeError:
                if isinstance(args, bytes):
                    args = os.fsdecode(args)
                elif isinstance(args, os.PathLike):
                    args = list2cmdline([args])
                else:
                    raise
            
    list2cmdline should support PathLike arguments via os.fsdecode. This removes an existing inconsistency since the POSIX implementation converts args via PyUnicode_FSConverter in Modules/_posixsubprocess.c. For example:

    list2cmdline:

        for arg in seq:
            if not isinstance(arg, str):
                arg = os.fsdecode(arg)

    Finally, passing args as a string when shell is false can never be deprecated on Windows. list2cmdline works in most cases, but Windows CreateProcess takes a command line, and applications are free to parse the command line however they want. IMHO, the case that should be deprecated is passing args as a list/sequence when shell is true.

    @serhiy-storchaka
    Copy link
    Member

    This is a complex issue for doing it right. Related issues -- supporting bytes paths on Windows, supporting path-like executable, deprecating some corner cases. I suggest to revert this change now and try to do it right in 3.8.

    @ned-deily
    Copy link
    Member

    What's the status of this issue? We need to make a decision soon about what to do for 3.7.0.

    @gpshead
    Copy link
    Member

    gpshead commented Feb 26, 2018

    I'm fine with undoing this for 3.7 in light of the many things we don't do "right" all over the place with path like objects.

    subprocess still presents as a mix of high and low level APIs so if we accept a path like object in a subset of places it seems like that'll add complexity to the APIs rather than being consistent.

    @gpshead
    Copy link
    Member

    gpshead commented Feb 26, 2018

    Ned: when removing a feature added in beta1, should we remove the original NEWS entry created for it? Or add a new NEWS entry that states that the previous feature has been reverted?

    @ned-deily
    Copy link
    Member

    We should either remove the entry in Misc/NEWS/3.7.0b1.rst or, perhaps better, add a line to it noting that it was removed in 3.7.0b2.

    @serhiy-storchaka
    Copy link
    Member

    There are two alternate PRs. PR 5912 removes this feature. PR 5914 fixes it.

    @ned-deily
    Copy link
    Member

    New changeset be50a7b by Ned Deily (Serhiy Storchaka) in branch 'master':
    Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" (bpo-5912)
    be50a7b

    @ned-deily
    Copy link
    Member

    New changeset b7dcae3 by Ned Deily (Miss Islington (bot)) in branch '3.7':
    Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" (GH-5912) (GH-5931)
    b7dcae3

    @ned-deily
    Copy link
    Member

    Thanks for everyone's input and thanks for the PRs! Since there are still outstanding review comments, I decided to revert this from both master and 3.7 for 3.7.0b2. I would suggest getting a polished version stabilized in master for 3.8.0. We could then discuss the possibility of a 3.7 backport.

    @serhiy-storchaka
    Copy link
    Member

    Gregory, could you please make a look at PR 5914?

    Differences from PR 4329:

    • Any item of args can be an iterable of bytes and path-like objects on Windows (not just a first item as in PR 4329).
    • Accepts bytes and path-like objects as executable on Windows.
    • Accepts bytes as cwd on Windows.
    • Raises a TypeError when shell is true and args is a path-like object (or bytes on Windows).
    • Does not use raising and catching a TypeError when args is an iterable on Windows or a path-like object on POSIX (this makes the code clearer and a tiny bit more efficient).

    @serhiy-storchaka
    Copy link
    Member

    Ping.

    @csabella
    Copy link
    Contributor

    gregory.p.smith, I believe Serhiy was looking for a review on PR5914. Maybe it's not too late for this to get into 3.8? Thanks!

    @serhiy-storchaka
    Copy link
    Member

    New changeset 9e3c452 by Serhiy Storchaka in branch 'master':
    bpo-31961: Fix support of path-like executables in subprocess. (GH-5914)
    9e3c452

    @zooba zooba closed this as completed Aug 2, 2019
    @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 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants