From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-2.9 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=unavailable version=3.3.2 X-Original-To: yahns-public@yhbt.net Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id DB2861F488 for ; Tue, 21 Apr 2015 08:49:05 +0000 (UTC) From: Eric Wong 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 Message-Id: <1429606145-14901-1-git-send-email-e@80x24.org> List-Id: 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 | 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 + # | 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