Add SSHManager support for invoking Windows workers via cmd.exe #30614

mgkuhn · 2019-01-06T00:46:12Z

Distributed.addprocs() now supports two new keyword arguments ssh
and shell.

Specifying shell=:wincmd now makes it possible to start workers on a
Windows machine with an sshd server that invokes cmd.exe as the shell
(as e.g. Microsoft's OpenSSH port does by default). Previously
SSHManager only supported ssh connections to a POSIX shell.

Specifying ssh="/usr/bin/ssh" makes it possible to specify the ssh
client that SSHManager will use (useful for debugging and where a
custom-version of ssh is required).

vtjnash · 2019-01-07T21:40:39Z

stdlib/Distributed/src/managers.jl

@@ -225,6 +268,68 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched,
 end


+function print_shell_escaped_windows(io, args::AbstractString...)::Nothing


I think this is the implementation for quoting Windows commands, but not DOS cmd commands, and actually doesn't involve the shell at all. I think you may need to pass it through both escaping passes (and set windows_verbatim=true on the Cmd object) to get it to be interpreted correctly?

I think we should put this in base/shells.jl, where shell_escape_posixly already lives (does anyone else have a preference?). Regardless, it will also need some unit tests. I think this can even be a separate initial PR (to add quoting functions for Win32 and DOS), then make this PR on top of it.

I have now also implemented an actual cmd.exe escaping algorithm on top of C-runtime ARGS escaping. The method I finally found (after several attempts) may seem a bit odd, but seems to work well: inserts a ^ escape character before any potential cmd.exe metacharacter whenever there was so far an even number of double quotes. This algorithms is needed because cmd.exe only half-treats double quotes as metacharacters: it does not remove them, but it uses them to toggle a quoting-mode flag, which in turn activates or deactivates other metacharacters, including ^.

I have tested this patch for far only in the case where a Linux leader machine invokes a worker on a Windows machine (my use case is remotely controlling from Linux some laboratory kit for which only a Windows driver DLL is available). I have not yet tested remote method calls from Windows to Windows or form Windows to Linux, but I wouldn't expect there any changes needed. In particular, I would not expect any need for setting windows_verbatim=true. Without this flag, libuv already applies locally the same C-runtime escaping algorithm to the ssh commandline that I also had implemented for the remote command line, and that is needed to pass remotecmd on to the ssh process on Windows.

I've followed the function signature and naming of shell_escape_posixly() closely in case someone wants to put this into shells.jl, but I don't have particular views on what it should be called or where it should go.

I think that's the clearest description of the cmd.exe algorithm I've ever heard.

I think it might be better to split this into a shell_escape_win32ly and shell_escape_dosly pair though, since some uses might only want one or the other (well, I don't care much about the exact names, just something memorable enough). I also think these are appropriate utility functions for inclusion in shell.jl. Can you move this there and add some tests? (you can probably just add them alongside the shell_escape_posixly tests)

Such a split is not very practical with the Strings[] -> String signature that the function currently has. The two escaping steps are quite interlinked for two reasons. Firstly, the outer function for cmd.exe ^ escaping would need to know where the argument-boundary spaces are (i.e., it still needs an array of strings as input, not a single string), and secondly, the inner function for escaping the C-runtime parser would need to know what meta-characters beyond space/tab should trigger using quotes (to escape them on behalf of the outer function). Therefore, rather than splitting these steps into two functions, I'd rather add a shell keyword argument that specifies what kind of Windows shell this will pass through, with values nothing, :wincmd and :powershell to be supported.

The reason I didn't commit that keyword argument initially is solely that sprint is currently unable to pass on keyword arguments (hence my new pull request #30708 to fix sprint first).

stdlib/Distributed/src/managers.jl

vtjnash · 2019-01-07T21:58:38Z

stdlib/Distributed/src/managers.jl

+                backslashes = 0
+                write(io, c)
+            end
+        end


I think you can can simplify this to just write(io, arg), then count the number of trailing \\ and output a second copy of that substring slice

vtjnash · 2019-01-08T22:25:54Z

stdlib/Distributed/src/managers.jl

+        remotecmd = shell_escape_windows(exename, exeflags...)
+        if dir !== nothing && dir != ""
+            any(c -> c == '"', dir) && throw(ArgumentError("invalid dir"))
+            remotecmd = "pushd \"$(dir)\" && $remotecmd"


"pushd $(print_shell_escaped_dosly(dir)) && $remotecmd" ?

I can't use cd because cmd.exe does not support setting the current working directory to a UNC path \\filer\share\.... I tried at first to get around this by changing the working directory via julia -e "cd(\"\\\\filer\\share\...\") only to realise that julia does not execute both options -e and --worker at the same time. I finally discovered that pushd is a much better command than cd, because when you give pushd a UNC, it automatically maps the share's root to a network drive as needed, and then cmd.exe happily takes that as its current working directory. (That discovery also reduced somewhat the need for very comprehensive string escaping that using option -e would have required, but having cmd.exe escaping implemented properly might still become useful one day if we put more stuff into exeflags.)

Since Windows does not permit " in filenames, there is no need to escape any of them in the argument of pushd. Therefore, always quoting the pathname passed to pushd seemed sufficient here.

"dosly" would be inaccurate because cmd.exe came new with Windows NT and is different from MS-DOS's COMMAND.EXE. (I don't recall if MS-DOS also had ^ escaping in COMMAND.EXE, it has been ~18 years since the last MS-DOS release ...)

Ah, true. Wikipedia mentions that capability as an improvement of cmd.exe over COMMAND.COM. Even so, there may or might not be another special rule (for '%'): https://en.wikibooks.org/wiki/Windows_Batch_Scripting#Quoting_and_escaping

vtjnash · 2019-01-08T22:30:40Z

stdlib/Distributed/src/managers.jl

+            remotecmd = "pushd \"$(dir)\" && $remotecmd"
+        end
+        if tval !== nothing && tval != ""
+            all(isdigit, tval) || throw(ArgumentError("invalid JULIA_WORKER_TIMEOUT"))


can we move this check outside the if block (and drop the escaping for the posix case)?

although this is too strict (the value is Float64-parsable)

vtjnash · 2019-01-08T22:31:55Z

stdlib/Distributed/src/managers.jl

+        cmdline_cookie = true
+        exeflags = `$exeflags --worker=$(cluster_cookie())`
+
+        any(c -> c == '"', exename) && throw(ArgumentError("invalid exename"))


should be safe (because of shell-escape below), although true it's not a valid Win32 name.

vtjnash · 2019-01-08T22:51:14Z

stdlib/Distributed/src/managers.jl

@@ -225,6 +268,68 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched,
 end


+function print_shell_escaped_windows(io, args::AbstractString...)::Nothing


I think that's the clearest description of the cmd.exe algorithm I've ever heard.

I think it might be better to split this into a shell_escape_win32ly and shell_escape_dosly pair though, since some uses might only want one or the other (well, I don't care much about the exact names, just something memorable enough). I also think these are appropriate utility functions for inclusion in shell.jl. Can you move this there and add some tests? (you can probably just add them alongside the shell_escape_posixly tests)

mgkuhn · 2019-01-18T10:31:53Z

Self-reminder to test implications of recent quoting changes in Win32-OpenSSH v7.9.0.0p1-Beta: PowerShell/Win32-OpenSSH#1082

musm · 2019-10-03T19:15:15Z

stdlib/Distributed/src/managers.jl

+                    write(io, '\\')
+                end
+                backslashes = 0
+                isword(c) || quoted || write(io, '^')   # for cmd.exe


@mgkuhn Could you explain this logic here with isword?
For the isword function it looks like every cmd.exe special meta-character is excluded except '"' and '%'.

What's confusing is that the ^ escaping depends upon whether the argument is quoted, why?

In the below example $ is quoted and is not isword
shell_escape_win(string"$) |> print gives "string\"^$"

but in this example this line is not triggered
shell_escape_win(string$) |> print gives "string$"

What's the rationale here? This is the only major thing I don't understand in the code. Everything else correctly implements the inverse of CommandLineToArgvW

Welcome to the wondrous world of the CMD.EXE parser. Whether ^ is an escape character or not depends on whether there have been an even or odd number of " on the line so far (see the Phase 2 description and the description of the quote flag in the previous link). We have not much control over the placement of ", as CMD.EXE always passes these on to the application, which then (this is typically done by libc according to the conventions of the Microsoft C runtime) uses them to split the command-line string into an argv array. Therefore we have to alternate between the two available quoting mechanisms in sync with the quote flag in the parser in CMD.EXE. This all gets complicated by the fact that the " are seen and interpreted by both CMD.EXE and the application.

@mgkuhn Thanks for the reply. (BTW I opened a PR to get portions of this PR merged, only because I thought you weren't interested in pushing this PR through due to inactivity. I'd be glad if you have the time otherwise, I can try to understand the issue and push it through.)

I think I kind of understand the gist of your comment. However wouldn't it be much easier to implement the following logic described at the end of the post here. https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/

1 Always escape all arguments so that they will be decoded properly by CommandLineToArgvW, perhaps using my ArgvQuote function above.

2 After step 1, then if and only if the command line produced will be interpreted by cmd, prefix each shell metacharacter (or each character) with a ^ character.

Currently your code tries to intelligently infer if we should quote the argument depending if all the characters in the word are isword. Since we know each argument is the output of shell_split, it doesn't matter and we can just \"arg\". This is much simpler.

As consequence of step 1, we will always have an even number of " and thus ^ will always be an escape character.

In any case, we can disregard my comment above, since it does indeed seem simpler to use your idea to toggle the state depending on " . It seems that all the other processing should be properly taken care of by CMD.

I.e. this seems to implement all the processing we need to do .

I'm still planning to push some variant of this PR through by the end of this year, but I will need more time for thorough testing as well as also covering PowerShell. I'm nore sure this PR is actually closely related to the one of Shell mode in REPL. The escaping routines in this PR are designed to pass strings transparently (past CMD.EXE or PowerShell) to an application (such as Julia) that parses the command line using the Microsoft C conventions. In REPL Shell mode, this is not the only or even main requirement. You may want to send commands that are interpreted by CMD.EXE, which parses its lines using completely different conventions from CommandLineToArgvW. I don't see how any function inverting CommandLineToArgvW would be of use in REPL Shell mode for Windows, as CMD.EXE does no use anything remotely resembling CommandLineToArgvW. I haven't looked yet closely into how REPL Shell mode works, but for Windows it would have to pass the whole command line transparently as a single string to the shell.

mgkuhn · 2019-12-15T23:04:19Z

I think it might be better to split this into a shell_escape_win32ly and shell_escape_dosly pair though, since some uses might only want one or the other (well, I don't care much about the exact names, just something memorable enough). I also think these are appropriate utility functions for inclusion in shell.jl. Can you move this there and add some tests? (you can probably just add them alongside the shell_escape_posixly tests)

@vtjnash Sorry for the long delay, I have now done all of that in #34111
When #34111 is OK to merge, I will update/simplify this PR accordingly.

Implementation of the raw-string escaping convention. Can also be used to escape command-line arguments for Windows C/C++/Julia applications, which use the same \" escaping convention as Julia non-standard string literals.

Using escape_raw_string() make this function much easier to understand. Name changed, as the function is not related to any shell.

* ensure that only an even number of double quotes is passed through unescaped, such that the quote flag in CMD.EXE remains unset afterwards * clarify in documentation why no attempt to escape `%` * operates now on bytes rather than Unicode characters (to avoid UTF-8 decoding overhead) * @ is now escaped at start of string * more optimizations and test cases

No longer assume that AbstractString uses some ASCII compatible encoding (such as UTF-8), but call the UTF-8 decoder for each character (even though this causes 30-60% runtime overhead compared to the previous version).

Distributed.addprocs() now supports two new keyword arguments `ssh` and `shell`. Specifying `shell=:wincmd` now makes it possible to start workers on a Windows machine with an sshd server that invokes cmd.exe as the shell (as e.g. Microsoft's OpenSSH port does by default). Previosly SSHManager only supported ssh connections to a POSIX shell. Specifying `ssh="/usr/bin/ssh"` makes it possible to specify the ssh client that SSHManager will use (useful for debugging and where a custom-version of ssh is required).

…osoft_c_args from PR JuliaLang#34111

* generalized mechanics for passing environment variables to workers * `cmdline_cookie` is work around for an ssh problem with Windows workers that run older (pre-ConPTY) Windows or Julia versions

mgkuhn · 2020-01-06T11:34:09Z

Now rebased on top of the latest commit on #34111, the separated out PR that adds shell_escape_wincmd().
What else is needed before this can be merged?

ViralBShah · 2020-04-10T18:17:29Z

Bump. Apart from rebasing, what else do we need to get this in?

mgkuhn · 2020-04-10T18:57:56Z

This (basically relatively simple) PR got unfortunately dragged out a bit, probably unnecessarily. It initially contained some shell-escaping functions to prepare Windows command lines, which @vtjnash suggested are more widely useful beyond Distributed and he therefore suggested these should go into base/shell.jl instead. There are really two functions: one escaping metacharacters of CMD.EXE, the other formatting a command line such that arbitrary characters can be passed on to a Microsoft C program's argv array. (Unlike Unix, Windows passes on a command line to an application as a single string, not as an array of strings, therefore a C program has its own parser to split that into argv[], and we need an inverse of that function.) Then @musm added one of these functions, the one that prepares an argv[] string, but using the rather odd name shell_escape_winsomely(), odd because it does not actually escape any “shell” functionality but Microsoft libc functionality. I then prepared #34111, which refactors and renames @musm's shell_escape_winsomely() into escape_microsoft_c_args(), and also adds the second function as shell_escape_wincmd().

Therefore my suggestion is to first look at #34111, and once that is merged, this PR here should be pretty straight forward.

I'll prepare another rebase of #34111 for you to look at.

mgkuhn · 2020-11-09T00:55:03Z

Replacing this PR with PR #38353

andreasnoack requested a review from vtjnash January 7, 2019 08:47

stevengj added system:windows Affects only Windows domain:parallelism Parallel or distributed computation labels Jan 7, 2019

vtjnash reviewed Jan 7, 2019

View reviewed changes

vtjnash mentioned this pull request Jan 7, 2019

shell_escape_posixly: fix handling of empty argument #30636

Merged

vtjnash reviewed Jan 8, 2019

View reviewed changes

vtjnash mentioned this pull request Mar 26, 2019

REPL shell mode for Windows #23703

Closed

vtjnash mentioned this pull request Oct 1, 2019

REPL shell mode for Windows #33427

Closed

musm reviewed Oct 3, 2019

View reviewed changes

musm mentioned this pull request Oct 3, 2019

Add winsomely that converts args to a command line #33465

Merged

mgkuhn force-pushed the wincmd branch from f3d3cf3 to cccd74e Compare December 14, 2019 17:19

mgkuhn mentioned this pull request Dec 15, 2019

Refactor shell_escape_winsomely() and add escaping function for CMD.EXE syntax #34111

Closed

mgkuhn added 6 commits December 16, 2019 10:17

adding escape_raw_string()

cba9a8a

Implementation of the raw-string escaping convention. Can also be used to escape command-line arguments for Windows C/C++/Julia applications, which use the same \" escaping convention as Julia non-standard string literals.

refactored shell_escape_winsomely() into escape_microsoft_c_args()

c09f816

Using escape_raw_string() make this function much easier to understand. Name changed, as the function is not related to any shell.

fix dependency problems, load regex.jl before shell.jl

afc8464

fixed doctest

5973452

remove shell_escape_winsomely()

5c21006

mgkuhn force-pushed the wincmd branch from cccd74e to 6034723 Compare December 23, 2019 22:45

mgkuhn added 5 commits January 6, 2020 10:53

shell_escape_wincmd: use proper AbstractString API

57425e2

No longer assume that AbstractString uses some ASCII compatible encoding (such as UTF-8), but call the UTF-8 decoder for each character (even though this causes 30-60% runtime overhead compared to the previous version).

escape Windows cmd.exe metacharacters with ^

237d4c9

replace shell_escape_windows with shell_escape_wincmd and escape_micr…

7542d70

…osoft_c_args from PR JuliaLang#34111

addprocs: new keyword arguments env and cmdline_cookie

d6ca9f7

* generalized mechanics for passing environment variables to workers * `cmdline_cookie` is work around for an ssh problem with Windows workers that run older (pre-ConPTY) Windows or Julia versions

mgkuhn force-pushed the wincmd branch from 6034723 to d6ca9f7 Compare January 6, 2020 11:15

mgkuhn mentioned this pull request Feb 5, 2020

Unable to pipe shell commands PowerShell/Win32-OpenSSH#1545

Closed

This was referenced Nov 8, 2020

Refactor/rename shell_escape_winsomely() and add escaping function for CMD.EXE syntax #38352

Merged

Add SSHManager support for invoking Windows workers via cmd.exe #38353

Merged

mgkuhn closed this Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSHManager support for invoking Windows workers via cmd.exe #30614

Add SSHManager support for invoking Windows workers via cmd.exe #30614

mgkuhn commented Jan 6, 2019 •

edited

Loading

vtjnash Jan 7, 2019

mgkuhn Jan 8, 2019

mgkuhn Jan 8, 2019

mgkuhn Jan 8, 2019 •

edited

Loading

vtjnash Jan 8, 2019

mgkuhn Jan 13, 2019

vtjnash Jan 7, 2019

vtjnash Jan 8, 2019

mgkuhn Jan 8, 2019

mgkuhn Jan 8, 2019

mgkuhn Jan 8, 2019

vtjnash Jan 8, 2019

vtjnash Jan 8, 2019

vtjnash Jan 8, 2019

vtjnash Jan 8, 2019

mgkuhn commented Jan 18, 2019 •

edited

Loading

musm Oct 3, 2019

mgkuhn Oct 3, 2019 •

edited

Loading

musm Oct 3, 2019

musm Oct 3, 2019 •

edited

Loading

mgkuhn Oct 4, 2019

mgkuhn commented Dec 15, 2019 •

edited

Loading

mgkuhn commented Jan 6, 2020

ViralBShah commented Apr 10, 2020

mgkuhn commented Apr 10, 2020 •

edited

Loading

mgkuhn commented Nov 9, 2020 •

edited

Loading

		@@ -225,6 +268,68 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched,
		end


		function print_shell_escaped_windows(io, args::AbstractString...)::Nothing

Add SSHManager support for invoking Windows workers via cmd.exe #30614

Add SSHManager support for invoking Windows workers via cmd.exe #30614

Conversation

mgkuhn commented Jan 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgkuhn Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgkuhn commented Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

mgkuhn Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgkuhn commented Dec 15, 2019 • edited Loading

mgkuhn commented Jan 6, 2020

ViralBShah commented Apr 10, 2020

mgkuhn commented Apr 10, 2020 • edited Loading

mgkuhn commented Nov 9, 2020 • edited Loading

mgkuhn commented Jan 6, 2019 •

edited

Loading

mgkuhn Jan 8, 2019 •

edited

Loading

mgkuhn commented Jan 18, 2019 •

edited

Loading

mgkuhn Oct 3, 2019 •

edited

Loading

musm Oct 3, 2019 •

edited

Loading

mgkuhn commented Dec 15, 2019 •

edited

Loading

mgkuhn commented Apr 10, 2020 •

edited

Loading

mgkuhn commented Nov 9, 2020 •

edited

Loading