yahns Ruby server user/dev discussion
 help / color / mirror / code / Atom feed
From: Eric Wong <e@80x24.org>
To: yahns-public@yhbt.net
Subject: [PATCH] proxy_pass: fix race condition due to flawed hijack check
Date: Tue, 21 Apr 2015 08:49:05 +0000	[thread overview]
Message-ID: <1429606145-14901-1-git-send-email-e@80x24.org> (raw)

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.
---
 lib/yahns/proxy_http_response.rb | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index af8d8cc..6d2457b 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -227,11 +227,37 @@ module Yahns::HttpResponse # :nodoc:
     proxy_busy_mod_done(wbuf.wbuf_persist) # returns nil
   end
 
+  def proxy_wait_next(qflags)
+    # We must 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 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
+    hs = Unicorn::HttpRequest.new
+    hs.buf.replace(@hs.buf)
+    @hs = hs
+    Thread.current[:yahns_queue].queue_mod(self, qflags)
+  end
+
   def proxy_busy_mod_done(alive)
-    q = Thread.current[:yahns_queue]
     case http_response_done(alive)
-    when :wait_readable then q.queue_mod(self, Yahns::Queue::QEV_RD)
-    when :wait_writable then q.queue_mod(self, Yahns::Queue::QEV_WR)
+    when :wait_readable then proxy_wait_next(Yahns::Queue::QEV_RD)
+    when :wait_writable then proxy_wait_next(Yahns::Queue::QEV_WR)
     when :close then Thread.current[:yahns_fdmap].sync_close(self)
     end
 
-- 
EW


             reply	other threads:[~2015-04-21  8:49 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-21  8:49 Eric Wong [this message]
2015-04-21  9:14 ` [PATCH] proxy_pass: fix race condition due to flawed hijack check Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://yhbt.net/yahns/README

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1429606145-14901-1-git-send-email-e@80x24.org \
    --to=e@80x24.org \
    --cc=yahns-public@yhbt.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://yhbt.net/yahns.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).