about summary refs log tree commit homepage
diff options
context:
space:
mode:
authorDirkjan Bussink <d.bussink@gmail.com>2021-03-08 09:51:09 +0100
committerEric Wong <bofh@yhbt.net>2021-03-13 02:19:04 +0000
commitc917ac526df214611ec33c21de2b070452ec8434 (patch)
tree627b9c3050e2533bde08766473ed045f49c967b7
parent5cdb68eb26faf7fd75fbf3ab1fadcf2a30fd4974 (diff)
downloadunicorn-c917ac526df214611ec33c21de2b070452ec8434.tar.gz
This removes the reuse of the parser between requests. Reusing these is
risky in the context of running any other threads within the unicorn
process, also for threads that run background tasks.

If any other thread accidentally grabs hold of the request it can modify
things for the next request in flight.

The downside here is that we allocate more for each request, but that is
worth the trade off here and the security risk we otherwise would carry
to leaking wrong and incorrect data.
-rw-r--r--lib/unicorn/http_server.rb2
-rw-r--r--lib/unicorn/oob_gc.rb6
-rw-r--r--t/hijack.ru55
-rwxr-xr-xt/t0200-rack-hijack.sh51
4 files changed, 4 insertions, 110 deletions
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index c0f14ba..22f067f 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -69,7 +69,6 @@ class Unicorn::HttpServer
   # incoming requests on the socket.
   def initialize(app, options = {})
     @app = app
-    @request = Unicorn::HttpRequest.new
     @reexec_pid = 0
     @default_middleware = true
     options = options.dup
@@ -621,6 +620,7 @@ class Unicorn::HttpServer
   # once a client is accepted, it is processed in its entirety here
   # in 3 easy steps: read request, call app, write app response
   def process_client(client)
+    @request = Unicorn::HttpRequest.new
     env = @request.read(client)
 
     if early_hints
diff --git a/lib/unicorn/oob_gc.rb b/lib/unicorn/oob_gc.rb
index 3b2f488..91a8e51 100644
--- a/lib/unicorn/oob_gc.rb
+++ b/lib/unicorn/oob_gc.rb
@@ -60,7 +60,6 @@ module Unicorn::OobGC
     self.const_set :OOBGC_INTERVAL, interval
     ObjectSpace.each_object(Unicorn::HttpServer) do |s|
       s.extend(self)
-      self.const_set :OOBGC_ENV, s.instance_variable_get(:@request).env
     end
     app # pretend to be Rack middleware since it was in the past
   end
@@ -68,9 +67,10 @@ module Unicorn::OobGC
   #:stopdoc:
   def process_client(client)
     super(client) # Unicorn::HttpServer#process_client
-    if OOBGC_PATH =~ OOBGC_ENV['PATH_INFO'] && ((@@nr -= 1) <= 0)
+    env = instance_variable_get(:@request).env
+    if OOBGC_PATH =~ env['PATH_INFO'] && ((@@nr -= 1) <= 0)
       @@nr = OOBGC_INTERVAL
-      OOBGC_ENV.clear
+      env.clear
       disabled = GC.enable
       GC.start
       GC.disable if disabled
diff --git a/t/hijack.ru b/t/hijack.ru
deleted file mode 100644
index 02260e2..0000000
--- a/t/hijack.ru
+++ /dev/null
@@ -1,55 +0,0 @@
-use Rack::Lint
-use Rack::ContentLength
-use Rack::ContentType, "text/plain"
-class DieIfUsed
-  @@n = 0
-  def each
-    abort "body.each called after response hijack\n"
-  end
-
-  def close
-    warn "closed DieIfUsed #{@@n += 1}\n"
-  end
-end
-
-envs = []
-
-run lambda { |env|
-  case env["PATH_INFO"]
-  when "/hijack_req"
-    if env["rack.hijack?"]
-      io = env["rack.hijack"].call
-      envs << env
-      if io.respond_to?(:read_nonblock) &&
-         env["rack.hijack_io"].respond_to?(:read_nonblock)
-
-        # exercise both, since we Rack::Lint may use different objects
-        env["rack.hijack_io"].write("HTTP/1.0 200 OK\r\n\r\n")
-        io.write("request.hijacked")
-        io.close
-        return [ 500, {}, DieIfUsed.new ]
-      end
-    end
-    [ 500, {}, [ "hijack BAD\n" ] ]
-  when "/hijack_res"
-    r = "response.hijacked"
-    [ 200,
-      {
-        "Content-Length" => r.bytesize.to_s,
-        "rack.hijack" => proc do |io|
-          envs << env
-          io.write(r)
-          io.close
-        end
-      },
-      DieIfUsed.new
-    ]
-  when "/normal_env_id"
-    b = "#{env.object_id}\n"
-    h = {
-      'Content-Type' => 'text/plain',
-      'Content-Length' => b.bytesize.to_s,
-    }
-    [ 200, h, [ b ] ]
-  end
-}
diff --git a/t/t0200-rack-hijack.sh b/t/t0200-rack-hijack.sh
deleted file mode 100755
index fee0791..0000000
--- a/t/t0200-rack-hijack.sh
+++ /dev/null
@@ -1,51 +0,0 @@
-#!/bin/sh
-. ./test-lib.sh
-t_plan 9 "rack.hijack tests (Rack 1.5+ (Rack::VERSION >= [ 1,2]))"
-
-t_begin "setup and start" && {
-        unicorn_setup
-        unicorn -D -c $unicorn_config hijack.ru
-        unicorn_wait_start
-}
-
-t_begin "normal env reused between requests" && {
-        env_a="$(curl -sSf http://$listen/normal_env_id)"
-        b="$(curl -sSf http://$listen/normal_env_id)"
-        test x"$env_a" = x"$b"
-}
-
-t_begin "check request hijack" && {
-        test "xrequest.hijacked" = x"$(curl -sSfv http://$listen/hijack_req)"
-}
-
-t_begin "env changed after request hijack" && {
-        env_b="$(curl -sSf http://$listen/normal_env_id)"
-        test x"$env_a" != x"$env_b"
-}
-
-t_begin "check response hijack" && {
-        test "xresponse.hijacked" = x"$(curl -sSfv http://$listen/hijack_res)"
-}
-
-t_begin "env changed after response hijack" && {
-        env_c="$(curl -sSf http://$listen/normal_env_id)"
-        test x"$env_b" != x"$env_c"
-}
-
-t_begin "env continues to be reused between requests" && {
-        b="$(curl -sSf http://$listen/normal_env_id)"
-        test x"$env_c" = x"$b"
-}
-
-t_begin "killing succeeds after hijack" && {
-        kill $unicorn_pid
-}
-
-t_begin "check stderr for hijacked body close" && {
-        check_stderr
-        grep 'closed DieIfUsed 1\>' $r_err
-        grep 'closed DieIfUsed 2\>' $r_err
-        ! grep 'closed DieIfUsed 3\>' $r_err
-}
-
-t_done