From af5fd2019d88e313dde4963c3632c98129b50f42 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 12 Jul 2014 18:55:08 +0900 Subject: [PATCH] src: Remove 0x00 concatenation for headers Now concatenating header values with 0x00 as delimiter is not necessary because HPACK reference set is removed and the order of header field fed into HPACK encoder is preserved when they are decoded. --- src/HttpServer.cc | 10 ++--- src/http2.cc | 52 +++--------------------- src/http2.h | 22 ++++------ src/http2_test.cc | 30 +++----------- src/http2_test.h | 3 +- src/nghttp.cc | 10 ++--- src/shrpx-unittest.cc | 5 +-- src/shrpx_downstream.cc | 18 ++------ src/shrpx_downstream.h | 8 ---- src/shrpx_http2_downstream_connection.cc | 2 +- src/shrpx_http2_upstream.cc | 2 +- 11 files changed, 34 insertions(+), 128 deletions(-) diff --git a/src/HttpServer.cc b/src/HttpServer.cc index 13e3424f..fbf836a9 100644 --- a/src/HttpServer.cc +++ b/src/HttpServer.cc @@ -94,9 +94,9 @@ namespace { void append_nv(Stream *stream, const std::vector& nva) { for(auto& nv : nva) { - http2::split_add_header(stream->headers, - nv.name, nv.namelen, nv.value, nv.valuelen, - nv.flags & NGHTTP2_NV_FLAG_NO_INDEX); + http2::add_header(stream->headers, + nv.name, nv.namelen, nv.value, nv.valuelen, + nv.flags & NGHTTP2_NV_FLAG_NO_INDEX); } } } // namespace @@ -1158,8 +1158,8 @@ int on_header_callback(nghttp2_session *session, if(!http2::check_nv(name, namelen, value, valuelen)) { return 0; } - http2::split_add_header(stream->headers, name, namelen, value, valuelen, - flags & NGHTTP2_NV_FLAG_NO_INDEX); + http2::add_header(stream->headers, name, namelen, value, valuelen, + flags & NGHTTP2_NV_FLAG_NO_INDEX); return 0; } } // namespace diff --git a/src/http2.cc b/src/http2.cc index 8bb09c7f..4971f970 100644 --- a/src/http2.cc +++ b/src/http2.cc @@ -275,31 +275,12 @@ Headers::value_type to_header(const uint8_t *name, size_t namelen, no_index); } -void split_add_header(Headers& nva, - const uint8_t *name, size_t namelen, - const uint8_t *value, size_t valuelen, - bool no_index) +void add_header(Headers& nva, + const uint8_t *name, size_t namelen, + const uint8_t *value, size_t valuelen, + bool no_index) { - if(valuelen == 0) { - nva.push_back(to_header(name, namelen, value, valuelen, no_index)); - return; - } - auto j = value; - auto end = value + valuelen; - for(;;) { - // Skip 0 length value - j = std::find_if(j, end, - [](uint8_t c) - { - return c != '\0'; - }); - if(j == end) { - break; - } - auto l = std::find(j, end, '\0'); - nva.push_back(to_header(name, namelen, j, l-j, no_index)); - j = l; - } + nva.push_back(to_header(name, namelen, value, valuelen, no_index)); } const Headers::value_type* get_unique_header(const Headers& nva, @@ -355,29 +336,6 @@ nghttp2_nv make_nv(const std::string& name, const std::string& value, name.size(), value.size(), flags}; } -Headers concat_norm_headers(Headers headers) -{ - auto res = Headers(); - res.reserve(headers.size()); - for(auto& kv : headers) { - if(!res.empty() && res.back().name == kv.name && - kv.name != "cookie" && kv.name != "set-cookie") { - - auto& last = res.back(); - - if(!kv.value.empty()) { - last.value.append(1, '\0'); - last.value += kv.value; - } - // We do ORing nv flags. This is done even if value is empty. - last.no_index |= kv.no_index; - } else { - res.push_back(std::move(kv)); - } - } - return res; -} - void copy_norm_headers_to_nva (std::vector& nva, const Headers& headers) { diff --git a/src/http2.h b/src/http2.h index 39cb3013..579ac4a2 100644 --- a/src/http2.h +++ b/src/http2.h @@ -104,15 +104,13 @@ Headers::value_type to_header(const uint8_t *name, size_t namelen, const uint8_t *value, size_t valuelen, bool no_index); -// Add name/value pairs to |nva|. The name is given in the |name| with -// |namelen| bytes. This function inspects the |value| and split it -// using '\0' as delimiter. Each token is added to the |nva| with the -// name |name|. If |no_index| is true, this name/value pair won't be -// indexed when it is forwarded to the next hop. -void split_add_header(Headers& nva, - const uint8_t *name, size_t namelen, - const uint8_t *value, size_t valuelen, - bool no_index); +// Add name/value pairs to |nva|. If |no_index| is true, this +// name/value pair won't be indexed when it is forwarded to the next +// hop. +void add_header(Headers& nva, + const uint8_t *name, size_t namelen, + const uint8_t *value, size_t valuelen, + bool no_index); // Returns sorted |nva| with |nvlen| elements. The headers are sorted // by name only and not necessarily stable. In addition to the @@ -143,12 +141,6 @@ bool value_lws(const Headers::value_type *nv); // and not contain illegal characters. bool non_empty_value(const Headers::value_type *nv); -// Concatenates field with same value by NULL as delimiter and returns -// new vector containing the resulting header fields. cookie and -// set-cookie header fields won't be concatenated. This function -// assumes that the |headers| is sorted by name. -Headers concat_norm_headers(Headers headers); - // Creates nghttp2_nv using |name| and |value| and returns it. The // returned value only references the data pointer to name.c_str() and // value.c_str(). If |no_index| is true, nghttp2_nv flags member has diff --git a/src/http2_test.cc b/src/http2_test.cc index fa6dce8e..c2541548 100644 --- a/src/http2_test.cc +++ b/src/http2_test.cc @@ -72,27 +72,19 @@ void test_http2_sort_nva(void) check_nv({"delta", "5"}, &nva[5]); } -void test_http2_split_add_header(void) +void test_http2_add_header(void) { - const uint8_t concatval[] = { '4', 0x00, 0x00, '6', 0x00, '5', '9', 0x00 }; auto nva = Headers(); - http2::split_add_header(nva, (const uint8_t*)"delta", 5, - concatval, sizeof(concatval), false); - CU_ASSERT(Headers::value_type("delta", "4") == nva[0]); - CU_ASSERT(Headers::value_type("delta", "6") == nva[1]); - CU_ASSERT(Headers::value_type("delta", "59") == nva[2]); - nva.clear(); - - http2::split_add_header(nva, (const uint8_t*)"alpha", 5, - (const uint8_t*)"123", 3, false); + http2::add_header(nva, (const uint8_t*)"alpha", 5, + (const uint8_t*)"123", 3, false); CU_ASSERT(Headers::value_type("alpha", "123") == nva[0]); CU_ASSERT(!nva[0].no_index); nva.clear(); - http2::split_add_header(nva, (const uint8_t*)"alpha", 5, - (const uint8_t*)"", 0, true); + http2::add_header(nva, (const uint8_t*)"alpha", 5, + (const uint8_t*)"", 0, true); CU_ASSERT(Headers::value_type("alpha", "") == nva[0]); CU_ASSERT(nva[0].no_index); } @@ -199,18 +191,6 @@ auto headers = Headers {"zulu", "12"}}; } // namespace -void test_http2_concat_norm_headers(void) -{ - auto hds = headers; - hds.emplace_back("cookie", "foo"); - hds.emplace_back("cookie", "bar"); - hds.emplace_back("set-cookie", "baz"); - hds.emplace_back("set-cookie", "buzz"); - auto res = http2::concat_norm_headers(hds); - CU_ASSERT(14 == res.size()); - CU_ASSERT(std::string("2") + '\0' + std::string("3") == res[2].value); -} - void test_http2_copy_norm_headers_to_nva(void) { std::vector nva; diff --git a/src/http2_test.h b/src/http2_test.h index f2a2769f..b7a99c29 100644 --- a/src/http2_test.h +++ b/src/http2_test.h @@ -27,13 +27,12 @@ namespace shrpx { -void test_http2_split_add_header(void); +void test_http2_add_header(void); void test_http2_sort_nva(void); void test_http2_check_http2_headers(void); void test_http2_get_unique_header(void); void test_http2_get_header(void); void test_http2_value_lws(void); -void test_http2_concat_norm_headers(void); void test_http2_copy_norm_headers_to_nva(void); void test_http2_build_http1_headers_from_norm_headers(void); void test_http2_lws(void); diff --git a/src/nghttp.cc b/src/nghttp.cc index 58bb4ec1..7a7c2752 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -1048,8 +1048,6 @@ int submit_request return lhs.name < rhs.name; }); - build_headers = http2::concat_norm_headers(std::move(build_headers)); - auto nva = std::vector(); nva.reserve(build_headers.size()); @@ -1264,8 +1262,8 @@ int on_header_callback(nghttp2_session *session, if(!req) { break; } - http2::split_add_header(req->res_nva, name, namelen, value, valuelen, - flags & NGHTTP2_NV_FLAG_NO_INDEX); + http2::add_header(req->res_nva, name, namelen, value, valuelen, + flags & NGHTTP2_NV_FLAG_NO_INDEX); break; } case NGHTTP2_PUSH_PROMISE: { @@ -1274,8 +1272,8 @@ int on_header_callback(nghttp2_session *session, if(!req) { break; } - http2::split_add_header(req->push_req_nva, name, namelen, value, valuelen, - flags & NGHTTP2_NV_FLAG_NO_INDEX); + http2::add_header(req->push_req_nva, name, namelen, value, valuelen, + flags & NGHTTP2_NV_FLAG_NO_INDEX); break; } } diff --git a/src/shrpx-unittest.cc b/src/shrpx-unittest.cc index 6015ad4b..0d6f0a5f 100644 --- a/src/shrpx-unittest.cc +++ b/src/shrpx-unittest.cc @@ -71,8 +71,7 @@ int main(int argc, char* argv[]) shrpx::test_shrpx_ssl_create_lookup_tree) || !CU_add_test(pSuite, "ssl_cert_lookup_tree_add_cert_from_file", shrpx::test_shrpx_ssl_cert_lookup_tree_add_cert_from_file) || - !CU_add_test(pSuite, "http2_split_add_header", - shrpx::test_http2_split_add_header) || + !CU_add_test(pSuite, "http2_add_header", shrpx::test_http2_add_header) || !CU_add_test(pSuite, "http2_sort_nva", shrpx::test_http2_sort_nva) || !CU_add_test(pSuite, "http2_check_http2_headers", shrpx::test_http2_check_http2_headers) || @@ -82,8 +81,6 @@ int main(int argc, char* argv[]) shrpx::test_http2_get_header) || !CU_add_test(pSuite, "http2_value_lws", shrpx::test_http2_value_lws) || - !CU_add_test(pSuite, "http2_concat_norm_headers", - shrpx::test_http2_concat_norm_headers) || !CU_add_test(pSuite, "http2_copy_norm_headers_to_nva", shrpx::test_http2_copy_norm_headers_to_nva) || !CU_add_test(pSuite, "http2_build_http1_headers_from_norm_headers", diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index 6c1c26b5..64e155bf 100644 --- a/src/shrpx_downstream.cc +++ b/src/shrpx_downstream.cc @@ -230,11 +230,6 @@ Headers::const_iterator Downstream::get_norm_request_header return get_norm_header(request_headers_, name); } -void Downstream::concat_norm_request_headers() -{ - request_headers_ = http2::concat_norm_headers(std::move(request_headers_)); -} - void Downstream::add_request_header(std::string name, std::string value) { request_header_key_prev_ = true; @@ -256,8 +251,8 @@ void Downstream::split_add_request_header bool no_index) { request_headers_sum_ += namelen + valuelen; - http2::split_add_header(request_headers_, name, namelen, value, valuelen, - no_index); + http2::add_header(request_headers_, name, namelen, value, valuelen, + no_index); } bool Downstream::get_request_header_key_prev() const @@ -485,11 +480,6 @@ void Downstream::normalize_response_headers() http2::normalize_headers(response_headers_); } -void Downstream::concat_norm_response_headers() -{ - response_headers_ = http2::concat_norm_headers(std::move(response_headers_)); -} - Headers::const_iterator Downstream::get_norm_response_header (const std::string& name) const { @@ -551,8 +541,8 @@ void Downstream::split_add_response_header bool no_index) { response_headers_sum_ += namelen + valuelen; - http2::split_add_header(response_headers_, name, namelen, value, valuelen, - no_index); + http2::add_header(response_headers_, name, namelen, value, valuelen, + no_index); } bool Downstream::get_response_header_key_prev() const diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h index c69c53d3..82105ec3 100644 --- a/src/shrpx_downstream.h +++ b/src/shrpx_downstream.h @@ -99,10 +99,6 @@ public: // called after calling normalize_request_headers(). Headers::const_iterator get_norm_request_header (const std::string& name) const; - // Concatenates request header fields with same name by NULL as - // delimiter. See http2::concat_norm_headers(). This function must - // be called after calling normalize_request_headers(). - void concat_norm_request_headers(); void add_request_header(std::string name, std::string value); void set_last_request_header_value(std::string value); @@ -162,10 +158,6 @@ public: const Headers& get_response_headers() const; // Makes key lowercase and sort headers by name using < void normalize_response_headers(); - // Concatenates response header fields with same name by NULL as - // delimiter. See http2::concat_norm_headers(). This function must - // be called after calling normalize_response_headers(). - void concat_norm_response_headers(); // Returns iterator pointing to the response header with the name // |name|. If multiple header have |name| as name, return first // occurrence from the beginning. If no such header is found, diff --git a/src/shrpx_http2_downstream_connection.cc b/src/shrpx_http2_downstream_connection.cc index 91dba637..61c97441 100644 --- a/src/shrpx_http2_downstream_connection.cc +++ b/src/shrpx_http2_downstream_connection.cc @@ -244,7 +244,7 @@ int Http2DownstreamConnection::push_request_headers() downstream_->crumble_request_cookie(); } downstream_->normalize_request_headers(); - downstream_->concat_norm_request_headers(); + auto end_headers = std::end(downstream_->get_request_headers()); // 7 means: diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index a7c89a3b..647f645e 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -1161,7 +1161,7 @@ int Http2Upstream::on_downstream_header_complete(Downstream *downstream) downstream->rewrite_norm_location_response_header (get_client_handler()->get_upstream_scheme(), get_config()->port); } - downstream->concat_norm_response_headers(); + auto end_headers = std::end(downstream->get_response_headers()); size_t nheader = downstream->get_response_headers().size(); auto nva = std::vector();