* [PATCH] Add worker_exec configuration option @ 2017-03-08 18:44 Jeremy Evans 2017-03-08 20:02 ` Eric Wong 0 siblings, 1 reply; 10+ messages in thread From: Jeremy Evans @ 2017-03-08 18:44 UTC (permalink / raw) To: unicorn-public The worker_exec configuration option makes all worker processes exec after forking. This initializes the worker processes with separate memory layouts, defeating address space discovery attacks on operating systems supporting address space layout randomization, such as Linux, MacOS X, NetBSD, OpenBSD, and Solaris. Support for execing workers is very similar to support for reexecing the master process. The main difference is the worker's to_i and master pipes also need to be inherited after worker exec just as the listening sockets need to be inherited after reexec. --- lib/unicorn/configurator.rb | 10 ++++++++ lib/unicorn/http_server.rb | 56 ++++++++++++++++++++++++++++++++++++++++++--- lib/unicorn/worker.rb | 5 ++-- 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb index 7ed5ffa..f69f220 100644 --- a/lib/unicorn/configurator.rb +++ b/lib/unicorn/configurator.rb @@ -53,6 +53,7 @@ class Unicorn::Configurator server.logger.info("worker=#{worker.nr} ready") }, :pid => nil, + :worker_exec => false, :preload_app => false, :check_client_connection => false, :rewindable_input => true, # for Rack 2.x: (Rack::VERSION[0] <= 1), @@ -239,6 +240,15 @@ def timeout(seconds) set[:timeout] = seconds > max ? max : seconds end + # Whether to exec in each worker process after forking. This changes the + # memory layout of each worker process, which is a security feature designed + # to defeat possible address space discovery attacks. Note that using + # worker_exec only makes sense if you are not preloading the application, + # and will result in higher memory usage. + def worker_exec(bool) + set_bool(:worker_exec, bool) + end + # sets the current number of worker_processes to +nr+. Each worker # process will serve exactly one client at a time. You can # increment or decrement this value at runtime by sending SIGTTIN diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb index ef897ad..d68d7d8 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -15,7 +15,7 @@ class Unicorn::HttpServer :before_fork, :after_fork, :before_exec, :listener_opts, :preload_app, :orig_app, :config, :ready_pipe, :user - attr_writer :after_worker_exit, :after_worker_ready + attr_writer :after_worker_exit, :after_worker_ready, :worker_exec attr_reader :pid, :logger include Unicorn::SocketHelper @@ -105,6 +105,14 @@ def initialize(app, options = {}) # list of signals we care about and trap in master. @queue_sigs = [ :WINCH, :QUIT, :INT, :TERM, :USR1, :USR2, :HUP, :TTIN, :TTOU ] + + if worker_nr = ENV['UNICORN_WORKER_NR'] + @worker_nr = worker_nr.to_i + @master_pid = Process.ppid + else + @master_pid = $$ + end + end # Runs the thing. Returns self so you can run join on it @@ -113,7 +121,6 @@ def start # this pipe is used to wake us up from select(2) in #join when signals # are trapped. See trap_deferred. @self_pipe.replace(Unicorn.pipe) - @master_pid = $$ # setup signal handlers before writing pid file in case people get # trigger happy and send signals as soon as the pid file exists. @@ -459,6 +466,39 @@ def reexec proc_name 'master (old)' end + def worker_exec(worker) + listener_fds = {} + LISTENERS.each do |sock| + sock.close_on_exec = false + listener_fds[sock.fileno] = sock + end + ENV['UNICORN_FD'] = listener_fds.keys.join(',') + ENV['UNICORN_WORKER_NR'] = worker.nr.to_s + ENV['UNICORN_WORKER_TO_IO'] = worker.to_io.fileno.to_s + ENV['UNICORN_WORKER_MASTER'] = worker.master.fileno.to_s + Dir.chdir(START_CTX[:cwd]) + cmd = [ START_CTX[0] ].concat(START_CTX[:argv]) + + listener_fds[worker.to_io.fileno] = worker.to_io + listener_fds[worker.master.fileno] = worker.master + + # avoid leaking FDs we don't know about, but let before_exec + # unset FD_CLOEXEC, if anything else in the app eventually + # relies on FD inheritence. + (3..1024).each do |io| + next if listener_fds.include?(io) + io = IO.for_fd(io) rescue next + io.autoclose = false + io.close_on_exec = true + end + + # exec(command, hash) works in at least 1.9.1+, but will only be + # required in 1.9.4/2.0.0 at earliest. + cmd << listener_fds + logger.info "executing worker #{cmd.inspect} (in #{Dir.pwd})" + exec(*cmd) + end + # forcibly terminate all workers that haven't checked in in timeout seconds. The timeout is implemented using an unlinked File def murder_lazy_workers next_sleep = @timeout - 1 @@ -495,6 +535,12 @@ def after_fork_internal end def spawn_missing_workers + if @worker_nr + worker = Unicorn::Worker.new(@worker_nr, [ENV["UNICORN_WORKER_TO_IO"], ENV["UNICORN_WORKER_MASTER"]].map{|i| Kgio::Pipe.for_fd(i.to_i)}) + worker_loop(worker) + exit + end + worker_nr = -1 until (worker_nr += 1) == @worker_processes @workers.value?(worker_nr) and next @@ -505,7 +551,11 @@ def spawn_missing_workers worker.atfork_parent else after_fork_internal - worker_loop(worker) + if @worker_exec + worker_exec(worker) + else + worker_loop(worker) + end exit end end diff --git a/lib/unicorn/worker.rb b/lib/unicorn/worker.rb index e22c1bf..8bbac5e 100644 --- a/lib/unicorn/worker.rb +++ b/lib/unicorn/worker.rb @@ -12,18 +12,19 @@ class Unicorn::Worker # :stopdoc: attr_accessor :nr, :switched attr_reader :to_io # IO.select-compatible + attr_reader :master PER_DROP = Raindrops::PAGE_SIZE / Raindrops::SIZE DROPS = [] - def initialize(nr) + def initialize(nr, pipe=nil) drop_index = nr / PER_DROP @raindrop = DROPS[drop_index] ||= Raindrops.new(PER_DROP) @offset = nr % PER_DROP @raindrop[@offset] = 0 @nr = nr @switched = false - @to_io, @master = Unicorn.pipe + @to_io, @master = pipe || Unicorn.pipe end def atfork_child # :nodoc: -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option 2017-03-08 18:44 [PATCH] Add worker_exec configuration option Jeremy Evans @ 2017-03-08 20:02 ` Eric Wong 2017-03-09 4:52 ` Jeremy Evans 2017-03-09 19:41 ` [PATCH] Add worker_exec configuration option V2 Jeremy Evans 0 siblings, 2 replies; 10+ messages in thread From: Eric Wong @ 2017-03-08 20:02 UTC (permalink / raw) To: Jeremy Evans; +Cc: unicorn-public Jeremy Evans <code@jeremyevans.net> wrote: > The worker_exec configuration option makes all worker processes > exec after forking. This initializes the worker processes with > separate memory layouts, defeating address space discovery > attacks on operating systems supporting address space layout > randomization, such as Linux, MacOS X, NetBSD, OpenBSD, and > Solaris. > > Support for execing workers is very similar to support for reexecing > the master process. The main difference is the worker's to_i and > master pipes also need to be inherited after worker exec just as the > listening sockets need to be inherited after reexec. Thanks, this seems like an acceptable feature. Some comments below.... > +++ b/lib/unicorn/http_server.rb > @@ -105,6 +105,14 @@ def initialize(app, options = {}) > # list of signals we care about and trap in master. > @queue_sigs = [ > :WINCH, :QUIT, :INT, :TERM, :USR1, :USR2, :HUP, :TTIN, :TTOU ] > + > + if worker_nr = ENV['UNICORN_WORKER_NR'] > + @worker_nr = worker_nr.to_i > + @master_pid = Process.ppid > + else > + @master_pid = $$ > + end > + > end We should nil @worker_nr in the normal case to avoid introducing new warnings on uninitialized ivars. > # Runs the thing. Returns self so you can run join on it > @@ -113,7 +121,6 @@ def start > # this pipe is used to wake us up from select(2) in #join when signals > # are trapped. See trap_deferred. > @self_pipe.replace(Unicorn.pipe) > - @master_pid = $$ Any particular reason setting @master_pid was moved to initialize? Maybe we (or anybody else) has tests or other code which do: foo = Unicorn::HttpServer.new ... pid = fork do foo.start.join end Which would be broken by the movement from start => initialize Again, I don't consider this a stable API and don't want people depending on our internals; but I will also avoid breaking existing behavior whenever possible. > @@ -459,6 +466,39 @@ def reexec > proc_name 'master (old)' > end > > + def worker_exec(worker) > + listener_fds = {} > + LISTENERS.each do |sock| > + sock.close_on_exec = false > + listener_fds[sock.fileno] = sock > + end > + ENV['UNICORN_FD'] = listener_fds.keys.join(',') > + ENV['UNICORN_WORKER_NR'] = worker.nr.to_s > + ENV['UNICORN_WORKER_TO_IO'] = worker.to_io.fileno.to_s > + ENV['UNICORN_WORKER_MASTER'] = worker.master.fileno.to_s We can cram these two FDs into one UNICORN_WORKER_FD comma-delimited variable like UNICORN_FD. The underlying getenv/setenv/putenv functions are all O(n), so keeping env smaller would be nice. And we can even avoid modifying ENV on Ruby 1.9+ by using exec(env, cmd, options...) or even Process.spawn (see below) > + Dir.chdir(START_CTX[:cwd]) > + cmd = [ START_CTX[0] ].concat(START_CTX[:argv]) > + > + listener_fds[worker.to_io.fileno] = worker.to_io > + listener_fds[worker.master.fileno] = worker.master > + > + # avoid leaking FDs we don't know about, but let before_exec > + # unset FD_CLOEXEC, if anything else in the app eventually > + # relies on FD inheritence. Comment seems blindly copy+pasted, since before_exec does not seem to exist, for this... > + (3..1024).each do |io| > + next if listener_fds.include?(io) > + io = IO.for_fd(io) rescue next > + io.autoclose = false > + io.close_on_exec = true > + end I suppose we should probably use Process.getrlimit, here, maybe skip the loop entirely for Ruby 2.0.0+ (which made close-on-exec the default). This will be a separate patch... Since there's no before_exec hook as with SIGUSR2, I think we can do all of this setup in master and use Process.spawn instead of fork+exec explicitly. Process.spawn transparently allows the use of vfork+exec to avoid fork overhead on some systems. > + # exec(command, hash) works in at least 1.9.1+, but will only be > + # required in 1.9.4/2.0.0 at earliest. I guess it's time to update that comment, since 1.9.4 never happened :) > + cmd << listener_fds > + logger.info "executing worker #{cmd.inspect} (in #{Dir.pwd})" > + exec(*cmd) > + end > + > # forcibly terminate all workers that haven't checked in in timeout seconds. The timeout is implemented using an unlinked File > def murder_lazy_workers > next_sleep = @timeout - 1 > @@ -495,6 +535,12 @@ def after_fork_internal > end > > def spawn_missing_workers > + if @worker_nr > + worker = Unicorn::Worker.new(@worker_nr, [ENV["UNICORN_WORKER_TO_IO"], ENV["UNICORN_WORKER_MASTER"]].map{|i| Kgio::Pipe.for_fd(i.to_i)}) Please keep new code wrapped to 80 chars or less. I'm still using hardware that predates this project and my eyes will only get worse with age. > @@ -505,7 +551,11 @@ def spawn_missing_workers > worker.atfork_parent > else > after_fork_internal > - worker_loop(worker) > + if @worker_exec > + worker_exec(worker) > + else > + worker_loop(worker) > + end I prefer the ternary operator for simple cases like this. It is more compact in terms of screen space and VM bytecode. But this may not be changed at all if we switch to using Process.spawn, instead. The rest looks fine. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option 2017-03-08 20:02 ` Eric Wong @ 2017-03-09 4:52 ` Jeremy Evans 2017-03-09 13:57 ` Eric Wong 2017-03-09 19:41 ` [PATCH] Add worker_exec configuration option V2 Jeremy Evans 1 sibling, 1 reply; 10+ messages in thread From: Jeremy Evans @ 2017-03-09 4:52 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public On 03/08 08:02, Eric Wong wrote: > Jeremy Evans <code@jeremyevans.net> wrote: > > The worker_exec configuration option makes all worker processes > > exec after forking. This initializes the worker processes with > > separate memory layouts, defeating address space discovery > > attacks on operating systems supporting address space layout > > randomization, such as Linux, MacOS X, NetBSD, OpenBSD, and > > Solaris. > > > > Support for execing workers is very similar to support for reexecing > > the master process. The main difference is the worker's to_i and > > master pipes also need to be inherited after worker exec just as the > > listening sockets need to be inherited after reexec. > > Thanks, this seems like an acceptable feature. <snip> Eric, Thanks for your detailed review. I will work on an updated patch and try to send it tomorrow or Friday. Thanks, Jeremy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option 2017-03-09 4:52 ` Jeremy Evans @ 2017-03-09 13:57 ` Eric Wong 0 siblings, 0 replies; 10+ messages in thread From: Eric Wong @ 2017-03-09 13:57 UTC (permalink / raw) To: Jeremy Evans; +Cc: unicorn-public Jeremy Evans <code@jeremyevans.net> wrote: > Thanks for your detailed review. I will work on an updated patch and > try to send it tomorrow or Friday. No problem and no rush; I might be out a bit the next few days. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option V2 2017-03-08 20:02 ` Eric Wong 2017-03-09 4:52 ` Jeremy Evans @ 2017-03-09 19:41 ` Jeremy Evans 2017-03-10 21:19 ` Eric Wong 1 sibling, 1 reply; 10+ messages in thread From: Jeremy Evans @ 2017-03-09 19:41 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public On 03/08 08:02, Eric Wong wrote: > Jeremy Evans <code@jeremyevans.net> wrote: > > The worker_exec configuration option makes all worker processes > > exec after forking. This initializes the worker processes with > > separate memory layouts, defeating address space discovery > > attacks on operating systems supporting address space layout > > randomization, such as Linux, MacOS X, NetBSD, OpenBSD, and > > Solaris. > > > > Support for execing workers is very similar to support for reexecing > > the master process. The main difference is the worker's to_i and > > master pipes also need to be inherited after worker exec just as the > > listening sockets need to be inherited after reexec. > > Thanks, this seems like an acceptable feature. Eric, Here's V2 of the patch, which I think should address all of the issues you pointed out. Thanks, Jeremy From 8e68bf8c6a8b91704f31dd9b9a62e6f2e330e380 Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@jeremyevans.net> Date: Wed, 8 Mar 2017 10:19:02 -0800 Subject: [PATCH] Add worker_exec configuration option The worker_exec configuration option makes all worker processes exec after forking. This initializes the worker processes with separate memory layouts, defeating address space discovery attacks on operating systems supporting address space layout randomization, such as Linux, MacOS X, NetBSD, OpenBSD, and Solaris. Support for execing workers is very similar to support for reexecing the master process. The main difference is the worker's to_i and master pipes also need to be inherited after worker exec just as the listening sockets need to be inherited after reexec. Because execing working is similar to reexecing the master, this extracts a couple of methods from reexec (listener_sockets and close_sockets_on_exec), so they can be reused in worker_spawn. --- lib/unicorn/configurator.rb | 10 ++++++ lib/unicorn/http_server.rb | 83 ++++++++++++++++++++++++++++++++++----------- lib/unicorn/worker.rb | 5 +-- 3 files changed, 77 insertions(+), 21 deletions(-) diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb index 7ed5ffa..f69f220 100644 --- a/lib/unicorn/configurator.rb +++ b/lib/unicorn/configurator.rb @@ -53,6 +53,7 @@ class Unicorn::Configurator server.logger.info("worker=#{worker.nr} ready") }, :pid => nil, + :worker_exec => false, :preload_app => false, :check_client_connection => false, :rewindable_input => true, # for Rack 2.x: (Rack::VERSION[0] <= 1), @@ -239,6 +240,15 @@ def timeout(seconds) set[:timeout] = seconds > max ? max : seconds end + # Whether to exec in each worker process after forking. This changes the + # memory layout of each worker process, which is a security feature designed + # to defeat possible address space discovery attacks. Note that using + # worker_exec only makes sense if you are not preloading the application, + # and will result in higher memory usage. + def worker_exec(bool) + set_bool(:worker_exec, bool) + end + # sets the current number of worker_processes to +nr+. Each worker # process will serve exactly one client at a time. You can # increment or decrement this value at runtime by sending SIGTTIN diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb index ef897ad..a5bd2c4 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -15,7 +15,7 @@ class Unicorn::HttpServer :before_fork, :after_fork, :before_exec, :listener_opts, :preload_app, :orig_app, :config, :ready_pipe, :user - attr_writer :after_worker_exit, :after_worker_ready + attr_writer :after_worker_exit, :after_worker_ready, :worker_exec attr_reader :pid, :logger include Unicorn::SocketHelper @@ -105,6 +105,14 @@ def initialize(app, options = {}) # list of signals we care about and trap in master. @queue_sigs = [ :WINCH, :QUIT, :INT, :TERM, :USR1, :USR2, :HUP, :TTIN, :TTOU ] + + @worker_data = if worker_data = ENV['UNICORN_WORKER'] + worker_data = worker_data.split(',').map!(&:to_i) + worker_data[1] = worker_data.slice!(1..2).map do |i| + Kgio::Pipe.for_fd(i) + end + worker_data + end end # Runs the thing. Returns self so you can run join on it @@ -113,7 +121,7 @@ def start # this pipe is used to wake us up from select(2) in #join when signals # are trapped. See trap_deferred. @self_pipe.replace(Unicorn.pipe) - @master_pid = $$ + @master_pid = @worker_data ? Process.ppid : $$ # setup signal handlers before writing pid file in case people get # trigger happy and send signals as soon as the pid file exists. @@ -430,11 +438,7 @@ def reexec end @reexec_pid = fork do - listener_fds = {} - LISTENERS.each do |sock| - sock.close_on_exec = false - listener_fds[sock.fileno] = sock - end + listener_fds = listener_sockets ENV['UNICORN_FD'] = listener_fds.keys.join(',') Dir.chdir(START_CTX[:cwd]) cmd = [ START_CTX[0] ].concat(START_CTX[:argv]) @@ -442,12 +446,7 @@ def reexec # avoid leaking FDs we don't know about, but let before_exec # unset FD_CLOEXEC, if anything else in the app eventually # relies on FD inheritence. - (3..1024).each do |io| - next if listener_fds.include?(io) - io = IO.for_fd(io) rescue next - io.autoclose = false - io.close_on_exec = true - end + close_sockets_on_exec(listener_fds) # exec(command, hash) works in at least 1.9.1+, but will only be # required in 1.9.4/2.0.0 at earliest. @@ -459,6 +458,40 @@ def reexec proc_name 'master (old)' end + def worker_spawn(worker) + listener_fds = listener_sockets + env = {} + env['UNICORN_FD'] = listener_fds.keys.join(',') + + listener_fds[worker.to_io.fileno] = worker.to_io + listener_fds[worker.master.fileno] = worker.master + + worker_info = [worker.nr, worker.to_io.fileno, worker.master.fileno] + env['UNICORN_WORKER'] = worker_info.join(',') + + close_sockets_on_exec(listener_fds) + + Process.spawn(env, START_CTX[0], *START_CTX[:argv], listener_fds) + end + + def listener_sockets + listener_fds = {} + LISTENERS.each do |sock| + sock.close_on_exec = false + listener_fds[sock.fileno] = sock + end + listener_fds + end + + def close_sockets_on_exec(sockets) + (3..1024).each do |io| + next if sockets.include?(io) + io = IO.for_fd(io) rescue next + io.autoclose = false + io.close_on_exec = true + end + end + # forcibly terminate all workers that haven't checked in in timeout seconds. The timeout is implemented using an unlinked File def murder_lazy_workers next_sleep = @timeout - 1 @@ -495,19 +528,31 @@ def after_fork_internal end def spawn_missing_workers + if @worker_data + worker = Unicorn::Worker.new(*@worker_data) + after_fork_internal + worker_loop(worker) + exit + end + worker_nr = -1 until (worker_nr += 1) == @worker_processes @workers.value?(worker_nr) and next worker = Unicorn::Worker.new(worker_nr) before_fork.call(self, worker) - if pid = fork - @workers[pid] = worker - worker.atfork_parent + + pid = if @worker_exec + worker_spawn(worker) else - after_fork_internal - worker_loop(worker) - exit + fork do + after_fork_internal + worker_loop(worker) + exit + end end + + @workers[pid] = worker + worker.atfork_parent end rescue => e @logger.error(e) rescue nil diff --git a/lib/unicorn/worker.rb b/lib/unicorn/worker.rb index e22c1bf..8bbac5e 100644 --- a/lib/unicorn/worker.rb +++ b/lib/unicorn/worker.rb @@ -12,18 +12,19 @@ class Unicorn::Worker # :stopdoc: attr_accessor :nr, :switched attr_reader :to_io # IO.select-compatible + attr_reader :master PER_DROP = Raindrops::PAGE_SIZE / Raindrops::SIZE DROPS = [] - def initialize(nr) + def initialize(nr, pipe=nil) drop_index = nr / PER_DROP @raindrop = DROPS[drop_index] ||= Raindrops.new(PER_DROP) @offset = nr % PER_DROP @raindrop[@offset] = 0 @nr = nr @switched = false - @to_io, @master = Unicorn.pipe + @to_io, @master = pipe || Unicorn.pipe end def atfork_child # :nodoc: -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option V2 2017-03-09 19:41 ` [PATCH] Add worker_exec configuration option V2 Jeremy Evans @ 2017-03-10 21:19 ` Eric Wong 2017-03-11 5:26 ` Jeremy Evans 0 siblings, 1 reply; 10+ messages in thread From: Eric Wong @ 2017-03-10 21:19 UTC (permalink / raw) To: Jeremy Evans; +Cc: unicorn-public Jeremy Evans <code@jeremyevans.net> wrote: > Here's V2 of the patch, which I think should address all of the issues > you pointed out. Thanks. Pushed to git://80x24.org/unicorn worker_exec I can add tests, later, or you can. I also had some FreeBSD test fixes (which might apply to OpenBSD) on a VM somewhere which I'll Cc: you on: there was also just SO_KEEPALIVE fix I posted: https://bogomips.org/unicorn-public/20170310203431.28067-1-e@80x24.org/raw > worker_nr = -1 > until (worker_nr += 1) == @worker_processes > @workers.value?(worker_nr) and next > worker = Unicorn::Worker.new(worker_nr) > before_fork.call(self, worker) > - if pid = fork > - @workers[pid] = worker > - worker.atfork_parent > + > + pid = if @worker_exec > + worker_spawn(worker) > else > - after_fork_internal > - worker_loop(worker) > - exit > + fork do > + after_fork_internal > + worker_loop(worker) > + exit > + end I prefer to avoid the block with fork. The block deepens the stack for the running app, so it can affect GC efficiency. Can be fixed in a separate patch... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option V2 2017-03-10 21:19 ` Eric Wong @ 2017-03-11 5:26 ` Jeremy Evans 2017-03-11 7:18 ` Eric Wong 0 siblings, 1 reply; 10+ messages in thread From: Jeremy Evans @ 2017-03-11 5:26 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public On 03/10 09:19, Eric Wong wrote: > Jeremy Evans <code@jeremyevans.net> wrote: > > Here's V2 of the patch, which I think should address all of the issues > > you pointed out. > > Thanks. Pushed to git://80x24.org/unicorn worker_exec I can add > tests, later, or you can. I also had some FreeBSD test fixes > (which might apply to OpenBSD) on a VM somewhere which I'll Cc: > you on: there was also just SO_KEEPALIVE fix I posted: > > https://bogomips.org/unicorn-public/20170310203431.28067-1-e@80x24.org/raw The C test code also returns 8 on OpenBSD, FWIW. I'm happy to test any test fixes on OpenBSD, just let me know. > > > worker_nr = -1 > > until (worker_nr += 1) == @worker_processes > > @workers.value?(worker_nr) and next > > worker = Unicorn::Worker.new(worker_nr) > > before_fork.call(self, worker) > > - if pid = fork > > - @workers[pid] = worker > > - worker.atfork_parent > > + > > + pid = if @worker_exec > > + worker_spawn(worker) > > else > > - after_fork_internal > > - worker_loop(worker) > > - exit > > + fork do > > + after_fork_internal > > + worker_loop(worker) > > + exit > > + end > > I prefer to avoid the block with fork. The block deepens the > stack for the running app, so it can affect GC efficiency. > > Can be fixed in a separate patch... That makes sense. If you would like me to send a separate patch to fix it, I can do that. Thanks, Jeremy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option V2 2017-03-11 5:26 ` Jeremy Evans @ 2017-03-11 7:18 ` Eric Wong 2017-03-13 15:32 ` Jeremy Evans 0 siblings, 1 reply; 10+ messages in thread From: Eric Wong @ 2017-03-11 7:18 UTC (permalink / raw) To: Jeremy Evans; +Cc: unicorn-public Jeremy Evans <code@jeremyevans.net> wrote: > On 03/10 09:19, Eric Wong wrote: > > tests, later, or you can. I also had some FreeBSD test fixes > > (which might apply to OpenBSD) on a VM somewhere which I'll Cc: > > you on: there was also just SO_KEEPALIVE fix I posted: > > > > https://bogomips.org/unicorn-public/20170310203431.28067-1-e@80x24.org/raw > > The C test code also returns 8 on OpenBSD, FWIW. I'm happy to test any > test fixes on OpenBSD, just let me know. Thanks. I'll be happy to help fix any OpenBSD failures you see from "gmake check" I don't think the accept_filter fixes apply to OpenBSD, but I guess the expr(1) fix did. Thank you. Same goes for NetBSD, DragonflyBSD or any other completely Free/Open Source OSes anybody else here uses. > > Jeremy Evans <code@jeremyevans.net> wrote: > > > - if pid = fork > > > - @workers[pid] = worker > > > - worker.atfork_parent > > > + > > > + pid = if @worker_exec > > > + worker_spawn(worker) > > > else > > > - after_fork_internal > > > - worker_loop(worker) > > > - exit > > > + fork do > > > + after_fork_internal > > > + worker_loop(worker) > > > + exit > > > + end > > > > I prefer to avoid the block with fork. The block deepens the > > stack for the running app, so it can affect GC efficiency. > > > > Can be fixed in a separate patch... > > That makes sense. If you would like me to send a separate patch to fix > it, I can do that. Yes, please. Not sure if we should automate the stack depth test... I resisted it in the past since it might be too fragile w.r.t changes to Ruby (even using "unicorn" via the RubyGems-generated wrapper deepens it by 2). Thanks again. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option V2 2017-03-11 7:18 ` Eric Wong @ 2017-03-13 15:32 ` Jeremy Evans 2017-03-13 19:18 ` Eric Wong 0 siblings, 1 reply; 10+ messages in thread From: Jeremy Evans @ 2017-03-13 15:32 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public On 03/11 07:18, Eric Wong wrote: > Jeremy Evans <code@jeremyevans.net> wrote: > > On 03/10 09:19, Eric Wong wrote: > > > Jeremy Evans <code@jeremyevans.net> wrote: > > > > - if pid = fork > > > > - @workers[pid] = worker > > > > - worker.atfork_parent > > > > + > > > > + pid = if @worker_exec > > > > + worker_spawn(worker) > > > > else > > > > - after_fork_internal > > > > - worker_loop(worker) > > > > - exit > > > > + fork do > > > > + after_fork_internal > > > > + worker_loop(worker) > > > > + exit > > > > + end > > > > > > I prefer to avoid the block with fork. The block deepens the > > > stack for the running app, so it can affect GC efficiency. > > > > > > Can be fixed in a separate patch... > > > > That makes sense. If you would like me to send a separate patch to fix > > it, I can do that. > > Yes, please. > > Not sure if we should automate the stack depth test... > I resisted it in the past since it might be too fragile > w.r.t changes to Ruby (even using "unicorn" via the > RubyGems-generated wrapper deepens it by 2). Here's a patch to fix the stack depth issue: From aa4914846a0870e5b01530a47d6dbfe09aeb39ce Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@jeremyevans.net> Date: Mon, 13 Mar 2017 08:28:54 -0700 Subject: [PATCH] Don't pass a block for fork when forking workers This reduces the stack depth, making GC more efficient. --- lib/unicorn/http_server.rb | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb index a5bd2c4..023df10 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -541,14 +541,12 @@ def spawn_missing_workers worker = Unicorn::Worker.new(worker_nr) before_fork.call(self, worker) - pid = if @worker_exec - worker_spawn(worker) - else - fork do - after_fork_internal - worker_loop(worker) - exit - end + pid = @worker_exec ? worker_spawn(worker) : fork + + unless pid + after_fork_internal + worker_loop(worker) + exit end @workers[pid] = worker -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Add worker_exec configuration option V2 2017-03-13 15:32 ` Jeremy Evans @ 2017-03-13 19:18 ` Eric Wong 0 siblings, 0 replies; 10+ messages in thread From: Eric Wong @ 2017-03-13 19:18 UTC (permalink / raw) To: Jeremy Evans; +Cc: unicorn-public Jeremy Evans <code@jeremyevans.net> wrote: > Here's a patch to fix the stack depth issue: Thanks, applied with some trailing whitespace fixups. (I suggest enabling the pre-commit hook distributed with git in $GIT_DIR/hooks/pre-commit, it'll run "git diff-index --check ..." to check that) > From aa4914846a0870e5b01530a47d6dbfe09aeb39ce Mon Sep 17 00:00:00 2001 Btw, you can omit the above line and replace it with "---8<---" (no quotes) so the applier can use "git am --scissors". Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-13 19:18 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-08 18:44 [PATCH] Add worker_exec configuration option Jeremy Evans 2017-03-08 20:02 ` Eric Wong 2017-03-09 4:52 ` Jeremy Evans 2017-03-09 13:57 ` Eric Wong 2017-03-09 19:41 ` [PATCH] Add worker_exec configuration option V2 Jeremy Evans 2017-03-10 21:19 ` Eric Wong 2017-03-11 5:26 ` Jeremy Evans 2017-03-11 7:18 ` Eric Wong 2017-03-13 15:32 ` Jeremy Evans 2017-03-13 19:18 ` Eric Wong
Code repositories for project(s) associated with this public inbox https://yhbt.net/unicorn.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).