This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Unsupported provider

classification
Title: subprocess.list2cmdline doesn't quote the & character
Type: behavior Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.4, Python 3.5, Python 2.7
process
Status: closed Resolution: not a bug
Dependencies: Superseder: Popen should raise ValueError if pass a string when shell=False or a list when shell=True
View: 7839
Assigned To: Nosy List: BreamoreBoy, SilentGhost, eric.smith, exarkun, ezio.melotti, gregory.p.smith, r.david.murray, shypike, vstinner
Priority: normal Keywords: patch

Created on 2010-06-11 13:11 by shypike, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
list2cmdline_ampersand_fix.patch shypike, 2010-06-12 16:58 patch file for subprocess.py and test_subprocess.py
list2cmdline_proper_fix.patch shypike, 2010-06-12 22:48
Messages (16)
msg107544 - (view) Author: (shypike) Date: 2010-06-11 13:11
subprocess.py/list2cmdline should also put double quotes around strings that contain ampersands (&), but no spaces.
If not, the Windows command processor will split the command into two separate parts. In short, '&' needs the same treatment as '|'.
msg107646 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-12 12:38
Thanks for reporting this issue.  Can you attach a patch which adds a unit test covering this behavior and fixing the quoting rules?  It would be very helpful in resolving this ticket.  If implementing the whole thing is too much work, if you could just provide a failing unit test that would still be greatly helpful.

Thanks again.
msg107672 - (view) Author: (shypike) Date: 2010-06-12 16:58
Added patch file for subprocess.py and test_subprocess.py.
msg107679 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-12 18:15
Thanks.

I'm not sure this is a correct change.  And in fact, I would say that the current quoting of | is also incorrect.

& and | (and ^ and perhaps several others) have special meaning to cmd.exe.  list2cmdline is documented as applying the quoting rules which the "MS C runtime" uses: cmd.exe and the C runtime are different and have different rules.

It seems to me that whoever added the | handling to list2cmdline was confused about the purpose of this function, or failed to properly document the function.

It would make more sense to document list2cmdline as applying cmd.exe-style quoting rules, if those are the rules it is actually going to implement.

A better option, though, would probably be to implement the cmd.exe quoting rules in a different function from the MS C runtime rules.

This all might benefit from a sanity check from someone who's actually worked with the subprocess module before, though (ie, not me).
msg107680 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-12 18:25
http://www.autohotkey.net/~deleyd/parameters/parameters.htm#WINCRULES is a helpful reference, by the way.
msg107713 - (view) Author: (shypike) Date: 2010-06-12 22:48
A work-around could be that the caller puts double quotes around the individual elements of the sequence that need it.
However, this won't work because list2cmdline doesn't handle backslash quoting properly. An element like r'"foo"' is translated to r'\"foo\"'. This is incorrect because cmd.exe cannot handle this. The backslash may be appropriate for embedded quotes (like in r'foo"bar'), but not for outer quotes (like in r'"foobar"').

The user shouldn't have to worry with adding quotes anyway, so it would be better to demand that '|' and '&' are passed as separate elements in the sequence. Example ['echo', 'foo', '&', 'bar'].
When someone passes ['echo', 'foo&bar'], it is very obvious that r'echo "foo&bar"' is expected and not r'echo foo & bar'.

I have added a patch for this way of working of list2cmdline.
msg107721 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-13 03:06
I'm not sure my last message was clear.

> A work-around could be that ...

What problem is being worked around?
msg107752 - (view) Author: (shypike) Date: 2010-06-13 21:09
The discussion is going the wrong way. Let me state what is the actual problem.
When the Popen() function is called with "shell=True", you cannot pass command line elements in which the characters '&' and '|' are embedded (example r'Q&A'). list2cmdline should embed such elements in double quotes '"' in the same way as when a space is detected (so r'"Q&A"').
When '&' and '|' require their special meaning, they should be passed as separate elements (r'&') and in that case list2cmdline should not double-quote them.

The whole C++ discussion was only invoked because I said a potential work-around doesn't work because the way list2cmdline is designed. That remark is not relevant, because the actual problem is different.
msg107768 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-14 00:59
> That remark is not relevant, because the actual problem is different.

Maybe you can expand the test case to demonstrate the actual problem?  The tests in the latest patch are for list2cmdline directly.  But you can't observe the problem a bug in list2cmdline causes until you actually try to launch a child process.  So perhaps the test should do that?
msg107769 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-06-14 01:55
The actual bug here is that list2cmdline is called when shell=True and a list is passed.  This should not be done.  Instead, Popen should raise an TypeError (see issue 7839).

When calling Popen with shell=True, you should be passing in a string that is already correctly quoted.  This is how it works in Unix, and is how it should work in Windows as well.

So I agree with exarkun's comments in msg107679, and I think Gregory's fix in r60115 for issue 1300 was incorrect and that in fact the OP was correct that he did not understand what was happening (note that he did not report an actual bug, he just reported that the code looked wrong to him).
msg108131 - (view) Author: (shypike) Date: 2010-06-18 19:45
I see your point about passing a command line as a single string instead of a list. Too bad that Popen doesn't just do the obvious thing (assemble the list into a string in a shell/cmd.exe compatible way).

Issue 1300 should indeed be reversed.

Raising ValueError will result in breaking some existing programs.
msg108135 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-18 20:11
I've reverted the issue1300 revision from 2.6, 2.7, 3.1, and 3.2.  I hope 
7839 is resolved soon.
msg108162 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-06-19 03:09
The reason that Popen doesn't do "the obvious thing" with a list and shell=True is that it is far from obvious what the correct thing is to do, especially for windows.

Thanks for the revert, exarkun.
msg128127 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2011-02-07 15:35
issue11139 was closed as a duplicate of this issue.
msg222887 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-12 21:46
I believe this is still valid in which case could we have the stage and resolution fields set appropriately please.  You might also like to take a look at issue 13238 which is referred to by issue 7839.
msg223128 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-07-15 17:33
The problem pointed to by this report was resolved by the revert.  The issue of what to do about a list passed with shell=True is addressed (with no consensus) by issue 7839.  A proposal to add a windows equivalent of shlex.quote has been floated, but no takers have come forward to implement one, as far as I know.

There's nothing left to do here in this issue, as far as I can see.
History
Date User Action Args
2022-04-11 14:57:02adminsetgithub: 53218
2014-07-15 17:33:05r.david.murraysetstatus: open -> closed

messages: + msg223128
2014-07-12 21:55:20brian.curtinsetnosy: - brian.curtin
2014-07-12 21:46:07BreamoreBoysetnosy: + BreamoreBoy

messages: + msg222887
versions: + Python 3.4, Python 3.5, - Python 2.6
2011-03-03 13:19:36vstinnersetnosy: + vstinner
2011-02-07 15:38:30r.david.murraylinkissue11139 superseder
2011-02-07 15:35:17SilentGhostsetnosy: + SilentGhost
messages: + msg128127
2010-06-19 03:09:45r.david.murraysetmessages: + msg108162
2010-06-18 20:11:29exarkunsetmessages: + msg108135
2010-06-18 19:45:55shypikesetstatus: pending -> open

messages: + msg108131
2010-06-14 01:55:21r.david.murraysetstatus: open -> pending

superseder: Popen should raise ValueError if pass a string when shell=False or a list when shell=True

nosy: + r.david.murray, gregory.p.smith
messages: + msg107769
resolution: not a bug
stage: resolved
2010-06-14 00:59:50exarkunsetmessages: + msg107768
2010-06-13 21:09:32shypikesetmessages: + msg107752
2010-06-13 19:55:04eric.smithsetnosy: + eric.smith
2010-06-13 03:06:54exarkunsetmessages: + msg107721
2010-06-12 22:48:20shypikesetfiles: + list2cmdline_proper_fix.patch

messages: + msg107713
2010-06-12 18:25:50exarkunsetmessages: + msg107680
2010-06-12 18:25:16brian.curtinsetnosy: + brian.curtin
2010-06-12 18:16:19exarkunsetnosy: exarkun, ezio.melotti, shypike
components: + Library (Lib)
stage: test needed -> (no value)
2010-06-12 18:15:37exarkunsetmessages: + msg107679
2010-06-12 16:58:11shypikesetfiles: + list2cmdline_ampersand_fix.patch
keywords: + patch
messages: + msg107672
2010-06-12 13:22:55ezio.melottisetnosy: + ezio.melotti
stage: test needed

versions: + Python 2.7, - Python 2.5
2010-06-12 12:38:23exarkunsetnosy: + exarkun
messages: + msg107646
2010-06-11 13:11:04shypikecreate