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

deprecate unescaped shell special chars in commands #19786

Merged
merged 4 commits into from
Jan 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions base/managers.jl
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,13 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched,

# the default worker timeout
tval = haskey(ENV, "JULIA_WORKER_TIMEOUT") ?
`export JULIA_WORKER_TIMEOUT=$(ENV["JULIA_WORKER_TIMEOUT"]);` : ``
`export JULIA_WORKER_TIMEOUT=$(ENV["JULIA_WORKER_TIMEOUT"])\;` : ``

# Julia process with passed in command line flag arguments
cmd = `cd $dir && $tval $exename $exeflags`
cmd = `cd $dir '&&' $tval $exename $exeflags`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is in the ssh code, do we test this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. @amitmurthy, @tanmaykm?

Copy link
Contributor

@tkelman tkelman Dec 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be behind the full-tests env var flag? or remote only...

we could add some simple tests that sending quoted/escaped special chars to programs that should be able to use them still act as expected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wish to test, set JULIA_TESTFULL=1 in your env. Enable password less ssh login into your local machine. Run the parallel test. This will test SSH Manager.

Else I can test this locally in a couple of days.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addprocs hangs on the open call with this change. Printing cmd just before the open shows that && is not treated correctly.

`ssh -T -a -x -o ClearAllForwardings=yes -n localhost "sh -l -c \"cd /Users/amitm/Julia/julia '&&' /Users/amitm/Julia/julia/usr/bin/julia --worker ehqbRgqspml7nZLg\""`


# shell login (-l) with string command (-c) to launch julia process
cmd = `sh -l -c $(shell_escape(cmd))`
cmd = `sh -l -c $(shell_escape(cmd, special = ""))`

# remote launch with ssh with given ssh flags / host / port information
# -T → disable pseudo-terminal allocation
Expand All @@ -195,7 +195,7 @@ function launch_on_machine(manager::SSHManager, machine, cnt, params, launched,
# forwarded connections are causing collisions
# -n → Redirects stdin from /dev/null (actually, prevents reading from stdin).
# Used when running ssh in the background.
cmd = `ssh -T -a -x -o ClearAllForwardings=yes -n $sshflags $host $(shell_escape(cmd))`
cmd = `ssh -T -a -x -o ClearAllForwardings=yes -n $sshflags $host $(shell_escape(cmd, special = ""))`

# launch the remote Julia process

Expand Down
3 changes: 2 additions & 1 deletion base/process.jl
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ end
hash(x::AndCmds, h::UInt) = hash(x.a, hash(x.b, h))
==(x::AndCmds, y::AndCmds) = x.a == y.a && x.b == y.b

shell_escape(cmd::Cmd) = shell_escape(cmd.exec...)
shell_escape(cmd::Cmd; special::AbstractString=shell_special) =
shell_escape(cmd.exec..., special=special)

function show(io::IO, cmd::Cmd)
print_env = cmd.env !== nothing
Expand Down
54 changes: 37 additions & 17 deletions base/shell.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

## shell-like command parsing ##

function shell_parse(raw::AbstractString, interp::Bool)
s = lstrip(raw)
#Strips the end but respects the space when the string endswith "\\ "
const shell_special = "#{}()[]<>|&*?~;"

function shell_parse(str::AbstractString, interpolate::Bool=true)
s = lstrip(str)
# strips the end but respects the space when the string ends with "\\ "
r = RevString(s)
i = start(r)
c_old = nothing
Expand All @@ -22,7 +24,7 @@ function shell_parse(raw::AbstractString, interp::Bool)
s = s[1:end-i+1]

last_parse = 0:-1
isempty(s) && return interp ? (Expr(:tuple,:()),last_parse) : ([],last_parse)
isempty(s) && return interpolate ? (Expr(:tuple,:()),last_parse) : ([],last_parse)

in_single_quotes = false
in_double_quotes = false
Expand Down Expand Up @@ -57,7 +59,7 @@ function shell_parse(raw::AbstractString, interp::Bool)
end
j = k
end
elseif interp && !in_single_quotes && c == '$'
elseif interpolate && !in_single_quotes && c == '$'
update_arg(s[i:j-1]); i = k; j = k
if done(s,k)
error("\$ right before end of command")
Expand Down Expand Up @@ -92,6 +94,8 @@ function shell_parse(raw::AbstractString, interp::Bool)
update_arg(s[i:j-1]); i = k
c, k = next(s,k)
end
elseif !in_single_quotes && !in_double_quotes && c in shell_special
depwarn("special characters \"$shell_special\" should now be quoted in commands", :shell_parse)
end
j = k
end
Expand All @@ -103,18 +107,15 @@ function shell_parse(raw::AbstractString, interp::Bool)
update_arg(s[i:end])
append_arg()

if !interp
return (args,last_parse)
end
interpolate || return args, last_parse

# construct an expression
ex = Expr(:tuple)
for arg in args
push!(ex.args, Expr(:tuple, arg...))
end
(ex,last_parse)
return ex, last_parse
end
shell_parse(s::AbstractString) = shell_parse(s,true)

function shell_split(s::AbstractString)
parsed = shell_parse(s,false)[1]
Expand All @@ -125,14 +126,14 @@ function shell_split(s::AbstractString)
args
end

function print_shell_word(io::IO, word::AbstractString)
function print_shell_word(io::IO, word::AbstractString, special::AbstractString = shell_special)
if isempty(word)
print(io, "''")
end
has_single = false
has_special = false
for c in word
if isspace(c) || c=='\\' || c=='\'' || c=='"' || c=='$'
if isspace(c) || c=='\\' || c=='\'' || c=='"' || c=='$' || c in special
has_special = true
if c == '\''
has_single = true
Expand All @@ -155,13 +156,32 @@ function print_shell_word(io::IO, word::AbstractString)
end
end

function print_shell_escaped(io::IO, cmd::AbstractString, args::AbstractString...)
print_shell_word(io, cmd)
function print_shell_escaped(
io::IO, cmd::AbstractString, args::AbstractString...;
special::AbstractString=shell_special
)
print_shell_word(io, cmd, special)
for arg in args
print(io, ' ')
print_shell_word(io, arg)
print_shell_word(io, arg, special)
end
end
print_shell_escaped(io::IO) = nothing
print_shell_escaped(io::IO; special::String=shell_special) = nothing

"""
shell_escape(args::Union{Cmd,AbstractString...}; special::AbstractString="$shell_special")

The unexported `shell_escape` function is the inverse of the unexported `shell_split` function:
it takes a string or command object and escapes any special characters in such a way that calling
`shell_split` on it would give back the array of words in the original command. The `special`
keyword argument controls what characters in addition to whitespace, backslashes, quotes and
dollar signs are considered to be special. Examples:

julia> Base.shell_escape("echo", "this", "&&", "that")
"echo this '&&' that"

shell_escape(args::AbstractString...) = sprint(print_shell_escaped, args...)
julia> Base.shell_escape("cat", "/foo/bar baz", "&&", "echo", "done", special="")
"cat '/foo/bar baz' && echo done"
"""
shell_escape(args::AbstractString...; special::AbstractString=shell_special) =
sprint(io->print_shell_escaped(io, args..., special=special))
2 changes: 1 addition & 1 deletion test/replcompletions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ for (T, arg) in [(String,"\")\""),(Char, "')'")]
@test s[r] == "CompletionFoo.test2"
end

s = "(1, CompletionFoo.test2(`)`,"
s = "(1, CompletionFoo.test2(`')'`,"
c, r, res = test_complete(s)
@test c[1] == string(first(methods(Main.CompletionFoo.test2, Tuple{Cmd})))
@test length(c) == 1
Expand Down
2 changes: 1 addition & 1 deletion test/spawn.jl
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ end

#### Examples used in the manual ####

@test readstring(`$echo hello | sort`) == "hello | sort\n"
@test readstring(`$echo hello \| sort`) == "hello | sort\n"
@test readstring(pipeline(`$echo hello`, sortcmd)) == "hello\n"
@test length(spawn(pipeline(`$echo hello`, sortcmd)).processes) == 2

Expand Down