Skip to content
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
77 changes: 76 additions & 1 deletion spec/std/process_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,89 @@ private def newline
{% end %}
end

private def to_ary(tuple)
[tuple[0]].concat(tuple[1])
end

# interpreted code doesn't receive SIGCHLD for `#wait` to work (#12241)
{% if flag?(:interpreted) && !flag?(:win32) %}
pending Process
{% skip_file %}
{% end %}

describe Process do
describe ".new" do
describe ".new (args)" do
it "raises if args is empty" do
expect_raises(File::NotFoundError, "Error executing process: No command") do
Process.new([] of String)
end
end

it "raises if args[0] is empty" do
expect_raises(IO::Error, /Error executing process: '("")?'/) do
Process.new([""] of String)
end
end

it "raises if command doesn't exist" do
expect_raises(File::NotFoundError, "Error executing process: 'foobarbaz'") do
Process.new(["foobarbaz"])
end
end

it "raises for long path" do
expect_raises(File::NotFoundError, "Error executing process: 'aaaaaaa") do
Process.new(["a" * 1000])
end
end

it "accepts nilable string for `chdir` (#13767)" do
expect_raises(File::NotFoundError, "Error executing process: 'foobarbaz'") do
Process.new(["foobarbaz"], chdir: nil.as(String?))
end
end

it "raises if command is not executable" do
with_tempfile("crystal-spec-run") do |path|
File.touch path
expect_raises({% if flag?(:win32) %} File::BadExecutableError {% else %} File::AccessDeniedError {% end %}, "Error executing process: '#{path.inspect_unquoted}'") do
Process.new([path])
end
end
end

it "raises if command is not executable" do
with_tempfile("crystal-spec-run") do |path|
Dir.mkdir path
expect_raises(File::AccessDeniedError, "Error executing process: '#{path.inspect_unquoted}'") do
Process.new([path])
end
end
end

it "raises if command could not be executed" do
with_tempfile("crystal-spec-run") do |path|
File.touch path
command = File.join(path, "foo")
expect_raises(IO::Error, "Error executing process: '#{command.inspect_unquoted}'") do
Process.new([command])
end
end
end

it "doesn't break if process is collected before completion", tags: %w[slow] do
200.times { Process.new(to_ary(exit_code_command(0))) }

# run the GC multiple times to unmap as much memory as possible
10.times { GC.collect }

# the processes above have now been queued after completion; if this last
# one finishes at all, nothing was broken by the GC
Process.run(*exit_code_command(0))
end
end

describe ".new (command + args)" do
it "raises if command doesn't exist" do
expect_raises(File::NotFoundError, "Error executing process: 'foobarbaz'") do
Process.new("foobarbaz")
Expand Down
5 changes: 4 additions & 1 deletion src/crystal/system/process.cr
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@ struct Crystal::System::Process
# def self.fork : ProcessInformation
# def self.fork(&)

# def prepare_args(command : String, args : Enumerable(String)?, shell : Bool) : Args
# def prepare_args(args : Enumerable(String)) : Args

# Launches a child process with the command + args.
# def self.spawn(command : String, args : Enumerable(String)?, shell : Bool, env : Env?, clear_env : Bool, input : Stdio, output : Stdio, error : Stdio, chdir : Path | String?) : ProcessInformation
# def self.spawn(prepared_args : Args, shell : Bool, env : Env?, clear_env : Bool, input : Stdio, output : Stdio, error : Stdio, chdir : Path | String?) : ProcessInformation

# Replaces the current process with a new one.
# def self.replace(command : String, args : Enumerable(String)?, shell : Bool, env : Env?, clear_env : Bool, input : Stdio, output : Stdio, error : Stdio, chdir : Path | String?) : NoReturn
Expand Down
16 changes: 9 additions & 7 deletions src/crystal/system/unix/process.cr
Original file line number Diff line number Diff line change
Expand Up @@ -292,23 +292,25 @@ struct Crystal::System::Process
def self.prepare_args(command : String, args : Enumerable(String)?, shell : Bool) : {String, LibC::Char**}
if shell
command = %(#{command} "${@}") unless command.includes?(' ')
argv_ary = ["/bin/sh", "-c", command, "sh"]
command_args = ["/bin/sh", "-c", command, "sh"]

if args
unless command.includes?(%("${@}"))
raise ArgumentError.new(%(Can't specify arguments in both command and args without including "${@}" into your command))
end
end

pathname = "/bin/sh"
else
argv_ary = [command]
pathname = command
command_args = [command]
end

argv_ary.concat(args) if args
command_args.concat(args) if args

prepare_args(command_args)
end

argv = argv_ary.map(&.check_no_null_byte.to_unsafe)
def self.prepare_args(args : Enumerable(String)) : {String, LibC::Char**}
pathname = args.first
argv = args.map(&.check_no_null_byte.to_unsafe)
{pathname, argv.to_unsafe}
end

Expand Down
4 changes: 1 addition & 3 deletions src/crystal/system/unix/spawn.cr
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ require "c/signal"
require "c/unistd"

struct Crystal::System::Process
def self.spawn(command, args, shell, env, clear_env, input, output, error, chdir)
prepared_args = prepare_args(command, args, shell)

def self.spawn(prepared_args, shell, env, clear_env, input, output, error, chdir)
r, w = FileDescriptor.system_pipe

envp = Env.make_envp(env, clear_env)
Expand Down
32 changes: 18 additions & 14 deletions src/crystal/system/win32/process.cr
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ struct Crystal::System::Process
{new_handle, dup_handle}
end

def self.spawn(command, args, shell, env, clear_env, input, output, error, chdir)
def self.spawn(prepared_args, shell, env, clear_env, input, output, error, chdir)
startup_info = LibC::STARTUPINFOW.new
startup_info.cb = sizeof(LibC::STARTUPINFOW)
startup_info.dwFlags = LibC::STARTF_USESTDHANDLES
Expand All @@ -297,7 +297,6 @@ struct Crystal::System::Process

process_info = LibC::PROCESS_INFORMATION.new

prepared_args = prepare_args(command, args, shell)
prepared_args = ::Process.quote_windows(prepared_args) unless prepared_args.is_a?(String)

if LibC.CreateProcessW(
Expand Down Expand Up @@ -331,20 +330,25 @@ struct Crystal::System::Process
end
command
else
# Disable implicit execution of batch files (https://github.com/crystal-lang/crystal/issues/14536)
#
# > `CreateProcessW()` implicitly spawns `cmd.exe` when executing batch files (`.bat`, `.cmd`, etc.), even if the application didn’t specify them in the command line.
# > The problem is that the `cmd.exe` has complicated parsing rules for the command arguments, and programming language runtimes fail to escape the command arguments properly.
# > Because of this, it’s possible to inject commands if someone can control the part of command arguments of the batch file.
# https://flatt.tech/research/posts/batbadbut-you-cant-securely-execute-commands-on-windows/
if command.rstrip(". ").byte_slice?(-4, 4).try(&.downcase).in?(".bat", ".cmd")
raise ::File::Error.from_os_error("Error executing process", WinError::ERROR_BAD_EXE_FORMAT, file: command)
end
command_args = [command]
command_args.concat(args) if args
prepare_args(command_args)
end
end

prepared_args = [command]
prepared_args.concat(args) if args
prepared_args
def self.prepare_args(args : Enumerable(String))
# Disable implicit execution of batch files (https://github.com/crystal-lang/crystal/issues/14536)
#
# > `CreateProcessW()` implicitly spawns `cmd.exe` when executing batch files (`.bat`, `.cmd`, etc.), even if the application didn’t specify them in the command line.
# > The problem is that the `cmd.exe` has complicated parsing rules for the command arguments, and programming language runtimes fail to escape the command arguments properly.
# > Because of this, it’s possible to inject commands if someone can control the part of command arguments of the batch file.
# https://flatt.tech/research/posts/batbadbut-you-cant-securely-execute-commands-on-windows/
command = args.first
if command.rstrip(". ").byte_slice?(-4, 4).try(&.downcase).in?(".bat", ".cmd")
raise ::File::Error.from_os_error("Error executing process", WinError::ERROR_BAD_EXE_FORMAT, file: command)
end

args
end

private def self.try_replace(command, prepared_args, env, clear_env, input, output, error, chdir)
Expand Down
74 changes: 69 additions & 5 deletions src/process.cr
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,67 @@ class Process
@process_info : Crystal::System::Process
@wait_count = 0

# Creates and executes a child process.
#
# This starts a new process for the command given in *args[0]*.
#
# The command is either a path to the executable to run, or the name of an
# executable which is then looked up by the operating system.
# The lookup uses the `PATH` variable of the current process environment
# (i.e. `ENV["PATH"]).
# In order to resolve to a specific executable, provide a path instead of
# only a command name. `Process.find_executable` can help with looking up a
# command in a custom `PATH`.
#
# The following arguments in *args* are passed as arguments to the child process.
#
# Raises `IO::Error` if executing *args[0]* fails, for example because the
# executable doesn't exist or is not executable.
#
# *env* provides a mapping of environment variables for the child process.
# If *clear_env* is `true`, only these explicit variables are used; if `false`,
# the child inherits the parent's environment with *env* merged.
#
# *input*, *output*, *error* configure the child process's standard streams.
# * `Redirect::Close` passes the null device
# * `Redirect::Pipe` creates a pipe that's accessible via `#input`, `#output`
# or `#error`.
# * `Redirect::Inherit` to share the parent's streams (`STDIN`, `STDOUT`, `STDERR`).
# * An `IO` instance creates a pipe that reads/writes into the given IO.
#
# *chdir* changes the working directory of the child process. If `nil`, uses
# the current working directory of the parent process.
#
# Example:
#
# ```
# process = Process.new(["echo", "Hello"], output: Process::Redirect::Pipe)
# process.output.gets_to_end # => "Hello\n"
# process.wait # => Process::Status[0]
# ```
#
# Similar methods:
#
# * `Process.run` is a convenient short cut if you just want to run a command
# and wait for it to finish.
# * `Process.exec` replaces the current process.
def initialize(args : Enumerable(String), *, env : Env = nil, clear_env : Bool = false,
input : Stdio = Redirect::Close, output : Stdio = Redirect::Close, error : Stdio = Redirect::Close, chdir : Path | String? = nil)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice touch to not include the shell argument 👍

Now if we could reduce the duplication between both constructors... Ideally the old one should merely wrap the new one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We cannot just make it a wrapper because the new one does not support shell feature.
Refactoring to remove duplication is also not easy because it's an initializer and involves instance variable setup. So I opted to leave it at that for now. It's acceptable to have a bit duplication.

raise File::NotFoundError.new("Error executing process: No command", file: "") if args.empty?

fork_input = stdio_to_fd(input, for: STDIN)
fork_output = stdio_to_fd(output, for: STDOUT)
fork_error = stdio_to_fd(error, for: STDERR)

prepared_args = Crystal::System::Process.prepare_args(args)
pid = Crystal::System::Process.spawn(prepared_args, false, env, clear_env, fork_input, fork_output, fork_error, chdir.try &.to_s)
@process_info = Crystal::System::Process.new(pid)

fork_input.close unless fork_input.in?(input, STDIN)
fork_output.close unless fork_output.in?(output, STDOUT)
fork_error.close unless fork_error.in?(error, STDERR)
end

# Creates and executes a child process.
#
# This starts a new process for `command`.
Expand Down Expand Up @@ -359,7 +420,8 @@ class Process
fork_output = stdio_to_fd(output, for: STDOUT)
fork_error = stdio_to_fd(error, for: STDERR)

pid = Crystal::System::Process.spawn(command, args, shell, env, clear_env, fork_input, fork_output, fork_error, chdir.try &.to_s)
prepared_args = Crystal::System::Process.prepare_args(command, args, shell)
pid = Crystal::System::Process.spawn(prepared_args, shell, env, clear_env, fork_input, fork_output, fork_error, chdir.try &.to_s)
@process_info = Crystal::System::Process.new(pid)

fork_input.close unless fork_input.in?(input, STDIN)
Expand Down Expand Up @@ -436,10 +498,12 @@ class Process
fork_io
end

# :nodoc:
def initialize(pid : LibC::PidT)
@process_info = Crystal::System::Process.new(pid)
end
{% unless flag?(:interpreted) %}
# :nodoc:
def initialize(pid : LibC::PidT)
@process_info = Crystal::System::Process.new(pid)
end
{% end %}

# Sends *signal* to this process.
#
Expand Down