Fixing Windows and DOS command line argument quoting

Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

[-- Attachment #1.1: Type: text/plain, Size: 398 bytes --]

As I explain in [1], many Windows programs get command line argument
quoting wrong.  Emacs is one of these programs.  The attached patch
resolves the issue, and if there are no objections, I'll apply it to
both the Emacs 23 branch and the trunk in the next few days.

[1]
http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx


[-- Attachment #1.2: quote-fix.patch --]
[-- Type: text/plain, Size: 2020 bytes --]

=== modified file 'lisp/subr.el'
--- lisp/subr.el	2011-04-19 13:44:55 +0000
+++ lisp/subr.el	2011-04-25 02:05:07 +0000
@@ -2507,19 +2507,33 @@
   "Quote ARGUMENT for passing as argument to an inferior shell."
   (if (or (eq system-type 'ms-dos)
           (and (eq system-type 'windows-nt) (w32-shell-dos-semantics)))
-      ;; Quote using double quotes, but escape any existing quotes in
-      ;; the argument with backslashes.
-      (let ((result "")
-	    (start 0)
-	    end)
-	(if (or (null (string-match "[^\"]" argument))
-		(< (match-end 0) (length argument)))
-	    (while (string-match "[\"]" argument start)
-	      (setq end (match-beginning 0)
-		    result (concat result (substring argument start end)
-				   "\\" (substring argument end (1+ end)))
-		    start (1+ end))))
-	(concat "\"" result (substring argument start) "\""))
+      
+      ;; First, quote argument so that CommandLineToArgvW will
+      ;; understand it.  Inside an argument, 2n or 2n + 1 backslashes
+      ;; followed by a double quote produces n backslashes.  In the
+      ;; former case, we also ends the argument.  In the latter case,
+      ;; we also produce a double-quote character and end the
+      ;; argument.  Backslashes not followed by a quotation mark are
+      ;; left alone. See
+      ;; http://msdn.microsoft.com/en-us/library/17w5ykft%28v=vs.85%29.aspx
+      ;; After we perform that level of quoting, replace shell
+      ;; metacharacters so that cmd won't mangle our argument.
+
+      (replace-regexp-in-string
+       "\\([()\"<>&|^]\\)"
+       "^\\1"
+       (concat
+        "\""
+        ;; escape backslashes at end of string
+        (replace-regexp-in-string
+         "\\(\\\\*\\)$"
+         "\\1\\1"
+         ;; escape backslashes and quotes in string body
+         (replace-regexp-in-string
+          "\\(\\\\*\\)\""
+          "\\1\\1\\\\\""
+          argument))
+        "\""))
     (if (equal argument "")
         "''"
       ;; Quote everything except POSIX filename characters.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Eli Zaretskii]

> Date: Sun, 24 Apr 2011 19:09:31 -0700
> From: Daniel Colascione <dan.colascione@gmail.com>
> 
> As I explain in [1], many Windows programs get command line argument
> quoting wrong.  Emacs is one of these programs.  The attached patch
> resolves the issue, and if there are no objections, I'll apply it to
> both the Emacs 23 branch and the trunk in the next few days.

Thanks.  I have 2 requests and a question.  The requests are:

 . Please leave the `ms-dos' quoting as it was before.  (Your research
   and blog are not valid for the MS-DOS, a.k.a. DJGPP, build of
   Emacs, which uses its own private way of passing and decoding
   command lines, bypassing MS runtime and OS facilities.  The
   problems you mention in your blog do not exist in the MS-DOS
   build.)  Please make the new quoting method effective for the
   `windows-nt' case alone.

 . Please install this only on the trunk.  The emacs-23 branch should
   not be destabilized by such experiments at this time.

My question is this: will cmdproxy need any changes to support this
new kind of quoting, or does it already have everything that it needs?

Thanks again for working on this.

Re: Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

Hi Eli,

Thanks for taking a look at the patch.

On 4/24/11 11:41 PM, Eli Zaretskii wrote:
> Thanks.  I have 2 requests and a question.  The requests are:
> 
>  . Please leave the `ms-dos' quoting as it was before.  (Your research
>    and blog are not valid for the MS-DOS, a.k.a. DJGPP, build of
>    Emacs, which uses its own private way of passing and decoding
>    command lines, bypassing MS runtime and OS facilities.  The
>    problems you mention in your blog do not exist in the MS-DOS
>    build.)  Please make the new quoting method effective for the
>    `windows-nt' case alone.

What happens when MS-DOS Emacs executes a non-DJGPP program?  Doesn't it
ever pass arguments through the command interpreter?  It's been a very
long time since I've used MS-DOS, and I don't know its command
interpreter's behavior differs from that of NT's cmd.exe.

>  . Please install this only on the trunk.  The emacs-23 branch should
>    not be destabilized by such experiments at this time.

Fair enough.

> My question is this: will cmdproxy need any changes to support this
> new kind of quoting, or does it already have everything that it needs?

Thanks for bringing up cmdproxy --- I hadn't considered its presence,
and for now, it's a fly in the ointment.

For concision, let's denote the CreateProcess/CommandLineToArgvW quoting
"level one quoting" and the requisite munging cmd.exe "level two
quoting".  w32proc.c already performs level one quoting, which is
sufficient because w32proc uses CreateProcess directly.

To execute a shell command with an argument Foo, we level-one-quote and
level-two-quote Foo, then run cmdproxy with Foo as an argument, allowing
w32proc to level-one-quote it (again).  cmdproxy (or more precisely, the
C runtime to which it's linked) level-one-dequotes the value supplied
with the /c option and runs the command interpreter, supplying that
argument as its command line.  The command interpreter
level-two-dequotes this command line, sending the result to the
indicated child program, which level-one-dequotes it and receives
exactly Foo.

This process would work well, except that cmdproxy contains an
optimization that causes it to bypass the command interpreter entirely
when the supplied command line doesn't appear to contain any shell
metacharacters.  In this case, cmdproxy passes the supplied command,
which is still level-two-quoted, directly to CreateProcess, causing the
child to attempt to level-one-dequote a level-two-quoted string, likely
yielding unexpected results.

Because shell-quote-argument doesn't know whether its return value will
be interpreted by cmd or by CreateProcess alone, we can't solve the
problem there.  Two solutions remain:

1. we can have cmdproxy level-two-dequote the supplied command line
before giving it to CreateProcess, or

2. we can remove optimization described above and have cmdproxy always
run the command interpreter.

I favor the second option: cmd starts very quickly, and we don't save
much time by bypassing it.  Adding level-two-dequoting to cmdproxy would
add additional complexity and duplicate logic already present in cmd
itself.  Also, the current strategy for deciding whether to run a
command directly or execute it via the shell doesn't sit right with me,
as it make the wrong decision when arguments happen to contain quoted
shell metacharacters.

Thanks again,
Daniel Colascione



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Jason Rumney]

On 25/04/2011 16:49, Daniel Colascione wrote:

> 1. we can have cmdproxy level-two-dequote the supplied command line
> before giving it to CreateProcess, or
>
> 2. we can remove optimization described above and have cmdproxy always
> run the command interpreter.
>
> I favor the second option: cmd starts very quickly, and we don't save
> much time by bypassing it.

IIRC, this "optimisation" isn't about saving time, but about avoiding 
limitations in cmd.exe where possible.

Another possibility is to make the decision whether to use cmd.exe or 
not based on the presence of "level 2" metacharacters rather than "level 1".

Re: Fixing Windows and DOS command line argument quoting

[Eli Zaretskii]

> Date: Mon, 25 Apr 2011 16:58:35 +0800
> From: Jason Rumney <jasonr@gnu.org>
> 
> On 25/04/2011 16:49, Daniel Colascione wrote:
> 
> > 1. we can have cmdproxy level-two-dequote the supplied command line
> > before giving it to CreateProcess, or
> >
> > 2. we can remove optimization described above and have cmdproxy always
> > run the command interpreter.
> >
> > I favor the second option: cmd starts very quickly, and we don't save
> > much time by bypassing it.
> 
> IIRC, this "optimisation" isn't about saving time, but about avoiding 
> limitations in cmd.exe where possible.

Indeed.  One such limitation (but not the only one) is the command
length limitation: cmd.exe supports only 8K, while CreateProcess can
support upto 32K (more if we ever move to using the Unicode
interfaces).

> Another possibility is to make the decision whether to use cmd.exe or 
> not based on the presence of "level 2" metacharacters rather than "level 1".

This is what I would prefer.  Daniel, is this feasible?

Re: Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On 4/25/11 1:58 AM, Jason Rumney wrote:
> On 25/04/2011 16:49, Daniel Colascione wrote:
> 
>> 1. we can have cmdproxy level-two-dequote the supplied command line
>> before giving it to CreateProcess, or
>>
>> 2. we can remove optimization described above and have cmdproxy always
>> run the command interpreter.
>>
>> I favor the second option: cmd starts very quickly, and we don't save
>> much time by bypassing it.
> 
> IIRC, this "optimisation" isn't about saving time, but about avoiding
> limitations in cmd.exe where possible.
> 
> Another possibility is to make the decision whether to use cmd.exe or
> not based on the presence of "level 2" metacharacters rather than "level
> 1".

I see. Is it just the length limit?

CreateProcess supports 32kb of command line; cmd supports 8kb.  If the
extra 24kb is important, then we can do what you suggest:
shell-quote-argument would also to be further modified to only use level
2 metacharacters when strictly necessary.  Bear in mind that the new
limit would apply only to *shell* commands explicitly, not to subprocess
invocations in general, which would continue to be handled directly by
the code in w32proc.  Besides, I think it's counter-intuitive for a
shell pipeline to stop working merely because it contains more than one
command.

Also, my objection regarding false positives arising from shell
metacharacters embedded in quoted arguments stands: if users do rely on
the successful execution of long, simple shell command lines, these
commands should not break merely because they contain *quoted* shell
metacharacters.  We could have cmdproxy distinguish between quoted and
unquoted metacharacters, but at that point, we could just add
level-2-dequoting as well and solve the problem that way.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Eli Zaretskii]

> Date: Mon, 25 Apr 2011 01:49:29 -0700
> From: Daniel Colascione <dan.colascione@gmail.com>
> CC: emacs-devel@gnu.org
> 
> >  . Please leave the `ms-dos' quoting as it was before.  (Your research
> >    and blog are not valid for the MS-DOS, a.k.a. DJGPP, build of
> >    Emacs, which uses its own private way of passing and decoding
> >    command lines, bypassing MS runtime and OS facilities.  The
> >    problems you mention in your blog do not exist in the MS-DOS
> >    build.)  Please make the new quoting method effective for the
> >    `windows-nt' case alone.
> 
> What happens when MS-DOS Emacs executes a non-DJGPP program?

I'm not interested in what happens in that case.  It is a mess anyway,
because of the vastly different quoting features supported by the
stock DOS/Windows shells.  Your method will work only on Windows NT
family, and only if the DJGPP library succeeds in forcing the OS to
use cmd.exe as the shell.  (AFAIK, normally, Windows still loads
command.com for DOS programs.)

Anyway, DJGPP users know very well that to have a viable and stable
environment, they should use DJGPP programs in preference to the other
kind.  If they don't, all bets are off, and the 126-character limit on
command lines doesn't leave you enough space to safely quote commands
anyway.  The current quoting method worked well for the last 12 years,
so it ought to be good enough for these marginal cases.

> Doesn't it ever pass arguments through the command interpreter?

Only when the command explicitly invokes the shell, or some of its
non-trivial internal commands, or batch files.  Otherwise, everything
is done internally, including pipes and redirections.

> It's been a very
> long time since I've used MS-DOS, and I don't know its command
> interpreter's behavior differs from that of NT's cmd.exe.

It is VERY different, especially where quoting is considered.

Re: Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On 4/25/11 2:22 AM, Daniel Colascione wrote:
> CreateProcess supports 32kb of command line; cmd supports 8kb.  If the
> extra 24kb is important, then we can do what you suggest:
> shell-quote-argument would also to be further modified to only use level
> 2 metacharacters when strictly necessary.  Bear in mind that the new
> limit would apply only to *shell* commands explicitly, not to subprocess
> invocations in general, which would continue to be handled directly by
> the code in w32proc.  Besides, I think it's counter-intuitive for a
> shell pipeline to stop working merely because it contains more than one
> command.

After another reading of cmdproxy.c, I also see that the CreateProcess
path also doesn't expand environment %variable% references, and that
doesn't fall back to cmd if the command to be executed contains them.
While we could expand these variables, doing so would move us even
closer to reimplementing half of cmd.exe.

I'd like to remove this path in the trunk and see whether the new
8192-character length limitation is a problem in practice.  Direct
process execution will not be affected: "shell command" will just mean
"shell command", not "maybe run through the shell if we kinda guess you
really meant to use the shell".


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On 4/25/11 1:49 AM, Daniel Colascione wrote:
>>  . Please install this only on the trunk.  The emacs-23 branch should
>>    not be destabilized by such experiments at this time.
> 
> Fair enough.

I'd just like to note that it'd be a good idea to eventually backport
this fix to Emacs 23: it's a security issue.  The current
shell-quote-argument doesn't, so (shell-command (format "cmd %s"
(shell-quote-argument untrusted-input))) can run an arbitrary command.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Eli Zaretskii]

> Date: Mon, 25 Apr 2011 10:56:07 -0700
> From: Daniel Colascione <dan.colascione@gmail.com>
> Cc: emacs-devel@gnu.org
> 
> After another reading of cmdproxy.c, I also see that the CreateProcess
> path also doesn't expand environment %variable% references, and that
> doesn't fall back to cmd if the command to be executed contains them.
> While we could expand these variables, doing so would move us even
> closer to reimplementing half of cmd.exe.

We don't want to reimplement cmd.exe, that's for sure.  I would say,
if it's easy to detect that case, just make that another condition for
going through the shell.  If it isn't easy to detect, we can always
say that these commands must be explicitly run through "cmd /c" by the
application.  After all, that's not a frequent use case in Emacs,
because Emacs always expands environment variables before running the
command, and if it doesn't, it surely won't use the %foo% syntax.  So
about the only use case for this is a Lisp application that directly
targets Windows -- and those could be told to go through cmd
explicitly.

> I'd like to remove this path in the trunk and see whether the new
> 8192-character length limitation is a problem in practice.

No, I don't think it's a good idea.  8K is too low by any measure; we
already hit the shell limitations once or twice when related problems
were discussed.

What are the disadvantages of detecting quoted command lines and
sending only those through the shell?

Re: Fixing Windows and DOS command line argument quoting

[Eli Zaretskii]

> Date: Mon, 25 Apr 2011 11:24:08 -0700
> From: Daniel Colascione <dan.colascione@gmail.com>
> CC: emacs-devel@gnu.org
> 
> On 4/25/11 1:49 AM, Daniel Colascione wrote:
> >>  . Please install this only on the trunk.  The emacs-23 branch should
> >>    not be destabilized by such experiments at this time.
> > 
> > Fair enough.
> 
> I'd just like to note that it'd be a good idea to eventually backport
> this fix to Emacs 23: it's a security issue.

That security issue exists for a very long time.  I don't want us to
put a change on the release branch whose precise effects are unclear
to us.  Perhaps later, when we gain more experience on the trunk, we
could consider that.  (And it's not clear at this point whether there
will be Emacs 23.4 at all.)

Re: Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On 4/25/11 11:47 AM, Eli Zaretskii wrote:
>> Date: Mon, 25 Apr 2011 10:56:07 -0700
>> From: Daniel Colascione <dan.colascione@gmail.com>
>> I'd like to remove this path in the trunk and see whether the new
>> 8192-character length limitation is a problem in practice.
> 
> No, I don't think it's a good idea.  8K is too low by any measure; we
> already hit the shell limitations once or twice when related problems
> were discussed.

All right.

> What are the disadvantages of detecting quoted command lines and
> sending only those through the shell?

Besides complexity, none.  I've implemented this approach, making the
necessary changes to cmdproxy.c and subr.el, and updated the trunk.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Eli Zaretskii]

> Date: Tue, 26 Apr 2011 03:44:31 -0700
> From: Daniel Colascione <dan.colascione@gmail.com>
> CC: emacs-devel@gnu.org, jasonr@gnu.org
> 
> > What are the disadvantages of detecting quoted command lines and
> > sending only those through the shell?
> 
> Besides complexity, none.  I've implemented this approach, making the
> necessary changes to cmdproxy.c and subr.el, and updated the trunk.

Thank you!

Re: Fixing Windows and DOS command line argument quoting

[Ben Key]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

Daniel Colascione writes:

> After another reading of cmdproxy.c, I also see that the
> CreateProcess path also doesn't expand environment %variable%
> references, and that doesn't fall back to cmd if the command
> to be executed contains them.  While we could expand these
> variables, doing so would move us even closer to
> reimplementing half of cmd.exe.

You are exaggerating a great deal.  It is a single function call,
ExpandEnvironmentStrings, documented at
http://msdn.microsoft.com/en-us/library/ms724265%28v=vs.85%29.aspx.
Patching cmdproxy.c to use ExpandEnvironmentStrings before calling
CreateProcess would add at most 10 lines of code.  This is by no means
"reimplementing half of cmd.exe."

[-- Attachment #2: Type: text/html, Size: 868 bytes --]

Re: Fixing Windows and DOS command line argument quoting

[Daniel Colascione]

On 4/26/2011 5:58 PM, Ben Key wrote:
> Daniel Colascione writes:
>
>  > After another reading of cmdproxy.c, I also see that the
>  > CreateProcess path also doesn't expand environment %variable%
>  > references, and that doesn't fall back to cmd if the command
>  > to be executed contains them.  While we could expand these
>  > variables, doing so would move us even closer to
>  > reimplementing half of cmd.exe.
>
> You are exaggerating a great deal.  It is a single function call,
> ExpandEnvironmentStrings, documented at
> http://msdn.microsoft.com/en-us/library/ms724265%28v=vs.85%29.aspx.
> Patching cmdproxy.c to use ExpandEnvironmentStrings before calling
> CreateProcess would add at most 10 lines of code.  This is by no means
> "reimplementing half of cmd.exe."
>

I actually didn't know about ExpandEnvironmentStrings.  Thank you for 
the link; this function may be useful in some cases.  But as the 
comments on the linked MSDN page point out, ExpandEnvironmentStrings 
doesn't expand variable references quite same way cmd does, and it would 
produce incorrect results for us whether we ran it before or after 
level-2-dequoting.  Since cmdproxy should mimic cmd's processing when 
bypassing cmd itself, ExpandEnvironmentStrings would be inappropriate 
for our purposes.

The issue is moot, however, because cmdproxy now punts processing 
variable references to cmd itself, and I suspect that Eli Zaretskii is 
correct when he says that such variable references are rare.