Create argv before fork (#16286)

Reduces code executing between `fork` and `exec`. Most importantly, removes the unsafe allocation from `command_args.map`.

Author: straight-shoota

src/crystal/system/process.cr

@@ -74,10 +74,10 @@ struct Crystal::System::Process
   # def self.fork(&)
 
   # Launches a child process with the command + args.
-  # def self.spawn(command_args : Args, env : Env?, clear_env : Bool, input : Stdio, output : Stdio, error : Stdio, chdir : Path | String?) : ProcessInformation
+  # def self.spawn(prepared_args : Args, 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_args : Args, env : Env?, clear_env : Bool, input : Stdio, output : Stdio, error : Stdio, chdir : Path | String?) : NoReturn
+  # def self.replace(command : String, prepared_args : Args, env : Env?, clear_env : Bool, input : Stdio, output : Stdio, error : Stdio, chdir : Path | String?) : NoReturn
 
   # Converts a command and array of arguments to the system-specific representation.
   # def self.prepare_args(command : String, args : Enumerable(String)?, shell : Bool) : Args

src/crystal/system/unix/process.cr

@@ -254,7 +254,7 @@ struct Crystal::System::Process
     end
   end
 
-  def self.spawn(command_args, env, clear_env, input, output, error, chdir)
+  def self.spawn(prepared_args, env, clear_env, input, output, error, chdir)
     r, w = FileDescriptor.system_pipe
 
     pid = fork(will_exec: true) do
@@ -266,7 +266,7 @@ struct Crystal::System::Process
     if !pid
       LibC.close(r)
       begin
-        self.try_replace(command_args, env, clear_env, input, output, error, chdir)
+        self.try_replace(prepared_args, env, clear_env, input, output, error, chdir)
         byte = 1_u8
         errno = Errno.value.to_i32
         FileDescriptor.write_fully(w, pointerof(byte))
@@ -292,15 +292,15 @@ struct Crystal::System::Process
       when 0
         # Error message coming
         message = reader_pipe.gets_to_end
-        raise RuntimeError.new("Error executing process: '#{command_args[0]}': #{message}")
+        raise RuntimeError.new("Error executing process: '#{prepared_args[0]}': #{message}")
       when 1
         # Errno coming
         # can't use IO#read_bytes(Int32) because we skipped system/network
         # endianness check when writing the integer while read_bytes would;
         # we thus read it in the same as order as written
         buf = uninitialized StaticArray(UInt8, 4)
         reader_pipe.read_fully(buf.to_slice)
-        raise_exception_from_errno(command_args[0], Errno.new(buf.unsafe_as(Int32)))
+        raise_exception_from_errno(prepared_args[0], Errno.new(buf.unsafe_as(Int32)))
       else
         raise RuntimeError.new("BUG: Invalid error response received from subprocess")
       end
@@ -311,28 +311,30 @@ struct Crystal::System::Process
     pid
   end
 
-  def self.prepare_args(command : String, args : Enumerable(String)?, shell : Bool) : Array(String)
+  def self.prepare_args(command : String, args : Enumerable(String)?, shell : Bool) : {String, LibC::Char**}
     if shell
       command = %(#{command} "${@}") unless command.includes?(' ')
-      shell_args = ["/bin/sh", "-c", command, "sh"]
+      argv_ary = ["/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
-
-        shell_args.concat(args)
       end
 
-      shell_args
+      pathname = "/bin/sh"
     else
-      command_args = [command]
-      command_args.concat(args) if args
-      command_args
+      argv_ary = [command]
+      pathname = command
     end
+
+    argv_ary.concat(args) if args
+
+    argv = argv_ary.map(&.check_no_null_byte.to_unsafe)
+    {pathname, argv.to_unsafe}
   end
 
-  private def self.try_replace(command_args, env, clear_env, input, output, error, chdir)
+  private def self.try_replace(prepared_args, env, clear_env, input, output, error, chdir)
     reopen_io(input, ORIGINAL_STDIN)
     reopen_io(output, ORIGINAL_STDOUT)
     reopen_io(error, ORIGINAL_STDERR)
@@ -348,11 +350,7 @@ struct Crystal::System::Process
 
     ::Dir.cd(chdir) if chdir
 
-    command = command_args[0]
-    argv = command_args.map &.check_no_null_byte.to_unsafe
-    argv << Pointer(UInt8).null
-
-    lock_write { execvpe(command, argv, LibC.environ) }
+    lock_write { LibC.execvp(*prepared_args) }
   end
 
   private def self.execvpe(command, argv, envp)
@@ -373,9 +371,9 @@ struct Crystal::System::Process
     LibC.execvp(command, argv)
   end
 
-  def self.replace(command_args, env, clear_env, input, output, error, chdir)
-    try_replace(command_args, env, clear_env, input, output, error, chdir)
-    raise_exception_from_errno(command_args[0])
+  def self.replace(command, prepared_args, env, clear_env, input, output, error, chdir)
+    try_replace(prepared_args, env, clear_env, input, output, error, chdir)
+    raise_exception_from_errno(command)
   end
 
   private def self.raise_exception_from_errno(command, errno = Errno.value)

src/crystal/system/wasi/process.cr

@@ -88,15 +88,15 @@ struct Crystal::System::Process
     raise NotImplementedError.new("Process.fork")
   end
 
-  def self.spawn(command_args, env, clear_env, input, output, error, chdir)
+  def self.spawn(prepared_args, env, clear_env, input, output, error, chdir)
     raise NotImplementedError.new("Process.spawn")
   end
 
   def self.prepare_args(command : String, args : Enumerable(String)?, shell : Bool) : Array(String)
     raise NotImplementedError.new("Process.prepare_args")
   end
 
-  def self.replace(command_args, env, clear_env, input, output, error, chdir)
+  def self.replace(command, prepared_args, env, clear_env, input, output, error, chdir)
     raise NotImplementedError.new("Process.replace")
   end
 

src/crystal/system/win32/process.cr

@@ -282,7 +282,7 @@ struct Crystal::System::Process
     {new_handle, dup_handle}
   end
 
-  def self.spawn(command_args, env, clear_env, input, output, error, chdir)
+  def self.spawn(prepared_args, env, clear_env, input, output, error, chdir)
     startup_info = LibC::STARTUPINFOW.new
     startup_info.cb = sizeof(LibC::STARTUPINFOW)
     startup_info.dwFlags = LibC::STARTF_USESTDHANDLES
@@ -293,19 +293,19 @@ struct Crystal::System::Process
 
     process_info = LibC::PROCESS_INFORMATION.new
 
-    command_args = ::Process.quote_windows(command_args) unless command_args.is_a?(String)
+    prepared_args = ::Process.quote_windows(prepared_args) unless prepared_args.is_a?(String)
 
     if LibC.CreateProcessW(
-         nil, System.to_wstr(command_args), nil, nil, true, LibC::CREATE_SUSPENDED | LibC::CREATE_UNICODE_ENVIRONMENT,
+         nil, System.to_wstr(prepared_args), nil, nil, true, LibC::CREATE_SUSPENDED | LibC::CREATE_UNICODE_ENVIRONMENT,
          make_env_block(env, clear_env), chdir.try { |str| System.to_wstr(str) } || Pointer(UInt16).null,
          pointerof(startup_info), pointerof(process_info)
        ) == 0
       error = WinError.value
       case error.to_errno
       when Errno::EACCES, Errno::ENOENT, Errno::ENOEXEC
-        raise ::File::Error.from_os_error("Error executing process", error, file: command_args)
+        raise ::File::Error.from_os_error("Error executing process", error, file: prepared_args)
       else
-        raise IO::Error.from_os_error("Error executing process: '#{command_args}'", error)
+        raise IO::Error.from_os_error("Error executing process: '#{prepared_args}'", error)
       end
     end
 
@@ -337,13 +337,13 @@ struct Crystal::System::Process
         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
-      command_args
+      prepared_args = [command]
+      prepared_args.concat(args) if args
+      prepared_args
     end
   end
 
-  private def self.try_replace(command_args, env, clear_env, input, output, error, chdir)
+  private def self.try_replace(command, prepared_args, env, clear_env, input, output, error, chdir)
     old_input_fd = reopen_io(input, ORIGINAL_STDIN)
     old_output_fd = reopen_io(output, ORIGINAL_STDOUT)
     old_error_fd = reopen_io(error, ORIGINAL_STDERR)
@@ -359,12 +359,12 @@ struct Crystal::System::Process
 
     ::Dir.cd(chdir) if chdir
 
-    if command_args.is_a?(String)
-      command = System.to_wstr(command_args)
+    if prepared_args.is_a?(String)
+      command = System.to_wstr(prepared_args)
       argv = [command]
     else
-      command = System.to_wstr(command_args[0])
-      argv = command_args.map { |arg| System.to_wstr(arg) }
+      command = System.to_wstr(prepared_args[0])
+      argv = prepared_args.map { |arg| System.to_wstr(arg) }
     end
     argv << Pointer(LibC::WCHAR).null
 
@@ -378,9 +378,9 @@ struct Crystal::System::Process
     errno
   end
 
-  def self.replace(command_args, env, clear_env, input, output, error, chdir) : NoReturn
-    errno = try_replace(command_args, env, clear_env, input, output, error, chdir)
-    raise_exception_from_errno(command_args.is_a?(String) ? command_args : command_args[0], errno)
+  def self.replace(command, prepared_args, env, clear_env, input, output, error, chdir) : NoReturn
+    errno = try_replace(command, prepared_args, env, clear_env, input, output, error, chdir)
+    raise_exception_from_errno(prepared_args.is_a?(String) ? prepared_args : prepared_args[0], errno)
   end
 
   private def self.raise_exception_from_errno(command, errno = Errno.value)

src/process.cr

@@ -215,13 +215,13 @@ class Process
   # Raises `IO::Error` if executing the command fails (for example if the executable doesn't exist).
   def self.exec(command : String, args : Enumerable(String)? = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false,
                 input : ExecStdio = Redirect::Inherit, output : ExecStdio = Redirect::Inherit, error : ExecStdio = Redirect::Inherit, chdir : Path | String? = nil) : NoReturn
-    command_args = Crystal::System::Process.prepare_args(command, args, shell)
+    prepared_args = Crystal::System::Process.prepare_args(command, args, shell)
 
     input = exec_stdio_to_fd(input, for: STDIN)
     output = exec_stdio_to_fd(output, for: STDOUT)
     error = exec_stdio_to_fd(error, for: STDERR)
 
-    Crystal::System::Process.replace(command_args, env, clear_env, input, output, error, chdir)
+    Crystal::System::Process.replace(command, prepared_args, env, clear_env, input, output, error, chdir)
   end
 
   private def self.exec_stdio_to_fd(stdio : ExecStdio, for dst_io : IO::FileDescriptor) : IO::FileDescriptor
@@ -280,13 +280,13 @@ class Process
   # Raises `IO::Error` if executing the command fails (for example if the executable doesn't exist).
   def initialize(command : String, args : Enumerable(String)? = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false,
                  input : Stdio = Redirect::Close, output : Stdio = Redirect::Close, error : Stdio = Redirect::Close, chdir : Path | String? = nil)
-    command_args = Crystal::System::Process.prepare_args(command, args, shell)
+    prepared_args = Crystal::System::Process.prepare_args(command, args, shell)
 
     fork_input = stdio_to_fd(input, for: STDIN)
     fork_output = stdio_to_fd(output, for: STDOUT)
     fork_error = stdio_to_fd(error, for: STDERR)
 
-    pid = Crystal::System::Process.spawn(command_args, env, clear_env, fork_input, fork_output, fork_error, chdir.try &.to_s)
+    pid = Crystal::System::Process.spawn(prepared_args, 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)