Skip to content

Commit

Permalink
[Experimental] Pitchfork::Info.close_all_fds!
Browse files Browse the repository at this point in the history
Ensuring that an application is fork safe can be complex, and
in case of mistake, one of the error condition is shared file
descriptors, causing one worker to read packets from another
which could have dire consequences.

Rather than to track down all the file descriptors that need
reopening, `close_all_fds!` allows to automatically close all
file descriptors that aren't explicitly marked as needing to
be kept.

And in case of mistake, the error condition is that trying to
read or write into that file descriptor will fail, which is
much less critical than to read packets from another worker.
  • Loading branch information
byroot committed Aug 1, 2023
1 parent 986c61b commit 5f65286
Show file tree
Hide file tree
Showing 8 changed files with 47 additions and 8 deletions.
4 changes: 4 additions & 0 deletions lib/pitchfork/flock.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def at_fork
nil
end

def to_io
@file
end

def unlink
File.unlink(@file.path)
rescue Errno::ENOENT
Expand Down
3 changes: 3 additions & 0 deletions lib/pitchfork/http_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ def initialize(app, options = {})
@respawn = false
@last_check = Pitchfork.time_now
@promotion_lock = Flock.new("pitchfork-promotion")
Info.keep_io(@promotion_lock)

options = options.dup
@ready_pipe = options.delete(:ready_pipe)
Expand Down Expand Up @@ -180,6 +181,7 @@ def start(sync = true)
# It's also used by newly spawned children to send their soft_signal pipe
# to the master when they are spawned.
@control_socket.replace(Pitchfork.socketpair)
Info.keep_ios(@control_socket)
@master_pid = $$

# setup signal handlers before writing pid file in case people get
Expand Down Expand Up @@ -264,6 +266,7 @@ def listen(address, opt = {}.merge(listener_opts[address] || {}))
io = server_cast(io)
end
logger.info "listening on addr=#{sock_name(io)} fd=#{io.fileno}"
Info.keep_io(io)
LISTENERS << io
io
rescue Errno::EADDRINUSE => err
Expand Down
30 changes: 30 additions & 0 deletions lib/pitchfork/info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,40 @@ module Pitchfork
module Info
@workers_count = 0
@fork_safe = true
@kept_ios = ObjectSpace::WeakMap.new

class << self
attr_accessor :workers_count

def keep_io(io)
@kept_ios[io] = io if io && !io.to_io.closed?
io
end

def keep_ios(ios)
ios.each { |io| keep_io(io) }
end

def close_all_fds!
ignored_fds = [$stdin.to_i, $stdout.to_i, $stderr.to_i]
@kept_ios.each_value do |io_like|
if io = io_like&.to_io
ignored_fds << io.to_i unless io.closed?
end
end

all_fds = Dir.children("/dev/fd").map(&:to_i)
all_fds -= ignored_fds

all_fds.each do |fd|
IO.for_fd(fd).close
rescue ArgumentError
# RubyVM internal file descriptor, leave it alone
rescue Errno::EBADF
# Likely a race condition
end
end

def fork_safe?
@fork_safe
end
Expand Down
3 changes: 2 additions & 1 deletion lib/pitchfork/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ def close # :nodoc:
end

def create_socketpair!
@to_io, @master = Pitchfork.socketpair
@to_io, @master = Info.keep_ios(Pitchfork.socketpair)
end

def after_fork_in_child
Expand All @@ -208,6 +208,7 @@ def after_fork_in_child
private

def pipe=(socket)
Info.keep_io(socket)
@master = MessageSocket.new(socket)
end

Expand Down
1 change: 1 addition & 0 deletions test/slow/test_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class WebServerStartTest < Pitchfork::Test

def test_preload_app
tmp = Tempfile.new('test_preload_app_config')
Pitchfork::Info.keep_io(tmp)
ObjectSpace.undefine_finalizer(tmp)
app = lambda { ||
tmp.sysseek(0)
Expand Down
6 changes: 3 additions & 3 deletions test/slow/test_signals.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def setup
@bs = 1 * 1024 * 1024
@count = 100
@port = unused_port
@sock = Tempfile.new('pitchfork.sock')
@tmp = Tempfile.new('pitchfork.write')
@sock = Pitchfork::Info.keep_io(Tempfile.new('pitchfork.sock'))
@tmp = Pitchfork::Info.keep_io(Tempfile.new('pitchfork.write'))
@tmp.sync = true
File.unlink(@sock.path)
File.unlink(@tmp.path)
Expand Down Expand Up @@ -75,7 +75,7 @@ def test_worker_dies_on_dead_master
end

def test_sleepy_kill
rd, wr = IO.pipe
rd, wr = Pitchfork::Info.keep_ios(IO.pipe)
pid = fork {
rd.close
app = lambda { |env| wr.syswrite('.'); sleep; [ 200, {}, [] ] }
Expand Down
2 changes: 1 addition & 1 deletion test/slow/test_upload.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def test_put_keepalive_truncates_small_overwrite
end

def test_put_excessive_overwrite_closed
tmp = Tempfile.new('overwrite_check')
tmp = Pitchfork::Info.keep_io(Tempfile.new('overwrite_check'))
tmp.sync = true
start_server(lambda { |env|
nr = 0
Expand Down
6 changes: 3 additions & 3 deletions test/unit/test_ccc.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ def test_ccc_tcpi
host = '127.0.0.1'
port = unused_port

rd, wr = IO.pipe
sleep_pipe = IO.pipe
ready_read, ready_write = IO.pipe
rd, wr = Pitchfork::Info.keep_ios(IO.pipe)
sleep_pipe = Pitchfork::Info.keep_ios(IO.pipe)
ready_read, ready_write = Pitchfork::Info.keep_ios(IO.pipe)

pid = fork do
ready_read.close
Expand Down

0 comments on commit 5f65286

Please sign in to comment.