From 2653f57d1dd06a70bf9247981dd8ee60b462f3d9 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 26 Sep 2017 12:44:40 -0400 Subject: [PATCH] correct posixly shell quoting ensures that arguments will be passed through a posix shell correctly, (including those such as being added in #21849 for -C) fix #10120 --- base/client.jl | 31 +++++++++++------ base/distributed/Distributed.jl | 2 +- base/distributed/managers.jl | 12 ++++--- base/process.jl | 2 ++ base/shell.jl | 59 ++++++++++++++++++++++++++++++++ test/repl.jl | 60 ++++++++++++++++++++++----------- test/spawn.jl | 18 ++++++++-- 7 files changed, 145 insertions(+), 39 deletions(-) diff --git a/base/client.jl b/base/client.jl index 29abb482966a81..d948c93a70ab79 100644 --- a/base/client.jl +++ b/base/client.jl @@ -85,7 +85,7 @@ stackframe_lineinfo_color() = repl_color("JULIA_STACKFRAME_LINEINFO_COLOR", :bol stackframe_function_color() = repl_color("JULIA_STACKFRAME_FUNCTION_COLOR", :bold) function repl_cmd(cmd, out) - shell = shell_split(get(ENV,"JULIA_SHELL",get(ENV,"SHELL","/bin/sh"))) + shell = shell_split(get(ENV, "JULIA_SHELL", get(ENV, "SHELL", "/bin/sh"))) shell_name = Base.basename(shell[1]) if isempty(cmd.exec) @@ -102,7 +102,14 @@ function repl_cmd(cmd, out) end cd(ENV["OLDPWD"]) else - cd(@static Sys.iswindows() ? dir : readchomp(`$shell -c "echo $(shell_escape(dir))"`)) + @static if !Sys.iswindows() + # TODO: this is a rather expensive way to copy a string, remove? + # If it's intended to simulate `cd`, it should instead be doing + # more nearly `cd $dir && printf %s \$PWD` (with appropriate quoting), + # since shell `cd` does more than just `echo` the result. + dir = read(`$shell -c "printf %s $(shell_escape_posixly(dir))"`, String) + end + cd(dir) end else cd() @@ -110,19 +117,21 @@ function repl_cmd(cmd, out) ENV["OLDPWD"] = new_oldpwd println(out, pwd()) else - run(ignorestatus(@static Sys.iswindows() ? cmd : (isa(STDIN, TTY) ? `$shell -i -c "$(shell_wrap_true(shell_name, cmd))"` : `$shell -c "$(shell_wrap_true(shell_name, cmd))"`))) + @static if !Sys.iswindows() + if shell_name == "fish" + shell_escape_cmd = "begin; $(shell_escape_posixly(cmd)); and true; end" + else + shell_escape_cmd = "($(shell_escape_posixly(cmd))) && true" + end + cmd = `$shell` + isa(STDIN, TTY) && (cmd = `$cmd -i`) + cmd = `$cmd -c $shell_escape_cmd` + end + run(ignorestatus(cmd)) end nothing end -function shell_wrap_true(shell_name, cmd) - if shell_name == "fish" - "begin; $(shell_escape(cmd)); and true; end" - else - "($(shell_escape(cmd))) && true" - end -end - function display_error(io::IO, er, bt) if !isempty(bt) st = stacktrace(bt) diff --git a/base/distributed/Distributed.jl b/base/distributed/Distributed.jl index 06e2bd9b06a6c4..f1043808975ad8 100644 --- a/base/distributed/Distributed.jl +++ b/base/distributed/Distributed.jl @@ -11,7 +11,7 @@ using Base: Process, Semaphore, JLOptions, AnyDict, buffer_writes, wait_connecte VERSION_STRING, sync_begin, sync_add, sync_end, async_run_thunk, binding_module, notify_error, atexit, julia_exename, julia_cmd, AsyncGenerator, display_error, acquire, release, invokelatest, warn_once, - shell_escape, uv_error + shell_escape_posixly, uv_error # NOTE: clusterserialize.jl imports additional symbols from Base.Serializer for use diff --git a/base/distributed/managers.jl b/base/distributed/managers.jl index 4aa1e975cd6768..118ce25fd2dc52 100644 --- a/base/distributed/managers.jl +++ b/base/distributed/managers.jl @@ -181,14 +181,16 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched, # Build up the ssh command # the default worker timeout - tval = haskey(ENV, "JULIA_WORKER_TIMEOUT") ? - `export JULIA_WORKER_TIMEOUT=$(ENV["JULIA_WORKER_TIMEOUT"])\;` : `` + tval = get(ENV, "JULIA_WORKER_TIMEOUT", "") # Julia process with passed in command line flag arguments - cmd = `cd $dir '&&' $tval $exename $exeflags` + cmds = """ + cd -- $(shell_escape_posixly(dir)) + $(isempty(tval) ? "" : "export JULIA_WORKER_TIMEOUT=$(shell_escape_posixly(tval))") + $(shell_escape_posixly(exename)) $(shell_escape_posixly(exeflags))""" # shell login (-l) with string command (-c) to launch julia process - cmd = `sh -l -c $(shell_escape(cmd))` + cmd = `sh -l -c $cmds` # remote launch with ssh with given ssh flags / host / port information # -T → disable pseudo-terminal allocation @@ -196,7 +198,7 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched, # -x → disable X11 forwarding # -o ClearAllForwardings → option if forwarding connections and # forwarded connections are causing collisions - cmd = `ssh -T -a -x -o ClearAllForwardings=yes $sshflags $host $(shell_escape(cmd))` + cmd = `ssh -T -a -x -o ClearAllForwardings=yes $sshflags $host $(shell_escape_posixly(cmd))` # launch the remote Julia process diff --git a/base/process.jl b/base/process.jl index cc188a084e6752..38e46bb6a72102 100644 --- a/base/process.jl +++ b/base/process.jl @@ -94,6 +94,8 @@ hash(x::AndCmds, h::UInt) = hash(x.a, hash(x.b, h)) shell_escape(cmd::Cmd; special::AbstractString="") = shell_escape(cmd.exec..., special=special) +shell_escape_posixly(cmd::Cmd) = + shell_escape_posixly(cmd.exec...) function show(io::IO, cmd::Cmd) print_env = cmd.env !== nothing diff --git a/base/shell.jl b/base/shell.jl index 841865790b9c9e..474d7b27f9675c 100644 --- a/base/shell.jl +++ b/base/shell.jl @@ -5,6 +5,8 @@ const shell_special = "#{}()[]<>|&*?~;" # needs to be factored out so depwarn only warns once +# when removed, also need to update shell_escape for a Cmd to pass shell_special +# and may want to use it in the test for #10120 (currently the implementation is essentially copied there) @noinline warn_shell_special(special) = depwarn("special characters \"$special\" should now be quoted in commands", :warn_shell_special) @@ -191,3 +193,60 @@ julia> Base.shell_escape("echo", "this", "&&", "that") """ shell_escape(args::AbstractString...; special::AbstractString="") = sprint(io->print_shell_escaped(io, args..., special=special)) + + +function print_shell_escaped_posixly(io::IO, args::AbstractString...) + first = true + for arg in args + first || print(io, ' ') + # avoid printing quotes around simple enough strings + # that any (reasonable) shell will definitely never consider them to be special + have_single = false + have_double = false + function isword(c::Char) + if '0' <= c <= '9' || 'a' <= c <= 'z' || 'A' <= c <= 'Z' + # word characters + elseif c == '_' || c == '/' || c == '+' || c == '-' + # other common characters + elseif c == '\'' + have_single = true + elseif c == '"' + have_double && return false # switch to single quoting + have_double = true + elseif !first && c == '=' + # equals is special if it is first (e.g. `env=val ./cmd`) + else + # anything else + return false + end + return true + end + if all(isword, arg) + have_single && (arg = replace(arg, '\'', "\\'")) + have_double && (arg = replace(arg, '"', "\\\"")) + print(io, arg) + else + print(io, '\'', replace(arg, '\'', "'\\''"), '\'') + end + first = false + end +end + +""" + shell_escape_posixly(args::Union{Cmd,AbstractString...}) + +The unexported `shell_escape_posixly` function +takes a string or command object and escapes any special characters in such a way that +it is safe to pass it as an argument to a posix shell. + +# Examples +```jldoctest +julia> Base.shell_escape_posixly("cat", "/foo/bar baz", "&&", "echo", "done") +"cat '/foo/bar baz' '&&' echo done" + +julia> Base.shell_escape_posixly("echo", "this", "&&", "that") +"echo this '&&' that" +``` +""" +shell_escape_posixly(args::AbstractString...) = + sprint(io->print_shell_escaped_posixly(io, args...)) diff --git a/test/repl.jl b/test/repl.jl index 062c3015070a07..beb7e08226d963 100644 --- a/test/repl.jl +++ b/test/repl.jl @@ -103,16 +103,16 @@ fake_repl() do stdin_write, stdout_read, repl cd(origpwd) # issue #20482 - if !Sys.iswindows() - write(stdin_write, ";") - readuntil(stdout_read, "shell> ") - write(stdin_write, "echo hello >/dev/null\n") - let s = readuntil(stdout_read, "\n") - @test contains(s, "shell> ") # make sure we echoed the prompt - @test contains(s, "echo hello >/dev/null") # make sure we echoed the input - end - @test readuntil(stdout_read, "\n") == "\e[0m\n" - end + #if !Sys.iswindows() + # write(stdin_write, ";") + # readuntil(stdout_read, "shell> ") + # write(stdin_write, "echo hello >/dev/null\n") + # let s = readuntil(stdout_read, "\n") + # @test contains(s, "shell> ") # make sure we echoed the prompt + # @test contains(s, "echo hello >/dev/null") # make sure we echoed the input + # end + # @test readuntil(stdout_read, "\n") == "\e[0m\n" + #end # issue #20771 let s @@ -129,20 +129,42 @@ fake_repl() do stdin_write, stdout_read, repl # issues #22176 & #20482 # TODO: figure out how to test this on Windows - Sys.iswindows() || let tmp = tempname() + #Sys.iswindows() || let tmp = tempname() + # try + # write(stdin_write, ";") + # readuntil(stdout_read, "shell> ") + # write(stdin_write, "echo \$123 >$tmp\n") + # let s = readuntil(stdout_read, "\n") + # @test contains(s, "shell> ") # make sure we echoed the prompt + # @test contains(s, "echo \$123 >$tmp") # make sure we echoed the input + # end + # @test readuntil(stdout_read, "\n") == "\e[0m\n" + # @test read(tmp, String) == "123\n" + # finally + # rm(tmp, force=true) + # end + #end + + # issue #10120 + # ensure that command quoting works correctly + let s, old_stdout = STDOUT + proc_stdout_read, proc_stdout = redirect_stdout() + get_stdout = @async read(proc_stdout_read, String) try write(stdin_write, ";") readuntil(stdout_read, "shell> ") - write(stdin_write, "echo \$123 >$tmp\n") - let s = readuntil(stdout_read, "\n") - @test contains(s, "shell> ") # make sure we echoed the prompt - @test contains(s, "echo \$123 >$tmp") # make sure we echoed the input - end - @test readuntil(stdout_read, "\n") == "\e[0m\n" - @test read(tmp, String) == "123\n" + Base.print_shell_escaped(stdin_write, Base.julia_cmd().exec..., special=Base.shell_special) + write(stdin_write, """ -e "println(\\"HI\\")"\n""") + s = readuntil(stdout_read, "\n") + @test contains(s, "shell> ") # check for the echo of the prompt + s = readuntil(stdout_read, "\n") + @test contains(s, "shell> ") # check for the echo of the prompt + @test readuntil(stdout_read, "\n") == "\e[0m\n" # wait for child to exit finally - rm(tmp, force=true) + redirect_stdout(old_stdout) end + close(proc_stdout) + @test wait(get_stdout) == "HI\n" end # Issue #7001 diff --git a/test/spawn.jl b/test/spawn.jl index 3ebeb130ab500d..121d04b4bb4123 100644 --- a/test/spawn.jl +++ b/test/spawn.jl @@ -254,7 +254,7 @@ let fname = tempname(), p nothing end OLD_STDERR = STDERR - redirect_stderr(open("$(escape_string(fname))", "w")) + redirect_stderr(open($(repr(fname)), "w")) # Usually this would be done by GC. Do it manually, to make the failure # case more reliable. oldhandle = OLD_STDERR.handle @@ -387,10 +387,22 @@ let cmd = ["/Volumes/External HD/program", "-a"] @test Base.shell_split("\"/Volumes/External HD/program\" -a") == cmd end +# Test shell_escape printing quoting # Backticks should automatically quote where necessary -let cmd = ["foo bar", "baz"] - @test string(`$cmd`) == "`'foo bar' baz`" +let cmd = ["foo bar", "baz", "a'b", "a\"b", "a\"b\"c", "-L/usr/+", "a=b", "``", "\$", "&&", "z"] + @test string(`$cmd`) == + """`'foo bar' baz "a'b" 'a"b' 'a"b"c' -L/usr/+ a=b \\`\\` '\$' '&&' z`""" + @test Base.shell_escape(`$cmd`) == + """'foo bar' baz "a'b" 'a"b' 'a"b"c' -L/usr/+ a=b `` '\$' && z""" + @test Base.shell_escape_posixly(`$cmd`) == + """'foo bar' baz a\\'b a\\"b 'a"b"c' -L/usr/+ a=b '``' '\$' '&&' z""" end +let cmd = ["foo=bar", "baz"] + @test string(`$cmd`) == "`foo=bar baz`" + @test Base.shell_escape(`$cmd`) == "foo=bar baz" + @test Base.shell_escape_posixly(`$cmd`) == "'foo=bar' baz" +end + @test Base.shell_split("\"\\\\\"") == ["\\"]