[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Martin Panter added the comment: I think it might be better to leave the platform-specific details in the full Popen description, not under Frequently Used Arguments. I suggest to use 20344_4.patch: * Move existing pointer to Popen constructor details up to top of section * Explain the escaping and quoting of a sequence only applies to Windows * Remove claim that a string simply names a program without arguments; appears to be only half true under Windows * Clarify shell=True command is normally a string, not a sequence * Drop new description of Unix shell argument sequence in favour of existing description under Popen * Move warning about Windows shell argument sequence under Popen -- stage: needs patch - patch review versions: +Python 3.5, Python 3.6 -Python 3.3 Added file: http://bugs.python.org/file39747/20344_4.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Martin Panter added the comment: Actually David I didn’t notice your suggested wording https://bugs.python.org/issue20344#msg214364 before. Adding that last sentence, pointing to the full description of the Popen constructor, would be fine. My complaint about mentioning “iterable” is that iterables are not mentioned anywhere else in the documentation. I would rather leave it out or change it to “sequence”, unless the rest of the documentation is made consistent, e.g. “args . . . should be a string, or a sequence of program arguments” would also need fixing. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
R. David Murray added the comment: Can you suggest a better phrasing that is still succinct? (Maybe just 'appear to be executed' would make it accurate enough?) But, that's why there needs to be a reference of *some* sort to the full explanation. I'm not sure what the issue is with iterable/sequence. check_output/Popen accepts an iterable. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Martin Panter added the comment: “Only the first element . . . will be executed on Unix” is a bit simplistic. The behaviour is already described more fully in the “shell” documentation under https://docs.python.org/dev/library/subprocess.html#subprocess.Popen. Also, “args” is allowed to be a _sequence_, but an _iterable_ is a more general concept. -- nosy: +vadmium ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Tobias Klausmann added the comment: Hi! On Tue, 25 Mar 2014, Tuomas Savolainen wrote: Created a patch that adds notice of using shell=True and iterable to the documentation. Please do comment if the formatting is wrong (this my first documentation patch). I'd use articles, i.e. and a list and does not raise an error Also, s/except/expect/ Regards, Tobias -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
R. David Murray added the comment: Also, the see below sentence is missing. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Tuomas Savolainen added the comment: Corrected the spelling of the patch. -- Added file: http://bugs.python.org/file34635/20344_3.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Tuomas Savolainen added the comment: Created a patch that adds notice of using shell=True and iterable to the documentation. Please do comment if the formatting is wrong (this my first documentation patch). -- Added file: http://bugs.python.org/file34619/documentation20344.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
Tuomas Savolainen added the comment: Made a patch that throws exception as suggested: 3- Make check_output() throw an Exception if the first argument is a list and shell=True -- keywords: +patch nosy: +Tuomas.Savolainen Added file: http://bugs.python.org/file34543/issue20344.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
R. David Murray added the comment: Thanks Tuomas, but we don't have any consensus that that kind of change will be accepted. It's just my opinion that it should be...and if it was, it would have to start with a deprecation, not raising an exception. What we need as a patch for this issue is a documentation patch explaining why using shell=True with a list is a bad idea in the 'frequently used arguments' section. The paragraph about 'args' in the frequently used arguments section should say something like 'using shell=True with a list containing more than one element does not currently raise an error, but it will not do what you expect; only the first element of the list will be executed on unix, and on windows the quoting is likely to be incorrect. See the Popen constructor documentation below for a more precise description.' (You can see why I think this usage should be deprecated, I hope ;) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
New submission from Tobias Klausmann: The subprocess docs state that the first argument can be either a string or an iterable that contains the program and arguments to run. It also points out that using shell=True allows for shell constructs. It does not mention a subtlety that is introduced by Python's use of sh -c (on Unix): Using a string that contains the full command line with args and shell=False breaks as expected: subprocess.check_output(ls foo, shell=False) Traceback (most recent call last): File stdin, line 1, in module File /usr/lib64/python3.3/subprocess.py, line 576, in check_output with Popen(*popenargs, stdout=PIPE, **kwargs) as process: File /usr/lib64/python3.3/subprocess.py, line 824, in __init__ restore_signals, start_new_session) File /usr/lib64/python3.3/subprocess.py, line 1448, in _execute_child raise child_exception_type(errno_num, err_msg) FileNotFoundError: [Errno 2] No such file or directory: 'ls foo' Using shell=True instead works as expected (since foo does not exist, ls exits with non-0 and a different Exception is raised. This, too is to spec and exactly what the docs describe. subprocess.check_output(ls foo, shell=True) ls: cannot access foo: No such file or directory Traceback (most recent call last): File stdin, line 1, in module File /usr/lib64/python3.3/subprocess.py, line 589, in check_output raise CalledProcessError(retcode, process.args, output=output) subprocess.CalledProcessError: Command 'ls foo' returned non-zero exit status 2 Using shell=False and making the command a list of command and args does the same thing: subprocess.check_output([ls, foo], shell=False) ls: cannot access foo: No such file or directory Traceback (most recent call last): File stdin, line 1, in module File /usr/lib64/python3.3/subprocess.py, line 589, in check_output raise CalledProcessError(retcode, process.args, output=output) subprocess.CalledProcessError: Command '['ls', 'foo']' returned non-zero exit status 2 But using an iterable _and_ requesting a shell results in something very broken: subprocess.check_output([ls, foo], shell=True) [contents of my homedir are returned] Note that the argument foo completely disappears, apparently. strace() reveals that foo is never added to the call to ls. Instead, it becomes $0 in the shell that ls runs in. This is exactly to spec (i.e. bash is not broken). bash -c foo bar baz will start a shell that sets $0 to bar, $1 to baz and then executes foo. Whereas bash -c 'foo bar baz' will run 'foo bar baz'. I think there are three possible fixes here: 1- Make check_output(list, shell=True) run something like bash -c '%s' % .join(list) 2- Change the docs to warn of the unintended consequences of combining lists with shell=True 3- Make check_output() throw an Exception if the first argument is a list and shell=True The first option would make it easiest for people to just use it, but I fear the string manipulation may become the source of bugs with security implications in the future. The second option keeps the status quo, but people tend to not read docs, so it may not be as effective as one would like, especially since shell-True already has warnings and people tend to go yeah I know about unverified input, but it's not a problem for me and then ignore both warnings. Option 3 has the disadvantage that existing scripts that _expect_ the behavior may break. -- assignee: docs@python components: Documentation, Library (Lib) messages: 208811 nosy: docs@python, klausman priority: normal severity: normal status: open title: subprocess.check_output() docs misrepresent what shell=True does type: behavior versions: Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20344] subprocess.check_output() docs misrepresent what shell=True does
R. David Murray added the comment: This was discussed in issue 6760. There I advocated (and still advocate) option (3). Unfortunately it looks like our doc update in that issue actually *lost* the documentation for your option (2) that used to be there, though it wasn't exactly clear what it meant when it was there. So IMO we should do a doc update to add that info back in a clearer form, and also do a deprecation cycle to disallow shell=True with a sequence. I haven't ever gotten consensus from the other devs on the latter, though. -- nosy: +r.david.murray stage: - needs patch versions: +Python 2.7, Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20344 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
