From 4b7aefd8fd1612d455f2f128c09230335ed0cee6 Mon Sep 17 00:00:00 2001

From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>

Date: Tue, 6 Aug 2019 20:48:50 +0900

Subject: [PATCH 1/3] nghttpx: Fix request stall

Fix request stall if backend connection is reused and buffer is full.

Upstream-commit: db2f612a30d54aa152ce5530fa1d683738baa4d1

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 integration-tests/nghttpx_http1_test.go | 29 +++++++++++++++++++++++++

 integration-tests/server_tester.go      |  4 +++-

 src/shrpx_downstream.cc                 | 12 +++++++++-

 src/shrpx_downstream.h                  |  4 ++++

 src/shrpx_http_downstream_connection.cc | 16 +++++++++++++-

 src/shrpx_https_upstream.cc             |  4 +---

 6 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/integration-tests/nghttpx_http1_test.go b/integration-tests/nghttpx_http1_test.go

index a765333..3d41677 100644

--- a/integration-tests/nghttpx_http1_test.go

+++ b/integration-tests/nghttpx_http1_test.go

@@ -625,6 +625,35 @@ func TestH1H1HTTPSRedirectPort(t *testing.T) {

 	}

 }

+// TestH1H1POSTRequests tests that server can handle 2 requests with

+// request body.

+func TestH1H1POSTRequests(t *testing.T) {

+	st := newServerTester(nil, t, noopHandler)

+	defer st.Close()

+

+	res, err := st.http1(requestParam{

+		name: "TestH1H1POSTRequestsNo1",

+		body: make([]byte, 1),

+	})

+	if err != nil {

+		t.Fatalf("Error st.http1() = %v", err)

+	}

+	if got, want := res.status, 200; got != want {

+		t.Errorf("res.status: %v; want %v", got, want)

+	}

+

+	res, err = st.http1(requestParam{

+		name: "TestH1H1POSTRequestsNo2",

+		body: make([]byte, 65536),

+	})

+	if err != nil {

+		t.Fatalf("Error st.http1() = %v", err)

+	}

+	if got, want := res.status, 200; got != want {

+		t.Errorf("res.status: %v; want %v", got, want)

+	}

+}

+

 // // TestH1H2ConnectFailure tests that server handles the situation that

 // // connection attempt to HTTP/2 backend failed.

 // func TestH1H2ConnectFailure(t *testing.T) {

diff --git a/integration-tests/server_tester.go b/integration-tests/server_tester.go

index d145519..1156986 100644

--- a/integration-tests/server_tester.go

+++ b/integration-tests/server_tester.go

@@ -662,7 +662,9 @@ func cloneHeader(h http.Header) http.Header {

 	return h2

 }

-func noopHandler(w http.ResponseWriter, r *http.Request) {}

+func noopHandler(w http.ResponseWriter, r *http.Request) {

+	ioutil.ReadAll(r.Body)

+}

 type APIResponse struct {

 	Status string                 `json:"status,omitempty"`

diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc

index 360a9a9..48db65b 100644

--- a/src/shrpx_downstream.cc

+++ b/src/shrpx_downstream.cc

@@ -144,7 +144,8 @@ Downstream::Downstream(Upstream *upstream, MemchunkPool *mcpool,

       request_header_sent_(false),

       accesslog_written_(false),

       new_affinity_cookie_(false),

-      blocked_request_data_eof_(false) {

+      blocked_request_data_eof_(false),

+      expect_100_continue_(false) {

   auto &timeoutconf = get_config()->http2.timeout;

@@ -807,6 +808,11 @@ void Downstream::inspect_http1_request() {

       chunked_request_ = true;

     }

   }

+

+  auto expect = req_.fs.header(http2::HD_EXPECT);

+  expect_100_continue_ =

+      expect &&

+      util::strieq(expect->value, StringRef::from_lit("100-continue"));

 }

 void Downstream::inspect_http1_response() {

@@ -1103,4 +1109,8 @@ bool Downstream::get_blocked_request_data_eof() const {

   return blocked_request_data_eof_;

 }

+bool Downstream::get_expect_100_continue() const {

+  return expect_100_continue_;

+}

+

 } // namespace shrpx

diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h

index c81fcf6..b9a851f 100644

--- a/src/shrpx_downstream.h

+++ b/src/shrpx_downstream.h

@@ -466,6 +466,8 @@ public:

     EVENT_TIMEOUT = 0x2,

   };

+  bool get_expect_100_continue() const;

+

   enum {

     DISPATCH_NONE,

     DISPATCH_PENDING,

@@ -556,6 +558,8 @@ private:

   // true if eof is received from client before sending header fields

   // to backend.

   bool blocked_request_data_eof_;

+  // true if request contains "expect: 100-continue" header field.

+  bool expect_100_continue_;

 };

 } // namespace shrpx

diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc

index f50c0f4..85ca947 100644

--- a/src/shrpx_http_downstream_connection.cc

+++ b/src/shrpx_http_downstream_connection.cc

@@ -698,7 +698,8 @@ int HttpDownstreamConnection::push_request_headers() {

   // signal_write() when we received request body chunk, and it

   // enables us to send headers and data in one writev system call.

   if (connect_method || downstream_->get_blocked_request_buf()->rleft() ||

-      (!req.http2_expect_body && req.fs.content_length == 0)) {

+      (!req.http2_expect_body && req.fs.content_length == 0) ||

+      downstream_->get_expect_100_continue()) {

     signal_write();

   }

@@ -1172,6 +1173,19 @@ int HttpDownstreamConnection::write_reuse_first() {

   reuse_first_write_done_ = true;

+  // upstream->resume_read() might be called in

+  // write_tls()/write_clear(), but before blocked_request_buf_ is

+  // reset.  So upstream read might still be blocked.  Let's do it

+  // again here.

+  auto input = downstream_->get_request_buf();

+  if (input->rleft() == 0) {

+    auto upstream = downstream_->get_upstream();

+    auto &req = downstream_->request();

+

+    upstream->resume_read(SHRPX_NO_BUFFER, downstream_,

+                          req.unconsumed_body_length);

+  }

+

   return 0;

 }

diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc

index 452ec90..96ca2cd 100644

--- a/src/shrpx_https_upstream.cc

+++ b/src/shrpx_https_upstream.cc

@@ -467,9 +467,7 @@ int htp_hdrs_completecb(http_parser *htp) {

     // and let them decide whether responds with 100 Continue or not.

     // For alternative mode, we have no backend, so just send 100

     // Continue here to make the client happy.

-    auto expect = req.fs.header(http2::HD_EXPECT);

-    if (expect &&

-        util::strieq(expect->value, StringRef::from_lit("100-continue"))) {

+    if (downstream->get_expect_100_continue()) {

       auto output = downstream->get_response_buf();

       constexpr auto res = StringRef::from_lit("HTTP/1.1 100 Continue\r\n\r\n");

       output->append(res);

-- 

2.20.1

From 589a98eba0b3c7a4dbb2262c60b609cac2b1f838 Mon Sep 17 00:00:00 2001

From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>

Date: Tue, 25 Jun 2019 22:33:35 +0900

Subject: [PATCH 2/3] Add nghttp2_option_set_max_outbound_ack

Upstream-commit: a76d0723b5f52902139ff453e0ec840673e86e75

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 doc/Makefile.am                |  1 +

 lib/includes/nghttp2/nghttp2.h | 11 +++++++++++

 lib/nghttp2_option.c           |  5 +++++

 lib/nghttp2_option.h           |  5 +++++

 lib/nghttp2_session.c          |  9 +++++++--

 lib/nghttp2_session.h          |  8 ++++++--

 tests/nghttp2_session_test.c   |  4 ++--

 7 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/doc/Makefile.am b/doc/Makefile.am

index 07cd34e..66e5ba3 100644

--- a/doc/Makefile.am

+++ b/doc/Makefile.am

@@ -67,6 +67,7 @@ APIDOCS= \

 	nghttp2_option_set_no_recv_client_magic.rst \

 	nghttp2_option_set_peer_max_concurrent_streams.rst \

 	nghttp2_option_set_user_recv_extension_type.rst \

+	nghttp2_option_set_max_outbound_ack.rst \

 	nghttp2_pack_settings_payload.rst \

 	nghttp2_priority_spec_check_default.rst \

 	nghttp2_priority_spec_default_init.rst \

diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h

index 14f8950..137a675 100644

--- a/lib/includes/nghttp2/nghttp2.h

+++ b/lib/includes/nghttp2/nghttp2.h

@@ -2637,6 +2637,17 @@ nghttp2_option_set_max_deflate_dynamic_table_size(nghttp2_option *option,

 NGHTTP2_EXTERN void nghttp2_option_set_no_closed_streams(nghttp2_option *option,

                                                          int val);

+/**

+ * @function

+ *

+ * This function sets the maximum number of outgoing SETTINGS ACK and

+ * PING ACK frames retained in :type:`nghttp2_session` object.  If

+ * more than those frames are retained, the peer is considered to be

+ * misbehaving and session will be closed.  The default value is 1000.

+ */

+NGHTTP2_EXTERN void nghttp2_option_set_max_outbound_ack(nghttp2_option *option,

+                                                        size_t val);

+

 /**

  * @function

  *

diff --git a/lib/nghttp2_option.c b/lib/nghttp2_option.c

index aec5dcf..ae22493 100644

--- a/lib/nghttp2_option.c

+++ b/lib/nghttp2_option.c

@@ -116,3 +116,8 @@ void nghttp2_option_set_no_closed_streams(nghttp2_option *option, int val) {

   option->opt_set_mask |= NGHTTP2_OPT_NO_CLOSED_STREAMS;

   option->no_closed_streams = val;

 }

+

+void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, size_t val) {

+  option->opt_set_mask |= NGHTTP2_OPT_MAX_OUTBOUND_ACK;

+  option->max_outbound_ack = val;

+}

diff --git a/lib/nghttp2_option.h b/lib/nghttp2_option.h

index c743e33..86d31f7 100644

--- a/lib/nghttp2_option.h

+++ b/lib/nghttp2_option.h

@@ -66,6 +66,7 @@ typedef enum {

   NGHTTP2_OPT_MAX_SEND_HEADER_BLOCK_LENGTH = 1 << 8,

   NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE = 1 << 9,

   NGHTTP2_OPT_NO_CLOSED_STREAMS = 1 << 10,

+  NGHTTP2_OPT_MAX_OUTBOUND_ACK = 1 << 11,

 } nghttp2_option_flag;

 /**

@@ -80,6 +81,10 @@ struct nghttp2_option {

    * NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE

    */

   size_t max_deflate_dynamic_table_size;

+  /**

+   * NGHTTP2_OPT_MAX_OUTBOUND_ACK

+   */

+  size_t max_outbound_ack;

   /**

    * Bitwise OR of nghttp2_option_flag to determine that which fields

    * are specified.

diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c

index c58f059..8628cc7 100644

--- a/lib/nghttp2_session.c

+++ b/lib/nghttp2_session.c

@@ -457,6 +457,7 @@ static int session_new(nghttp2_session **session_ptr,

   (*session_ptr)->remote_settings.max_concurrent_streams = 100;

   (*session_ptr)->max_send_header_block_length = NGHTTP2_MAX_HEADERSLEN;

+  (*session_ptr)->max_outbound_ack = NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM;

   if (option) {

     if ((option->opt_set_mask & NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE) &&

@@ -516,6 +517,10 @@ static int session_new(nghttp2_session **session_ptr,

         option->no_closed_streams) {

       (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_NO_CLOSED_STREAMS;

     }

+

+    if (option->opt_set_mask & NGHTTP2_OPT_MAX_OUTBOUND_ACK) {

+      (*session_ptr)->max_outbound_ack = option->max_outbound_ack;

+    }

   }

   rv = nghttp2_hd_deflate_init2(&(*session_ptr)->hd_deflater,

@@ -6831,7 +6836,7 @@ int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags,

   mem = &session->mem;

   if ((flags & NGHTTP2_FLAG_ACK) &&

-      session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) {

+      session->obq_flood_counter_ >= session->max_outbound_ack) {

     return NGHTTP2_ERR_FLOODED;

   }

@@ -6976,7 +6981,7 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,

       return NGHTTP2_ERR_INVALID_ARGUMENT;

     }

-    if (session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) {

+    if (session->obq_flood_counter_ >= session->max_outbound_ack) {

       return NGHTTP2_ERR_FLOODED;

     }

   }

diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h

index c7cb27d..d9e2846 100644

--- a/lib/nghttp2_session.h

+++ b/lib/nghttp2_session.h

@@ -97,7 +97,7 @@ typedef struct {

    response frames are stacked up, which leads to memory exhaustion.

    The value selected here is arbitrary, but safe value and if we have

    these frames in this number, it is considered suspicious. */

-#define NGHTTP2_MAX_OBQ_FLOOD_ITEM 10000

+#define NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM 1000

 /* The default value of maximum number of concurrent streams. */

 #define NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS 0xffffffffu

@@ -260,8 +260,12 @@ struct nghttp2_session {

   size_t num_idle_streams;

   /* The number of bytes allocated for nvbuf */

   size_t nvbuflen;

-  /* Counter for detecting flooding in outbound queue */

+  /* Counter for detecting flooding in outbound queue.  If it exceeds

+     max_outbound_ack, session will be closed. */

   size_t obq_flood_counter_;

+  /* The maximum number of outgoing SETTINGS ACK and PING ACK in

+     outbound queue. */

+  size_t max_outbound_ack;

   /* The maximum length of header block to send.  Calculated by the

      same way as nghttp2_hd_deflate_bound() does. */

   size_t max_send_header_block_length;

diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c

index 783b0ed..debec59 100644

--- a/tests/nghttp2_session_test.c

+++ b/tests/nghttp2_session_test.c

@@ -9894,7 +9894,7 @@ void test_nghttp2_session_flooding(void) {

   buf = &bufs.head->buf;

-  for (i = 0; i < NGHTTP2_MAX_OBQ_FLOOD_ITEM; ++i) {

+  for (i = 0; i < NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM; ++i) {

     CU_ASSERT(

         (ssize_t)nghttp2_buf_len(buf) ==

         nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)));

@@ -9916,7 +9916,7 @@ void test_nghttp2_session_flooding(void) {

   buf = &bufs.head->buf;

-  for (i = 0; i < NGHTTP2_MAX_OBQ_FLOOD_ITEM; ++i) {

+  for (i = 0; i < NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM; ++i) {

     CU_ASSERT(

         (ssize_t)nghttp2_buf_len(buf) ==

         nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)));

-- 

2.20.1

From e32b3e4c9df4abb83ca1c1c41901fadbae44699b Mon Sep 17 00:00:00 2001

From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>

Date: Tue, 25 Jun 2019 22:38:43 +0900

Subject: [PATCH 3/3] Don't read too greedily

Upstream-commit: 83d362c6d21f76599b86e7b94cd1992288a1d43c

Signed-off-by: Kamil Dudka <kdudka@redhat.com>

---

 src/HttpServer.cc           | 2 ++

 src/shrpx_client_handler.cc | 9 +++++++--

 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/HttpServer.cc b/src/HttpServer.cc

index b3e35ef..a75cee4 100644

--- a/src/HttpServer.cc

+++ b/src/HttpServer.cc

@@ -650,6 +650,7 @@ int Http2Handler::read_clear() {

       }

       return -1;

     }

+    break;

   }

   return write_(*this);

@@ -775,6 +776,7 @@ int Http2Handler::read_tls() {

       }

       return -1;

     }

+    break;

   }

 fin:

diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc

index 21430dd..fa1fc87 100644

--- a/src/shrpx_client_handler.cc

+++ b/src/shrpx_client_handler.cc

@@ -111,6 +111,7 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) {

 int ClientHandler::noop() { return 0; }

 int ClientHandler::read_clear() {

+  auto should_break = false;

   rb_.ensure_chunk();

   for (;;) {

     if (rb_.rleft() && on_read() != 0) {

@@ -123,7 +124,7 @@ int ClientHandler::read_clear() {

       return 0;

     }

-    if (!ev_is_active(&conn_.rev)) {

+    if (!ev_is_active(&conn_.rev) || should_break) {

       return 0;

     }

@@ -141,6 +142,7 @@ int ClientHandler::read_clear() {

     }

     rb_.write(nread);

+    should_break = true;

   }

 }

@@ -205,6 +207,8 @@ int ClientHandler::tls_handshake() {

 }

 int ClientHandler::read_tls() {

+  auto should_break = false;

+

   ERR_clear_error();

   rb_.ensure_chunk();

@@ -221,7 +225,7 @@ int ClientHandler::read_tls() {

       return 0;

     }

-    if (!ev_is_active(&conn_.rev)) {

+    if (!ev_is_active(&conn_.rev) || should_break) {

       return 0;

     }

@@ -239,6 +243,7 @@ int ClientHandler::read_tls() {

     }

     rb_.write(nread);

+    should_break = true;

   }

 }

-- 

2.20.1