You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
453 lines
15 KiB
453 lines
15 KiB
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 |
|
@@ -810,7 +810,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 |
|
@@ -142,7 +142,8 @@ Downstream::Downstream(Upstream *upstream, MemchunkPool *mcpool, |
|
request_pending_(false), |
|
request_header_sent_(false), |
|
accesslog_written_(false), |
|
- new_affinity_cookie_(false) { |
|
+ new_affinity_cookie_(false), |
|
+ expect_100_continue_(false) { |
|
|
|
auto &timeoutconf = get_config()->http2.timeout; |
|
|
|
@@ -764,6 +765,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() { |
|
@@ -1052,4 +1058,8 @@ uint32_t Downstream::get_affinity_cookie_to_send() const { |
|
return 0; |
|
} |
|
|
|
+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 |
|
@@ -463,6 +463,8 @@ public: |
|
EVENT_TIMEOUT = 0x2, |
|
}; |
|
|
|
+ bool get_expect_100_continue() const; |
|
+ |
|
enum { |
|
DISPATCH_NONE, |
|
DISPATCH_PENDING, |
|
@@ -547,6 +549,8 @@ private: |
|
bool accesslog_written_; |
|
// true if affinity cookie is generated for this request. |
|
bool new_affinity_cookie_; |
|
+ // 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 |
|
@@ -694,7 +694,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 || |
|
- (!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(); |
|
} |
|
|
|
@@ -1142,6 +1143,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 |
|
@@ -448,9 +448,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 |
|
@@ -2632,6 +2632,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 |
|
@@ -112,3 +112,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 |
|
@@ -447,6 +447,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) && |
|
@@ -506,6 +507,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, |
|
@@ -6654,7 +6659,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; |
|
} |
|
|
|
@@ -6799,7 +6804,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 |
|
@@ -96,7 +96,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 |
|
@@ -258,8 +258,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 |
|
@@ -9552,7 +9552,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))); |
|
@@ -9574,7 +9574,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
|
|
|