[PATCH 0/3] another round of proxy_pass fixes

[PATCH 0/3] another round of proxy_pass fixes

[Eric Wong]

I really hope this will be the last round of critical fixes :)

Stress-tested numerous times on a yet-to-be-public 800M+ git
repo over dumb HTTP cloning:

	GIT_SMART_HTTP=0 git clone (client)
	|
	v
	yahns HTTP/HTTPS proxy_pass(proxy_buffering: false)
	|
	v
	varnish 4.0.2 (random chunks results,
		       even given Content-Length :p)
	|
	v
	static file server (with some dynamic parts)

Eric Wong (3):
      proxy_pass: avoid TOCTTOU race when unbuffering, too
      proxy_pass: avoid accessing logger in env after hijacking
      proxy_pass: avoid stuck responses in "proxy_buffering: false"

 lib/yahns/proxy_http_response.rb | 8 +++-----
 lib/yahns/wbuf_lite.rb           | 8 ++++++--
 2 files changed, 9 insertions(+), 7 deletions(-)

P.S. and git dumb HTTP cloning could probably be better about
memory/CPU usage...

[PATCH 1/3] proxy_pass: avoid TOCTTOU race when unbuffering, too

[Eric Wong]

proxy_unbuffer is vulnerable to the same race condition
we avoided in commit 5328992829b2
("proxy_pass: fix race condition due to flawed hijack check")
---
 lib/yahns/proxy_http_response.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index 0ca2c34..a37b387 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -13,10 +13,8 @@ module Yahns::HttpResponse # :nodoc:
   def proxy_unbuffer(wbuf, nxt = :ignore)
     @state = wbuf
     wbuf.req_res = nil if nxt.nil? && wbuf.respond_to?(:req_res=)
-    tc = Thread.current
-    tc[:yahns_fdmap].remember(self) # Yahns::HttpClient
-    tc[:yahns_queue].queue_mod(self, wbuf.busy == :wait_readable ?
-                               Yahns::Queue::QEV_RD : Yahns::Queue::QEV_WR)
+    proxy_wait_next(wbuf.busy == :wait_readable ? Yahns::Queue::QEV_RD :
+                    Yahns::Queue::QEV_WR)
     nxt
   end
 
-- 
EW

[PATCH 2/3] proxy_pass: avoid accessing logger in env after hijacking

[Eric Wong]

The HTTP state (@hs) object could be replaced in proxy_wait_next
causing @hs.env['rack.logger'] to become inaccessible.
---
 lib/yahns/proxy_http_response.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index a37b387..74f5ce5 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -46,7 +46,7 @@ def proxy_write(wbuf, buf, req_res)
   end
 
   def proxy_err_response(code, req_res, exc)
-    logger = @hs.env['rack.logger']
+    logger = self.class.logger # Yahns::HttpContext#logger
     case exc
     when nil
       logger.error('premature upstream EOF')
-- 
EW

[PATCH 3/3] proxy_pass: avoid stuck responses in "proxy_buffering: false"

[Eric Wong]

Another critical bugfix for this yet-to-be-released feature.
By the time we call proxy_unbuffer in proxy_read_body,
the req_res socket may be completely drained of readable data
and a persistent-connection-capable backend will be waiting
for the next request (not knowing we do not yet support
persistent connections).

This affects both chunked and identity responses from the
upstream.
---
 lib/yahns/wbuf_lite.rb | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/yahns/wbuf_lite.rb b/lib/yahns/wbuf_lite.rb
index fa52f54..25daf21 100644
--- a/lib/yahns/wbuf_lite.rb
+++ b/lib/yahns/wbuf_lite.rb
@@ -33,10 +33,14 @@ def wbuf_flush(client)
   def wbuf_close(client)
     wbuf_abort
 
-    # resume reading when @blocked is empty
+    # resume the event loop when @blocked is empty
+    # The actual Yahns::ReqRes#yahns_step is actually read/write-event
+    # agnostic, and we should actually watch for writability here since
+    # the req_res socket itself could be completely drained of readable
+    # data and just waiting for another request (which we don't support, yet)
     if @req_res
       client.hijack_cleanup
-      Thread.current[:yahns_queue].queue_mod(@req_res, Yahns::Queue::QEV_RD)
+      Thread.current[:yahns_queue].queue_mod(@req_res, Yahns::Queue::QEV_WR)
       return :ignore
     end
     @wbuf_persist
-- 
EW