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

document correct usage for shell_escape_wincmd #38513

Merged
merged 3 commits into from
Dec 6, 2020
Merged

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Nov 20, 2020

Adds the documentation missing from #38352 on the purpose of this function.
Note that most resources online are wrong, and even cmd /c help cmd prints the wrong list,
so it is important to be clear here about the actual guarantees this function can afford.

@vtjnash vtjnash added the domain:docs This change adds or pertains to documentation label Nov 20, 2020
base/shell.jl Show resolved Hide resolved
base/shell.jl Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
@mgkuhn
Copy link
Contributor

mgkuhn commented Nov 30, 2020

The result is safe to pass as an argument to a command call being processed by
CMD.exe /S /C " ... " (with surrounding double-quote pair) and will be
received verbatim by the target application if the input does not contain %
(else this function will fail with an ArgumentError).

I'm not yet convinced this sentence is true. But I've also not even understood yet the precise application context talked about here (e.g., who will interpret the surrounding double-quote pair?).

Could you please provide me with a complete Julia script that demonstrates the use described in this sentence?

That should then allow me to try to falsify the safety claim made in this sentence, by trying to form an input string that will not be passed verbatim to the application called by your example script (e.g. echo).

@mgkuhn
Copy link
Contributor

mgkuhn commented Nov 30, 2020

Could we also list as one (perhaps even the first!) use of this function:

  • to build command lines that are passed to cmd.exe via an SSH connection

?

That was my not only initial motivation to write it: I also still think this use case is much easier to understand than any of the nested calls to cmd.exe from within julia.exe via other, local command-line invocation APIs with built-in metacharacter escaping functionality (e.g. the one interpreting “the windows_verbatim flag”, libuv?) that your docstring extension alludes to.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 30, 2020

Here's how your naive attempt can be hijacked:

cmd> cmd /c "argsecho" "a b"
'argsecho" "a' is not recognized as an internal or external command,
operable program or batch file.

What??

Oh, if we consult the documentation carefully, we discover the command parsed here was instead argsecho a and b for various backwards compatibility reasons.

How to fix? Always use /s with " ... ":

cmd> cmd /s /c " "argsecho" "a b" "
"argsecho"  "a b"
0: argsecho
1: a b

via an SSH connection

This should be irrelevant in this sentence, if correctly implemented. The key phrase is "passed to cmd.exe", and any other program mentioned is a distraction, as their existence must be handled elsewhere.

windows_verbatim

This flag is typically mandatory when using this function, to disable the C-standard behaviors otherwise employed.

@mgkuhn
Copy link
Contributor

mgkuhn commented Nov 30, 2020

Could we perhaps include:

An example of a Julia wrapper to invoke the built-in echo command of cmd.exe:

wincmd(c::String) =
   run(Cmd(Cmd([ENV["windir"] * raw"\system32\cmd.exe", "/s", "/c", "\"$c\""]);
           windows_verbatim=true))
echo(s::String) =
   wincmd(Base.shell_escape_wincmd("echo $s"))
echo("hello %user% and the &()^\"^^\"%\" world!")

Note that it escapes all meta characters in s except for pairs of %...% enclosing existing environment variables.

@vtjnash Does this concrete example script demonstrate the use case you wanted to have covered?

base/shell.jl Outdated Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
Note that most resources online are wrong, and even `cmd /c help cmd` prints the wrong list,
so it is important to be clear here about the actual guarantees this function can afford.

Refs #38352
base/shell.jl Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
base/shell.jl Outdated Show resolved Hide resolved
base/shell.jl Show resolved Hide resolved
@vtjnash vtjnash merged commit 39a6c0d into master Dec 6, 2020
@vtjnash vtjnash deleted the jn/38352-docs branch December 6, 2020 23:00
@mgkuhn
Copy link
Contributor

mgkuhn commented Dec 7, 2020

Doesn't the new text give the (incorrect) impression that including % will cause an ArgumentError?

@musm
Copy link
Contributor

musm commented Dec 8, 2020

Doesn't the new text give the (incorrect) impression that including % will cause an ArgumentError?

Do you specifically mean this line: https://github.com/JuliaLang/julia/pull/38513/files#diff-92d17ed4c3ab24cca65d43dd11a040f4f8e96378384fa143e2d418c5b07656d3R277

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Dec 8, 2020

I could see how that's unclear which clause the 'else' connects to, though I feel the surrounding text is clear that '%' does not throw an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants