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

Command does not escape arguments as expected on windows #29494

Closed
Diggsey opened this issue Oct 31, 2015 · 23 comments · Fixed by #85832
Closed

Command does not escape arguments as expected on windows #29494

Diggsey opened this issue Oct 31, 2015 · 23 comments · Fixed by #85832
Labels
C-bug Category: This is a bug. I-needs-decision Issue: In need of a decision. O-windows Operating system: Windows P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Oct 31, 2015

Currently the windows implementation of Command will automatically attempt to escape arguments before concatenating them into a single command line string to be passed to CreateProcess. However, the escaping method used doesn't work for many windows command-line tools, including cmd, msiexec and others.

As an example, it is impossible to specify arguments like this:
ARG="some parameter with spaces"
because Command will try to quote the entire thing, whereas msiexec expects only the value to be quoted.

What's required is a way to pass arguments through unchanged: I suggest an arg_raw or arg_unescaped method be added to Command. This function can either be a windows-specific extension, or a no-op on other platforms. The advantage of the latter is that it avoids the need to cfg out code that relies on it at compile time.

@hanna-kruppe
Copy link
Contributor

Perhaps the implementation can be changed to not be that terrible? I know absolutely nothing about the win32 API, but I do know that Python's subprocess module uses a function called CreateProcess and AFAIK doesn't have this problem.

Edit: Nevermind, a look at the documentation reveals that this function takes a single string as command to be executed.

@Diggsey
Copy link
Contributor Author

Diggsey commented Oct 31, 2015

At the very least it should strive to perform the exact, minimal inverse of CommandLineToArgvW (a very nontrivial task unfortunately, see: http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx which describes the problems well enough, but the final solution given is actually still wrong!)

This would be enough to fix the problems for me with msiexec and possibly cmd. However, there's no requirement that processes on windows use CommandLineToArgvW at all, and many in fact do not, so regardless a way of passing through arguments unmolested is required.

For reference, the string ARG="some parameter with spaces" does not need escaping: CommandLineToArgvW will return it as a single argument, which at the very least shows that rusts "inverse" function is currently incorrect.

edit: Actually, no it seems it does still need escaping, it may just be that msiexec does not use CommandLineToArgvW.

@alexcrichton alexcrichton added the O-windows Operating system: Windows label Nov 2, 2015
@brson brson changed the title Command does not handle arguments well on windows. Command does not escape arguments as expected on windows Apr 11, 2017
@brson brson added P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-needs-decision Issue: In need of a decision. labels Apr 11, 2017
@retep998
Copy link
Member

This could probably be solved by adding a CommandExt method that specifies the entire command line as a single string overriding how arguments would otherwise be specified. Any code that needs behavior other than the default can then use that method to have full control over the command line.

@shepmaster
Copy link
Member

shepmaster commented Jun 26, 2017

This popped up on Stack Overflow:

let comm = r#""C:\Program Files\Google\Chrome\Application\chrome.exe" https://stackoverflow.com/"#;
let mut cmd = Command::new("cmd");
cmd.arg("/c");
cmd.arg(comm);

As I understand it, it is expected to execute

"cmd" "/c" ""C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe" https://stackoverflow.com/"

But instead executes

"cmd" "/c" "\"C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe\" https://stackoverflow.com/"

Note that the inner " have been escaped.

@mzji
Copy link

mzji commented Jul 1, 2017

Met this issue too. Do we have any workaround now?

@retep998
Copy link
Member

retep998 commented Jul 1, 2017

The workaround is calling CreateProcessW yourself. The solution is providing a Windows specific CommandExt that allows specifying a custom command string.

@mzji
Copy link

mzji commented Jul 2, 2017

@retep998 I changed the argument in the command line to avoid the quotes.

@rivy
Copy link

rivy commented Nov 18, 2018

I'm now running into this issue while trying to fix env for windows in the coretools repo.

The summary at this point seems to be, at least for Windows, with its some-what crazy command-line quoting, that there are simply some command lines which are valid and used which cannot be generated by the current std::process::Command (eg, powershell -command "Get-WmiObject -class Win32_Product"; see https://users.rust-lang.org/t/std-process-is-escaping-a-raw-string-literal-when-i-dont-want-it-to/19441 and https://stackoverflow.com/questions/44757893/cmd-c-doesnt-work-in-rust-when-command-includes-spaces).

IMO, there needs to be some way of using std::process::Command to generate completely arbitrary command lines. Specifically, argument escaping needs to be more controllable by the user. Pushing users to drop back to calling CreateProcessW, although possibly workable, doesn't seem to be the right path to encourage portability in rust.

An addition to the API of std::process::Command (eg, arg_raw()) such as suggested by @kornelski on the rust-internals discussion board (see <https://internals.rust-lang.org/t/std-process-on-windows-is-escaping-raw-literals-which-causes-problems-with-chaining-commands/8163/7) would likely help solve the issue.

Any thoughts?

And what should be done to push this issue forward?

cc: @rivy

@galich
Copy link

galich commented Jul 7, 2019

Same problem here, we can't use std::process::Command to run something like:

app.exe --folder="C:\Program Files\"

@hrydgard
Copy link

hrydgard commented Nov 4, 2019

Also ran into this, trying to open Explorer with a selected file, like this:

let result = std::process::Command::new("explorer.exe").arg(format!("/select,\"{:?}\"", path)).spawn();

This does not work, due to the quotes getting escaped when they shouldn't be. Explorer.exe expects to be called like this:

explorer.exe /select,"C:\my\qouted\path"

which becomes

explorer.exe /select,""C:\my\qouted\path""

which it doesn't accept.

Would a pull request adding arg_raw() be accepted at this point? This really needs a resolution, it's not acceptable to have to drop down to CreateProcess.

@Artoria2e5
Copy link
Contributor

Artoria2e5 commented Mar 18, 2020

At the very least it should strive to perform the exact, minimal inverse of CommandLineToArgvW (a very nontrivial task unfortunately, see: http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx which describes the problems well enough,

The new link is at https://docs.microsoft.com/en-gb/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way.

but the final solution given is actually still wrong!)

How exactly is it wrong? This appears to be a pretty good inverse of the function as implemented in .NET CoreFX. There are two caveats though:

  1. For those who type the resultant command-line in cmd, this will break on metacharacters like & and | because they are not tested for quoting. Even if they quote it, a quote would easily break it because cmd does not see \" as a way to un-mark an end-quotation part. The workaround is to use the undocumented, but ubiquitously accepted "" escape
  2. Wildcards. The behavior regarding quoting and wildcards is poorly documented. Generally it is assumed that quoting turns off wildcard expansion even if _setargv is linked in, but exceptions exist in certain versions of the MS runtime. Just do your best, pretend this is wine __getmainargs, and quote.

(Yeah, the "better method" in this article is pretty verbose if you consider the workaround from 1. I generally just do a JS one-liner of return `"${arg.replace(/(\\*)($|")/g, '$1$1$2').replace(/"/g, '""')}"`; for it, so that it works in both.)

Anyway, this blog code is what process::sys(windows)::Command does. It seems fine.


There is another reason for a raw command-line thing that many people might be more familiar with: Cygwin. Cygwin and MSYS2 ship an incompatible command-line parser in their dcrt0.cc, and I don't think I need to explain how common these things are these days. If you want to interface with, say, Git for Windows correctly, you gotta escape the strings differently.

@kornelski
Copy link
Contributor

kornelski commented Mar 18, 2020

I'd like something like .arg_raw() that just injects an argument without any escaping.

  • Should it have a more scary name? .arg_arbitrary_code_execution_vulnerability_if_you_dont_properly_escape_this_yourself()?

  • Is it acceptable for this method to surround the unescaped argument with spaces? Is there any command that cares if there is a space between exe and the args? e.g. foo.exe/arg is different from foo.exe /arg?

  • Should there be an escaping helper function to help build raw arg that includes both quoted and unquoted parts? Or maybe raw args should not add spaces to be able to prefix other quoted args?.raw_no_spaces("foo,").arg("bar") == foo,"bar"

@Artoria2e5
Copy link
Contributor

Artoria2e5 commented Mar 18, 2020

arg_arbitrary_code_execution_vulnerability_if_you_dont_properly_escape_this_yourself

This should not be named as such for our CreateProcess-based invocation, since it does not really make it less safe than the default escape. We are not escaping for cmd, which actually has metacharacters for pipes and semicolons.

Is there any command that cares if there is a space between exe and the args?

No. There is a DOS-inherited cmd-ism here, but it is exclusively for internal commands. See https://stackoverflow.com/questions/4094699/how-does-the-windows-command-interpreter-cmd-exe-parse-scripts.

The CreateProcess documentation only mentions using spaces to tokenize when the file argument is null.

helper

Maybe, with some big warnings about non-generality preferably. The argv[0] will almost always need quoting.

Doing this does raise the question of how many kinds of helpers are supposed to be in stdlib though.

@retep998
Copy link
Member

Specifying an arbitrary command line should be completely safe. The only way you could cause unsound behavior is if the spawned process has a bug in its command line parser, which isn't Rust's problem.

Just add a windows specific method to let users specify the full command line themselves, and that's all libstd needs to do to enable spawning those quirky processes with Command.

@dgrunwald
Copy link
Contributor

dgrunwald commented May 27, 2021

CommandExt::raw_arg should exist to allow providing arbitrary command-lines to child processes without having to call CreateProcess manually.

But it should be noted that Python solves this differently!
Python has two possible ways of starting subprocesses:

subprocess.run(['program.exe', 'arg1', 'arg2'])

On Unix, this just passes the argument-array as-is. On Windows, this constructs a single command-line string with the "inverse of CommandLineToArgvW" rules. As such, just like Rust's implementation, this works in a cross-platform manner for most Windows programs, with the notable exception of cmd.exe.

But Python also has:

subprocess.run('program.exe arg1 arg2', shell=True)

On Unix, this runs [getenv('SHELL', '/bin/sh'), '-c', cmdline].
On Windows, this runs f'{getenv('COMSPEC', 'cmd.exe')} /c "{cmdline}"'. Yes, for cmd.exe (and cmd.exe alone), it is correct to surround the cmdline with quotes, without escaping any quotes that appear inside the cmdline.

Having a "run command-line in shell" function nicely abstracts away this weirdness. If Rust had this (e.g. via a Command::new_shell constructor), tools like hyperfine wouldn't need platform-specific code.

@Artoria2e5
Copy link
Contributor

My take 2 of the raw_arg and sized arg PR is still available. A recent rebase broke some macro invocation though.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 9, 2021
Unescaped command-line arguments for Windows

Some Windows commands, expecially `cmd.exe /c`, have unusual quoting requirements which are incompatible with default rules assumed for `.arg()`.

This adds `.unquoted_arg()` to `Command` via Windows `CommandExt` trait.

Fixes rust-lang#29494
@bors bors closed this as completed in d868da7 Jul 9, 2021
@kornelski
Copy link
Contributor

It's here!

https://doc.rust-lang.org/nightly/std/process/struct.Command.html#method.raw_arg

Now it's a good time to bikeshed the name. Is raw_arg acceptable? It kinda sounds like a raw pointer.

I've tried unquoted_arg, but that was ambiguous regarding escaping (e.g. foo\ bar is still unquoted).

Maybe literal? Hopefully won't be confused with string literals :/

@Artoria2e5
Copy link
Contributor

Hmm. That's one thing to split off from #82930.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Jan 15, 2022

Tracking issue for stabilizing the fix to this #92939

facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Jul 6, 2023
Summary:
The `cmd.exe` does not use the "common" argv[] parsing [1] [2]. For example, the Rust code:

  Command::new("cmd.exe").arg("/c").arg(r#"notepad "a b.txt"#)

will execute (Windows OS command line, as a single string):

  cmd.exe /c "notepad \"a b.txt\""

which will execute:

  notepad \"a b.txt\"

and notepad will complain that the file cannot be found.

To fix it we need to pass the unquoted command and execute either:

  cmd.exe /c "notepad "a b.txt""
  cmd.exe /c notepad "a b.txt"

which will execute:

  notepad "a b.txt"

See also rust-lang/rust#29494.

[1]: https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw
[2]: https://learn.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments

Reviewed By: zzl0

Differential Revision: D47242639

fbshipit-source-id: c75aa83430520c29002a095333546cc48695244e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-needs-decision Issue: In need of a decision. O-windows Operating system: Windows P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.