Opened 12 years ago

Closed 8 years ago

Last modified 8 years ago

#1123 defect closed duplicate (duplicate)

win32reactor spawnProcess incorrectly escapes cmd line

Reported by: justinj Owned by:
Priority: high Milestone:
Component: core Keywords: win32
Cc: Glyph, Jean-Paul Calderone, itamarst, justinj, jknight, osuchw, bwh, teratorn Branch:
Author:

Description


Attachments (1)

cmdline_quote.patch (655 bytes) - added by osuchw 12 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 12 years ago by justinj

Recent changes to win32eventreactor.py in _cmdLineQuote incorrectly replace 
all slashes (\) with double slashes (\\).  The attempt of the code is to deal 
with MS command line goofiness as defined at 
http://msdn.microsoft.com/library/default.asp?url=/library/en-
us/vclang/html/_pluslang_Parsing_C.2b2b_.Command.2d.Line_Arguments.asp.  
Unfortunately the change broke the current code so that the current code 
doesn't work if the command you are spawning can't deal with the double 
slashes.

comment:2 Changed 12 years ago by itamarst

James checked in a fix for this.

Changed 12 years ago by osuchw

Attachment: cmdline_quote.patch added

comment:3 Changed 12 years ago by osuchw

Currently every argument gets quoted.  Some programs eg. SAS.EXE get confused by
this.  Attached patch, inspired by subprocess from std lib fixes it for me.

comment:4 Changed 12 years ago by justinj

I'm reassigning this to me.  Let me know if you have a problem with that.

comment:5 Changed 12 years ago by justinj

Resolved in r15008.

comment:6 Changed 12 years ago by bwh

Firstly, some Windows commands really really shouldn't have their arguments
quoted. In particular, if I want to run a command line through cmd, I should
prefix it with "cmd /c" and *not* quote it, even though it contains spaces.
Quoting the command line causes cmd to treat the whole thing as a command name!

Secondly, backslashes at the end of a quoted string also need to be escaped. The
quoting function should be:

_cmdLineEscapeRe = re.compile(r'(\\*)"')
_cmdLineEscapeQuotedRe = re.compile(r'(\\*)("|$)')
def _cmdLineEscapeQuoted(m):
    return m.group(1) + m.group(1) + (m.group(2) and '\\"' or '')
def _cmdLineQuote(s):
    if (" " in s) or ("\t" in s):
        return '"' + _cmdLineEscapeQuoteRe.sub(_cmdLineEscapeQuoted, s) + '"'
    else:
        return _cmdLineEscapeRe.sub(r'\1\1\\"', s)

comment:7 Changed 12 years ago by bwh

Following on from my first point, can I suggest that spawnProcess on Windows
should accept command line strings as well as argument sequences?

comment:8 Changed 12 years ago by Jean-Paul Calderone

Can you supply a unit test for the quoting change?  I recommend changing
test_process.ProcessTestCase.testCommandLine to cover the behavior.

comment:9 Changed 12 years ago by Glyph

The quoting fix looks correct, however, the reason that spawnProcess does
quoting the way that it does is that the C runtime does dequoting according to
the same rules.  I don't see a reason to add pass-arguments-as-string to
spawnProcess - I think that your example can be easily satisfied by other means.
 For example, if you want to run "cmd /c dir /?", you can pass that as ["cmd",
"/c", "dir", "/?"], which (although certainly not portable) at least doesn't
require a new API or a different data structure.  The behaviour of cmd /c as
opposed to sh -c seems to be simply the difference between a function that takes
a list as an argument vs. a function that takes *args as an argument.

comment:10 Changed 12 years ago by bwh

The quoting/escaping function works with programs that use the Microsoft C
run-time library or CommandLineToArgvW or something similar to parse their
command-line, which covers most Windows console programs. However, there are
plenty of programs that parse differently for backward-compatibility with DOS or
because they are primarily GUI programs and don't have a main() function. I want
(and I believe I need) a way to run processes with arbitrary command-lines.
Here's another example:

    cmd /c set foo=^"bar && baz

This will run baz with the variable foo="bar in its environment. Escaping the
double-quote in the usual way does *not* work for the set command. I don't
believe it's possible to call spawnProcess in such a way that it will run this
command line.

How about adding a spawnCommand that runs 'cmd /c ' + command on Windows and is
equivalent to spawnProcess(..., '/bin/sh', ['sh', '-c', command], ...) on Posix
systems?

comment:11 Changed 12 years ago by bwh

Sorry, that was an insufficiently weird example. How about:

    cmd /c set foo=  ^"bar  && baz

This will set the value of foo to '  "bar  '.

comment:12 Changed 11 years ago by justinj

Component: conch
Owner: justinj deleted

comment:13 Changed 11 years ago by Jean-Paul Calderone

Component: conchcore
Keywords: win32 added
Owner: set to teratorn

Quoting rules should be defined by ProcessProtocol. The default should do what people usually expect, but making it overridable will provide support for ridiculous programs which can't deal with correct quoting rules.

comment:14 Changed 11 years ago by teratorn

Owner: teratorn deleted

comment:15 Changed 11 years ago by teratorn

Cc: teratorn added

comment:16 Changed 9 years ago by Jean-Paul Calderone

Also, "" isn't quoted properly.

comment:17 Changed 8 years ago by Jean-Paul Calderone

Resolution: duplicate
Status: newclosed

#3933 was a duplicate of this.

comment:18 Changed 8 years ago by khorn

see also: #3876

comment:19 Changed 7 years ago by <automation>

Note: See TracTickets for help on using tickets.