proxy_pass: fix race condition due to flawed hijack check

The entire idea of a one-shot-based design is all the mutual
exclusion is handled by the event dispatch mechanism (epoll or
kqueue) without burdening the user with extra locking.  However, the
way the hijack works means we check the Rack env for the
'rack.hijack_io' key which is shared across requests and may
be cleared.

Ideally, this would not be a problem if the Rack dispatch allowed
returning a special value (e.g. ":ignore") instead of the normal
status-headers-body array, much like what the non-standard
"async.callback" API Thin started.

We could also avoid this problem by disallowing our "unhijack-ing"
of the socket but at a significant cost of crippling code
reusability, including that of existing middleware.

Thus, we allocate a new, empty request object here to avoid a TOCTTOU
in the following timeline:

original thread:                                 | another thread
HttpClient#yahns_step                            |
r = k.app.call(env = @hs.env)  # socket hijacked into epoll queue
<thread is scheduled away>                       | epoll_wait readiness
                                                 | ReqRes#yahns_step
                                                 | proxy dispatch ...
                                                 | proxy_busy_mod_done
************************** DANGER BELOW ********************************
                                                 | HttpClient#yahns_step
                                                 | # clears env
 # sees empty env:                               |
return :ignore if env.include?('rack.hijack_io') |

In other words, we cannot ever touch the original env seen by the
original thread since it must see the 'rack.hijack_io' value because
both are operating in the same Yahns::HttpClient object.  This will
happen regardless of GVL existence.

Avoiding errors like this is absolutely critical to every one-shot
based design.

1 files changed, 10 insertions, 0 deletions

diff --git a/test/test_proxy_pass.rb b/test/test_proxy_pass.rb
index 840ad1c..a27a45e 100644
--- a/test/test_proxy_pass.rb
+++ b/test/test_proxy_pass.rb
@@ -437,6 +437,16 @@ class TestProxyPass < Testcase
       end
       re = /^Date:[^\r\n]+/
       assert_equal after_up.sub(re, ''), r1.sub(re, '')
+
+      pl.write "GET / HTTP/1.1\r\nHost: example.com"
+      sleep 0.1 # hope epoll wakes up and reads in this time
+      pl.write "\r\n\r\nGET / HTTP/1.1\r\nHost: example.com\r\n\r\n"
+      burst = pl.readpartial(666)
+      until burst.scan(/^hi$/).size == 2
+        burst << pl.readpartial(666)
+      end
+      assert_equal 2, burst.scan(/^hi$/).size
+      assert_match %r{\r\n\r\nhi\n\z}, burst
     end
     r1 = r1.split("\r\n").reject { |x| x =~ /^Date: / }
     r2 = r2.split("\r\n").reject { |x| x =~ /^Date: / }