From 58d3b5b4a0b45941bc1a8b5386f5ab84d45f099e Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Sun, 15 Feb 2015 00:24:52 +0900
Subject: [PATCH] nghttpx: Fix occasional HTTP/2 backend connection failure
 with proxy

Previously if HTTP/1 proxy is used for backend connection, we read all
incoming bytes from proxy including response body, which may be part
of HTTP/2 protocol.  While investigating this issue, we found that
http_parser_execute() returns 1-less length when we call
http_parser_pause() inside on_headers_complete callback.  To
workaround this, we increment the return value by 1.  This commit also
fixes possible segmentation fault error, which could be caused by the
lack of stopping libev watcher in disconnect().
---
 src/shrpx_http2_session.cc | 71 ++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc
index eef42369..eb870bb5 100644
--- a/src/shrpx_http2_session.cc
+++ b/src/shrpx_http2_session.cc
@@ -184,6 +184,9 @@ int Http2Session::disconnect(bool hard) {
   rb_.reset();
   wb_.reset();
 
+  conn_.rlimit.stopw();
+  conn_.wlimit.stopw();
+
   ev_timer_stop(conn_.loop, &settings_timer_);
   ev_timer_stop(conn_.loop, &connchk_timer_);
 
@@ -198,6 +201,7 @@ int Http2Session::disconnect(bool hard) {
 
   connection_check_state_ = CONNECTION_CHECK_NONE;
   state_ = DISCONNECTED;
+  write_requested_ = false;
 
   // Delete all client handler associated to Downstream. When deleting
   // Http2DownstreamConnection, it calls this object's
@@ -328,6 +332,9 @@ int Http2Session::initiate_connection() {
         if (rv != 0 && errno != EINPROGRESS) {
           return -1;
         }
+
+        ev_io_set(&conn_.rev, conn_.fd, EV_READ);
+        ev_io_set(&conn_.wev, conn_.fd, EV_WRITE);
       }
 
       if (SSL_set_fd(conn_.tls.ssl, conn_.fd) == 0) {
@@ -353,25 +360,12 @@ int Http2Session::initiate_connection() {
         if (rv != 0 && errno != EINPROGRESS) {
           return -1;
         }
-      } else {
-        // Without TLS but with proxy.  Connection already
-        // established.
-        if (on_connect() != -1) {
-          state_ = CONNECT_FAILING;
-          return -1;
-        }
+
+        ev_io_set(&conn_.rev, conn_.fd, EV_READ);
+        ev_io_set(&conn_.wev, conn_.fd, EV_WRITE);
       }
     }
 
-    // rev_ and wev_ could possibly be active here.  Since calling
-    // ev_io_set is not allowed while watcher is active, we have to
-    // stop them just in case.
-    conn_.rlimit.stopw();
-    conn_.wlimit.stopw();
-
-    ev_io_set(&conn_.rev, conn_.fd, EV_READ);
-    ev_io_set(&conn_.wev, conn_.fd, EV_WRITE);
-
     write_ = &Http2Session::connected;
 
     on_write_ = &Http2Session::downstream_write;
@@ -399,6 +393,18 @@ int Http2Session::initiate_connection() {
 namespace {
 int htp_hdrs_completecb(http_parser *htp) {
   auto http2session = static_cast<Http2Session *>(htp->data);
+
+  // We only read HTTP header part.  If tunneling succeeds, response
+  // body is a different protocol (HTTP/2 in this case), we don't read
+  // them here.
+  //
+  // Here is a caveat: http-parser returns 1 less bytes if we pause
+  // here.  The reason why they do this is probably they want to eat
+  // last 1 byte in s_headers_done state, on the other hand, this
+  // callback is called its previous state s_headers_almost_done.  We
+  // will do "+ 1" to the return value to workaround this.
+  http_parser_pause(htp, 1);
+
   // We just check status code here
   if (htp->status_code == 200) {
     if (LOG_ENABLED(INFO)) {
@@ -443,20 +449,33 @@ int Http2Session::downstream_read_proxy() {
 
     auto htperr = HTTP_PARSER_ERRNO(proxy_htp_.get());
 
+    if (htperr == HPE_PAUSED) {
+      switch (state_) {
+      case Http2Session::PROXY_CONNECTED:
+        // we need to increment nread by 1 since http_parser_execute()
+        // returns 1 less value we expect.  This means taht
+        // rb_.pos[nread] points to \x0a (LF), which is last byte of
+        // empty line to terminate headers.  We want to eat that byte
+        // here.
+        rb_.drain(1);
+
+        // Initiate SSL/TLS handshake through established tunnel.
+        if (initiate_connection() != 0) {
+          return -1;
+        }
+        return 0;
+      case Http2Session::PROXY_FAILED:
+        return -1;
+      }
+      // should not be here
+      assert(0);
+    }
+
     if (htperr != HPE_OK) {
       return -1;
     }
 
-    switch (state_) {
-    case Http2Session::PROXY_CONNECTED:
-      // Initiate SSL/TLS handshake through established tunnel.
-      if (initiate_connection() != 0) {
-        return -1;
-      }
-      return 0;
-    case Http2Session::PROXY_FAILED:
-      return -1;
-    }
+    return 0;
   }
 }