Skip to content

Commit

Permalink
Fix spawn(..., fd => fd) on macOS
Browse files Browse the repository at this point in the history
* Rename redirect arguments for clarity.
  • Loading branch information
eregon committed Oct 12, 2022
1 parent a02c9a3 commit 448ba1c
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ New features:

Bug fixes:

* Fix `spawn(..., fd => fd)` on macOS, it did not work due to a macOS bug (@eregon).

Compatibility:

Expand Down
48 changes: 30 additions & 18 deletions src/main/ruby/truffleruby/core/truffle/process_operations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,9 +254,9 @@ def parse_options(options)
options.each do |key, value|
case key
when IO, Integer, :in, :out, :err
from = convert_io_fd key
to = convert_to_fd value, from
redirect from, to
child_fd = convert_io_fd key
parent_fd = convert_to_fd value, child_fd
redirect child_fd, parent_fd
when Array

# When redirecting multiple fds to one file, as in
Expand All @@ -279,8 +279,8 @@ def parse_options(options)
# duplicate the string containing the file path.

fds = key.map { |k| convert_io_fd(k) }
to = convert_to_fd value, fds.first
fds.each { |fd| redirect fd, to }
parent_fd = convert_to_fd value, fds.first
fds.each { |fd| redirect fd, parent_fd }
when :unsetenv_others
if value
@options[:unsetenv_others] = true
Expand All @@ -307,13 +307,25 @@ def parse_options(options)
end
end

def redirect(from, to)
def redirect(child_fd, parent_fd)
map = (@options[:redirect_fd] ||= [])
if to.nil?
@options[:fds_to_close] ||= []
@options[:fds_to_close] << from
if parent_fd.nil?
fds_to_close = (@options[:fds_to_close] ||= [])
fds_to_close << child_fd
elsif child_fd == parent_fd
# posix_spawn_file_actions_adddup2() noops (and so keeps the FD_CLOEXEC flag) on older OS and macOS:
# https://pubs.opengroup.org/onlinepubs/000095399/functions/posix_spawn_file_actions_adddup2.html
# This means the fd is actually closed in the child process.
# Workaround by dup'ing the fd so it's never noop, and it has no FD_CLOEXEC flag, as needed to use it in the child.

# autoclose: false means don't actually close(2) on IO#close (this IO instance does not own the fd)
io = IO.for_fd(parent_fd, autoclose: false)
# copies with F_DUPFD_CLOEXEC, which is nice to automatically close the copy in the child
copy = io.dup
@files_for_child << copy
map << child_fd << copy.fileno
else
map << from << to
map << child_fd << parent_fd
end
end

Expand Down Expand Up @@ -460,8 +472,8 @@ def spawn_setup(alter_process)
end

if redirect_fd = @options[:redirect_fd]
redirect_fd.each_slice(2) do |from, to|
redirect_file_descriptor(from, to, alter_process)
redirect_fd.each_slice(2) do |child_fd, parent_fd|
redirect_file_descriptor(child_fd, parent_fd, alter_process)
end
end
end
Expand All @@ -474,15 +486,15 @@ def safe_close_on_exec?(fd)
(flags & File::FD_CLOEXEC) != 0
end

def redirect_file_descriptor(from, to, alter_process)
to = (-to + 1) if to < 0
if alter_process && from == to
flags = Truffle::POSIX.fcntl(from, File::F_GETFD, 0)
def redirect_file_descriptor(child_fd, parent_fd, alter_process)
parent_fd = (-parent_fd + 1) if parent_fd < 0
if alter_process && child_fd == parent_fd
flags = Truffle::POSIX.fcntl(child_fd, File::F_GETFD, 0)
if flags >= 0 && (flags & File::FD_CLOEXEC) != 0
Truffle::POSIX.fcntl(from, File::F_SETFD, flags ^ File::FD_CLOEXEC)
Truffle::POSIX.fcntl(child_fd, File::F_SETFD, flags ^ File::FD_CLOEXEC)
end
end
result = Truffle::POSIX.dup2(to, from)
result = Truffle::POSIX.dup2(parent_fd, child_fd)
Errno.handle if result < 0
end

Expand Down

0 comments on commit 448ba1c

Please sign in to comment.