From c917ac526df214611ec33c21de2b070452ec8434 Mon Sep 17 00:00:00 2001
From: Dirkjan Bussink <d.bussink@gmail.com>
Date: Mon, 8 Mar 2021 09:51:09 +0100
Subject: Allocate a new request for each client

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.
---
 lib/unicorn/http_server.rb |  2 +-
 lib/unicorn/oob_gc.rb      |  6 ++---
 t/hijack.ru                | 55 ----------------------------------------------
 t/t0200-rack-hijack.sh     | 51 ------------------------------------------
 4 files changed, 4 insertions(+), 110 deletions(-)
 delete mode 100644 t/hijack.ru
 delete mode 100755 t/t0200-rack-hijack.sh

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
-- 
cgit v1.2.3-24-ge0c7