From eb393985b78c7eca4f948c1ad0ebf76930680cbc Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 11 Mar 2016 00:50:27 +0900 Subject: [PATCH] nghttpx: Make a copy before adding header to Downstream --- src/http2.cc | 9 +++++ src/http2.h | 3 ++ src/shrpx-unittest.cc | 4 +- src/shrpx_downstream.cc | 49 +++++++------------------ src/shrpx_downstream.h | 4 -- src/shrpx_downstream_test.cc | 39 +++++++------------- src/shrpx_downstream_test.h | 2 +- src/shrpx_http2_session.cc | 20 ++++++---- src/shrpx_http2_upstream.cc | 32 +++++++++------- src/shrpx_http_downstream_connection.cc | 9 ++++- src/shrpx_https_upstream.cc | 9 ++++- src/shrpx_mruby_module_request.cc | 20 ++++++---- src/shrpx_mruby_module_response.cc | 24 +++++++----- src/shrpx_spdy_upstream.cc | 4 +- 14 files changed, 118 insertions(+), 110 deletions(-) diff --git a/src/http2.cc b/src/http2.cc index 3a4474b8..8385cafe 100644 --- a/src/http2.cc +++ b/src/http2.cc @@ -1793,6 +1793,15 @@ StringRef rewrite_clean_path(BlockAllocator &balloc, const StringRef &src) { StringRef{query, fragment}); } +StringRef copy_lower(BlockAllocator &balloc, const StringRef &src) { + auto iov = make_byte_ref(balloc, src.size() + 1); + auto p = iov.base; + p = std::copy(std::begin(src), std::end(src), p); + *p = '\0'; + util::inp_strlower(iov.base, p); + return StringRef{iov.base, p}; +} + } // namespace http2 } // namespace nghttp2 diff --git a/src/http2.h b/src/http2.h index cc295079..48d4916d 100644 --- a/src/http2.h +++ b/src/http2.h @@ -385,6 +385,9 @@ int construct_push_component(BlockAllocator &balloc, StringRef &scheme, StringRef &authority, StringRef &path, const StringRef &base, const StringRef &uri); +// Copies |src| and return its lower-cased version. +StringRef copy_lower(BlockAllocator &balloc, const StringRef &src); + } // namespace http2 } // namespace nghttp2 diff --git a/src/shrpx-unittest.cc b/src/shrpx-unittest.cc index 8cd75690..0f2e2fd2 100644 --- a/src/shrpx-unittest.cc +++ b/src/shrpx-unittest.cc @@ -101,8 +101,8 @@ int main(int argc, char *argv[]) { shrpx::test_http2_get_pure_path_component) || !CU_add_test(pSuite, "http2_construct_push_component", shrpx::test_http2_construct_push_component) || - !CU_add_test(pSuite, "downstream_field_store_add_header_lower", - shrpx::test_downstream_field_store_add_header_lower) || + !CU_add_test(pSuite, "downstream_field_store_append_last_header", + shrpx::test_downstream_field_store_append_last_header) || !CU_add_test(pSuite, "downstream_field_store_header", shrpx::test_downstream_field_store_header) || !CU_add_test(pSuite, "downstream_crumble_request_cookie", diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index eadfed39..51931aad 100644 --- a/src/shrpx_downstream.cc +++ b/src/shrpx_downstream.cc @@ -348,23 +348,21 @@ void add_header(bool &key_prev, size_t &sum, HeaderRefs &headers, } } // namespace -namespace { -void add_header(size_t &sum, HeaderRefs &headers, const StringRef &name, - const StringRef &value, bool no_index, int32_t token) { - sum += name.size() + value.size(); - headers.emplace_back(name, value, no_index, token); -} -} // namespace - namespace { void append_last_header_key(BlockAllocator &balloc, bool &key_prev, size_t &sum, HeaderRefs &headers, const char *data, size_t len) { assert(key_prev); sum += len; auto &item = headers.back(); - item.name = concat_string_ref(balloc, item.name, StringRef{data, len}); - util::inp_strlower((uint8_t *)item.name.c_str(), - (uint8_t *)item.name.c_str() + item.name.size()); + auto iov = make_byte_ref(balloc, item.name.size() + len + 1); + auto p = iov.base; + p = std::copy(std::begin(item.name), std::end(item.name), p); + p = std::copy_n(data, len, p); + util::inp_strlower(p - len, p); + *p = '\0'; + + item.name = StringRef{iov.base, p}; + item.token = http2::lookup_token(item.name); } } // namespace @@ -424,21 +422,10 @@ const HeaderRefs::value_type *FieldStore::header(const StringRef &name) const { return search_header_linear_backwards(headers_, name); } -void FieldStore::add_header_lower(const StringRef &name, const StringRef &value, - bool no_index) { - auto low_name = make_string_ref(balloc_, name); - util::inp_strlower((uint8_t *)low_name.c_str(), - (uint8_t *)low_name.c_str() + low_name.size()); - auto token = http2::lookup_token(low_name); - shrpx::add_header(header_key_prev_, buffer_size_, headers_, low_name, - make_string_ref(balloc_, value), no_index, token); -} - void FieldStore::add_header_token(const StringRef &name, const StringRef &value, bool no_index, int32_t token) { - shrpx::add_header(buffer_size_, headers_, make_string_ref(balloc_, name), - make_string_ref(balloc_, value), no_index, token); - make_string_ref(balloc_, name); + shrpx::add_header(header_key_prev_, buffer_size_, headers_, name, value, + no_index, token); } void FieldStore::append_last_header_key(const char *data, size_t len) { @@ -453,23 +440,13 @@ void FieldStore::append_last_header_value(const char *data, size_t len) { void FieldStore::clear_headers() { headers_.clear(); } -void FieldStore::add_trailer_lower(const StringRef &name, - const StringRef &value, bool no_index) { - auto low_name = make_string_ref(balloc_, name); - util::inp_strlower((uint8_t *)low_name.c_str(), - (uint8_t *)low_name.c_str() + low_name.size()); - auto token = http2::lookup_token(low_name); - shrpx::add_header(trailer_key_prev_, buffer_size_, trailers_, low_name, - make_string_ref(balloc_, value), no_index, token); -} - void FieldStore::add_trailer_token(const StringRef &name, const StringRef &value, bool no_index, int32_t token) { // Header size limit should be applied to all header and trailer // fields combined. - shrpx::add_header(buffer_size_, trailers_, make_string_ref(balloc_, name), - make_string_ref(balloc_, value), no_index, token); + shrpx::add_header(trailer_key_prev_, buffer_size_, trailers_, name, value, + no_index, token); } void FieldStore::append_last_trailer_key(const char *data, size_t len) { diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h index cea17d86..a5024169 100644 --- a/src/shrpx_downstream.h +++ b/src/shrpx_downstream.h @@ -81,8 +81,6 @@ public: // such header is found, returns nullptr. const HeaderRefs::value_type *header(const StringRef &name) const; - void add_header_lower(const StringRef &name, const StringRef &value, - bool no_index); void add_header_token(const StringRef &name, const StringRef &value, bool no_index, int32_t token); @@ -98,8 +96,6 @@ public: // Empties headers. void clear_headers(); - void add_trailer_lower(const StringRef &name, const StringRef &value, - bool no_index); void add_trailer_token(const StringRef &name, const StringRef &value, bool no_index, int32_t token); diff --git a/src/shrpx_downstream_test.cc b/src/shrpx_downstream_test.cc index 5bfcda67..786e9046 100644 --- a/src/shrpx_downstream_test.cc +++ b/src/shrpx_downstream_test.cc @@ -32,35 +32,22 @@ namespace shrpx { -void test_downstream_field_store_add_header_lower(void) { +void test_downstream_field_store_append_last_header(void) { BlockAllocator balloc(4096, 4096); FieldStore fs(balloc, 0); - fs.add_header_lower(StringRef::from_lit("1"), StringRef::from_lit("0"), - false); - fs.add_header_lower(StringRef::from_lit("2"), StringRef::from_lit("1"), - false); - fs.add_header_lower(StringRef::from_lit("Charlie"), StringRef::from_lit("2"), - false); - fs.add_header_lower(StringRef::from_lit("Alpha"), StringRef::from_lit("3"), - false); - fs.add_header_lower(StringRef::from_lit("Delta"), StringRef::from_lit("4"), - false); - fs.add_header_lower(StringRef::from_lit("BravO"), StringRef::from_lit("5"), - false); - fs.add_header_lower(StringRef::from_lit(":method"), StringRef::from_lit("6"), - false); - fs.add_header_lower(StringRef::from_lit(":authority"), - StringRef::from_lit("7"), false); + fs.add_header_token(StringRef::from_lit("alpha"), StringRef{}, false, -1); + auto bravo = StringRef::from_lit("BRAVO"); + fs.append_last_header_key(bravo.c_str(), bravo.size()); + auto charlie = StringRef::from_lit("Charlie"); + fs.append_last_header_value(charlie.c_str(), charlie.size()); + auto delta = StringRef::from_lit("deltA"); + fs.append_last_header_value(delta.c_str(), delta.size()); + fs.add_header_token(StringRef::from_lit("echo"), + StringRef::from_lit("foxtrot"), false, -1); - auto ans = - HeaderRefs{{StringRef::from_lit("1"), StringRef::from_lit("0")}, - {StringRef::from_lit("2"), StringRef::from_lit("1")}, - {StringRef::from_lit("charlie"), StringRef::from_lit("2")}, - {StringRef::from_lit("alpha"), StringRef::from_lit("3")}, - {StringRef::from_lit("delta"), StringRef::from_lit("4")}, - {StringRef::from_lit("bravo"), StringRef::from_lit("5")}, - {StringRef::from_lit(":method"), StringRef::from_lit("6")}, - {StringRef::from_lit(":authority"), StringRef::from_lit("7")}}; + auto ans = HeaderRefs{ + {StringRef::from_lit("alphabravo"), StringRef::from_lit("CharliedeltA")}, + {StringRef::from_lit("echo"), StringRef::from_lit("foxtrot")}}; CU_ASSERT(ans == fs.headers()); } diff --git a/src/shrpx_downstream_test.h b/src/shrpx_downstream_test.h index bcae1887..f7b1e603 100644 --- a/src/shrpx_downstream_test.h +++ b/src/shrpx_downstream_test.h @@ -31,7 +31,7 @@ namespace shrpx { -void test_downstream_field_store_add_header_lower(void); +void test_downstream_field_store_append_last_header(void); void test_downstream_field_store_header(void); void test_downstream_crumble_request_cookie(void); void test_downstream_assemble_request_cookie(void); diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index 828d888c..25a7f996 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -818,6 +818,7 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, auto &resp = downstream->response(); auto &httpconf = get_config()->http; + auto &balloc = downstream->get_block_allocator(); switch (frame->hd.type) { case NGHTTP2_HEADERS: { @@ -847,13 +848,15 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, if (trailer) { // just store header fields for trailer part - resp.fs.add_trailer_token(StringRef{name, namelen}, - StringRef{value, valuelen}, no_index, token); + resp.fs.add_trailer_token( + make_string_ref(balloc, StringRef{name, namelen}), + make_string_ref(balloc, StringRef{value, valuelen}), no_index, token); return 0; } - resp.fs.add_header_token(StringRef{name, namelen}, - StringRef{value, valuelen}, no_index, token); + resp.fs.add_header_token( + make_string_ref(balloc, StringRef{name, namelen}), + make_string_ref(balloc, StringRef{value, valuelen}), no_index, token); return 0; } case NGHTTP2_PUSH_PROMISE: { @@ -870,6 +873,7 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, assert(promised_downstream); auto &promised_req = promised_downstream->request(); + auto &promised_balloc = promised_downstream->get_block_allocator(); // We use request header limit for PUSH_PROMISE if (promised_req.fs.buffer_size() + namelen + valuelen > @@ -886,9 +890,11 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, } auto token = http2::lookup_token(name, namelen); - promised_req.fs.add_header_token(StringRef{name, namelen}, - StringRef{value, valuelen}, - flags & NGHTTP2_NV_FLAG_NO_INDEX, token); + promised_req.fs.add_header_token( + make_string_ref(promised_balloc, StringRef{name, namelen}), + make_string_ref(promised_balloc, StringRef{value, valuelen}), + flags & NGHTTP2_NV_FLAG_NO_INDEX, token); + return 0; } } diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 5bfdb826..a3c78572 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -176,6 +176,8 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, auto &httpconf = get_config()->http; + auto &balloc = downstream->get_block_allocator(); + if (req.fs.buffer_size() + namelen + valuelen > httpconf.request_header_field_buffer || req.fs.num_fields() >= httpconf.max_request_header_fields) { @@ -206,12 +208,14 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, if (frame->headers.cat == NGHTTP2_HCAT_HEADERS) { // just store header fields for trailer part - req.fs.add_trailer_token(StringRef{name, namelen}, - StringRef{value, valuelen}, no_index, token); + req.fs.add_trailer_token( + make_string_ref(balloc, StringRef{name, namelen}), + make_string_ref(balloc, StringRef{value, valuelen}), no_index, token); return 0; } - req.fs.add_header_token(StringRef{name, namelen}, StringRef{value, valuelen}, + req.fs.add_header_token(make_string_ref(balloc, StringRef{name, namelen}), + make_string_ref(balloc, StringRef{value, valuelen}), no_index, token); return 0; } @@ -588,27 +592,29 @@ int on_frame_send_callback(nghttp2_session *session, const nghttp2_frame *frame, for (size_t i = 0; i < frame->push_promise.nvlen; ++i) { auto &nv = frame->push_promise.nva[i]; + + auto name = + make_string_ref(promised_balloc, StringRef{nv.name, nv.namelen}); + auto value = + make_string_ref(promised_balloc, StringRef{nv.value, nv.valuelen}); + auto token = http2::lookup_token(nv.name, nv.namelen); switch (token) { case http2::HD__METHOD: - req.method = http2::lookup_method_token(nv.value, nv.valuelen); + req.method = http2::lookup_method_token(value); break; case http2::HD__SCHEME: - req.scheme = - make_string_ref(promised_balloc, StringRef{nv.value, nv.valuelen}); + req.scheme = value; break; case http2::HD__AUTHORITY: - req.authority = - make_string_ref(promised_balloc, StringRef{nv.value, nv.valuelen}); + req.authority = value; break; case http2::HD__PATH: - req.path = http2::rewrite_clean_path(promised_balloc, - StringRef{nv.value, nv.valuelen}); + req.path = http2::rewrite_clean_path(promised_balloc, value); break; } - req.fs.add_header_token(StringRef{nv.name, nv.namelen}, - StringRef{nv.value, nv.valuelen}, - nv.flags & NGHTTP2_NV_FLAG_NO_INDEX, token); + req.fs.add_header_token(name, value, nv.flags & NGHTTP2_NV_FLAG_NO_INDEX, + token); } promised_downstream->inspect_http2_request(); diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index d7f5492b..5f4f9468 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -690,6 +690,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { auto downstream = static_cast(htp->data); auto &resp = downstream->response(); auto &httpconf = get_config()->http; + auto &balloc = downstream->get_block_allocator(); if (ensure_header_field_buffer(downstream, httpconf, len) != 0) { return -1; @@ -702,7 +703,9 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { if (ensure_max_header_fields(downstream, httpconf) != 0) { return -1; } - resp.fs.add_header_lower(StringRef{data, len}, StringRef{}, false); + auto name = http2::copy_lower(balloc, StringRef{data, len}); + auto token = http2::lookup_token(name); + resp.fs.add_header_token(name, StringRef{}, false, token); } } else { // trailer part @@ -715,7 +718,9 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { // wrong place or crash if trailer fields are currently empty. return -1; } - resp.fs.add_trailer_lower(StringRef(data, len), StringRef{}, false); + auto name = http2::copy_lower(balloc, StringRef{data, len}); + auto token = http2::lookup_token(name); + resp.fs.add_trailer_token(name, StringRef{}, false, token); } } return 0; diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 73e91a67..dd028190 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -121,6 +121,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { auto downstream = upstream->get_downstream(); auto &req = downstream->request(); auto &httpconf = get_config()->http; + auto &balloc = downstream->get_block_allocator(); if (req.fs.buffer_size() + len > httpconf.request_header_field_buffer) { if (LOG_ENABLED(INFO)) { @@ -145,7 +146,9 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { Downstream::HTTP1_REQUEST_HEADER_TOO_LARGE); return -1; } - req.fs.add_header_lower(StringRef{data, len}, StringRef{}, false); + auto name = http2::copy_lower(balloc, StringRef{data, len}); + auto token = http2::lookup_token(name); + req.fs.add_header_token(name, StringRef{}, false, token); } } else { // trailer part @@ -159,7 +162,9 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { } return -1; } - req.fs.add_trailer_lower(StringRef{data, len}, StringRef{}, false); + auto name = http2::copy_lower(balloc, StringRef{data, len}); + auto token = http2::lookup_token(name); + req.fs.add_trailer_token(name, StringRef{}, false, token); } } return 0; diff --git a/src/shrpx_mruby_module_request.cc b/src/shrpx_mruby_module_request.cc index bf83eba2..f98442a4 100644 --- a/src/shrpx_mruby_module_request.cc +++ b/src/shrpx_mruby_module_request.cc @@ -213,6 +213,7 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) { auto data = static_cast(mrb->ud); auto downstream = data->downstream; auto &req = downstream->request(); + auto &balloc = downstream->get_block_allocator(); check_phase(mrb, data->phase, PHASE_REQUEST); @@ -226,7 +227,8 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) { key = mrb_funcall(mrb, key, "downcase", 0); auto keyref = - StringRef{RSTRING_PTR(key), static_cast(RSTRING_LEN(key))}; + make_string_ref(balloc, StringRef{RSTRING_PTR(key), + static_cast(RSTRING_LEN(key))}); auto token = http2::lookup_token(keyref.byte(), keyref.size()); if (repl) { @@ -249,15 +251,19 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) { for (int i = 0; i < n; ++i) { auto value = mrb_ary_entry(values, i); req.fs.add_header_token( - keyref, StringRef{RSTRING_PTR(value), - static_cast(RSTRING_LEN(value))}, + keyref, + make_string_ref(balloc, + StringRef{RSTRING_PTR(value), + static_cast(RSTRING_LEN(value))}), false, token); } } else if (!mrb_nil_p(values)) { - req.fs.add_header_token(keyref, - StringRef{RSTRING_PTR(values), - static_cast(RSTRING_LEN(values))}, - false, token); + req.fs.add_header_token( + keyref, + make_string_ref(balloc, + StringRef{RSTRING_PTR(values), + static_cast(RSTRING_LEN(values))}), + false, token); } return mrb_nil_value(); diff --git a/src/shrpx_mruby_module_response.cc b/src/shrpx_mruby_module_response.cc index ada76b7c..65fec390 100644 --- a/src/shrpx_mruby_module_response.cc +++ b/src/shrpx_mruby_module_response.cc @@ -107,6 +107,7 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) { auto data = static_cast(mrb->ud); auto downstream = data->downstream; auto &resp = downstream->response(); + auto &balloc = downstream->get_block_allocator(); mrb_value key, values; mrb_get_args(mrb, "oo", &key, &values); @@ -118,7 +119,8 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) { key = mrb_funcall(mrb, key, "downcase", 0); auto keyref = - StringRef{RSTRING_PTR(key), static_cast(RSTRING_LEN(key))}; + make_string_ref(balloc, StringRef{RSTRING_PTR(key), + static_cast(RSTRING_LEN(key))}); auto token = http2::lookup_token(keyref.byte(), keyref.size()); if (repl) { @@ -141,14 +143,18 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) { for (int i = 0; i < n; ++i) { auto value = mrb_ary_entry(values, i); resp.fs.add_header_token( - keyref, StringRef{RSTRING_PTR(value), - static_cast(RSTRING_LEN(value))}, + keyref, + make_string_ref(balloc, + StringRef{RSTRING_PTR(value), + static_cast(RSTRING_LEN(value))}), false, token); } } else if (!mrb_nil_p(values)) { resp.fs.add_header_token( - keyref, StringRef{RSTRING_PTR(values), - static_cast(RSTRING_LEN(values))}, + keyref, + make_string_ref(balloc, + StringRef{RSTRING_PTR(values), + static_cast(RSTRING_LEN(values))}), false, token); } @@ -222,7 +228,6 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) { if (cl) { cl->value = content_length; } else { - // TODO we don't have to make a copy here. resp.fs.add_header_token(StringRef::from_lit("content-length"), content_length, false, http2::HD_CONTENT_LENGTH); } @@ -232,9 +237,10 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) { if (!date) { auto lgconf = log_config(); lgconf->update_tstamp(std::chrono::system_clock::now()); - resp.fs.add_header_token(StringRef::from_lit("date"), - StringRef{lgconf->time_http_str}, false, - http2::HD_DATE); + resp.fs.add_header_token( + StringRef::from_lit("date"), + make_string_ref(balloc, StringRef{lgconf->time_http_str}), false, + http2::HD_DATE); } auto upstream = downstream->get_upstream(); diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 89508ba8..4b48c93f 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -197,7 +197,9 @@ void on_ctrl_recv_callback(spdylay_session *session, spdylay_frame_type type, auto name = StringRef{nv[i]}; auto value = StringRef{nv[i + 1]}; auto token = http2::lookup_token(name.byte(), name.size()); - req.fs.add_header_token(name, value, false, token); + req.fs.add_header_token(make_string_ref(balloc, StringRef{name}), + make_string_ref(balloc, StringRef{value}), false, + token); } if (req.fs.parse_content_length() != 0) {