From 6f83b584aaf80e1c5e6f9681e73f3ad9767e3db3 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 20 Aug 2018 18:20:38 -0400 Subject: [PATCH 1/3] http: support appending to prefined inline headers. Previously we just ASSERTed when performing an addReferenceKey() with a predefined header. This PR adds support for comma concatenation and adjusts docs to make clear the contract for value lifetime across the API surface. Fixes #3919. Also resolves ClusterFuzz issue https://oss-fuzz.com/v2/testcase-detail/5107723602493440?noredirect=1. Risk level: High. This affects header processing and wire representation of predefined headers when appending. Testing: Unit/integration tests, corpus entry added. Signed-off-by: Harvey Tuch --- docs/root/intro/version_history.rst | 4 ++ include/envoy/http/header_map.h | 28 ++++++---- source/common/http/header_map_impl.cc | 8 ++- test/common/http/header_map_impl_test.cc | 4 +- ...e-header_parser_fuzz_test-5107723602493440 | 14 +++++ test/integration/header_integration_test.cc | 55 +++++++++++++++++++ 6 files changed, 98 insertions(+), 15 deletions(-) create mode 100644 test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index bb4661c3d10fe..4e9c9fd9ba2c4 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -38,6 +38,10 @@ Version history * http: added generic :ref:`Upgrade support `. * http: better handling of HEAD requests. Now sending transfer-encoding: chunked rather than content-length: 0. +* http: fixed missing support for appending to predefined inline headers, e.g. + *authorization*, in features that interact with request and response headers, + e.g. :ref:`request_headers_to_add + `. * http: response filters not applied to early error paths such as http_parser generated 400s. * http: :ref:`hpack_table_size ` now controls dynamic table size of both: encoder and decoder. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index efbf38638eb25..3e158dc608aa5 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -321,10 +321,12 @@ class HeaderMap { /** * Add a reference header to the map. Both key and value MUST point to data that will live beyond * the lifetime of any request/response using the string (since a codec may optimize for zero - * copy). Nothing will be copied. + * copy). The key will not be copied and a best effort will be made not to + * copy the value (but this may happen when comma concatenating, see below). * - * Calling addReference multiple times for the same header will result in multiple headers being - * present in the HeaderMap. + * Calling addReference multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL NOT be copied. @@ -336,8 +338,9 @@ class HeaderMap { * the lifetime of any request/response using the string (since a codec may optimize for zero * copy). The value will be copied. * - * Calling addReferenceKey multiple times for the same header will result in multiple headers - * being present in the HeaderMap. + * Calling addReference multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL be copied. @@ -349,8 +352,9 @@ class HeaderMap { * live beyond the lifetime of any request/response using the string (since a codec may optimize * for zero copy). The value will be copied. * - * Calling addReferenceKey multiple times for the same header will result in multiple headers - * being present in the HeaderMap. + * Calling addReference multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL be copied. @@ -360,8 +364,9 @@ class HeaderMap { /** * Add a header by copying both the header key and the value. * - * Calling addCopy multiple times for the same header will result in multiple headers being - * present in the HeaderMap. + * Calling addCopy multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL be copied. * @param value specifies the value of the header to add; it WILL be copied. @@ -371,8 +376,9 @@ class HeaderMap { /** * Add a header by copying both the header key and the value. * - * Calling addCopy multiple times for the same header will result in multiple headers being - * present in the HeaderMap. + * Calling addCopy multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL be copied. * @param value specifies the value of the header to add; it WILL be copied. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 28728515ef12e..13d9bd257dbda 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -323,8 +323,12 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { if (cb) { key.clear(); StaticLookupResponse ref_lookup_response = cb(*this); - ASSERT(*ref_lookup_response.entry_ == nullptr); // This function doesn't handle append. - maybeCreateInline(ref_lookup_response.entry_, *ref_lookup_response.key_, std::move(value)); + if (*ref_lookup_response.entry_ == nullptr) { + maybeCreateInline(ref_lookup_response.entry_, *ref_lookup_response.key_, std::move(value)); + } else { + appendToHeader((*ref_lookup_response.entry_)->value(), value.c_str()); + value.clear(); + } } else { std::list::iterator i = headers_.insert(std::move(key), std::move(value)); i->entry_ = i; diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 7d8bb9413ece3..9163dd1116f71 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -427,8 +427,8 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { TEST(HeaderMapImplTest, DoubleInlineAdd) { HeaderMapImpl headers; headers.addReferenceKey(Headers::get().ContentLength, 5); - EXPECT_DEBUG_DEATH(headers.addReferenceKey(Headers::get().ContentLength, 6), ""); - EXPECT_STREQ("5", headers.ContentLength()->value().c_str()); + headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_STREQ("5,6", headers.ContentLength()->value().c_str()); EXPECT_EQ(1UL, headers.size()); } diff --git a/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 b/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 new file mode 100644 index 0000000000000..1866c52aa3098 --- /dev/null +++ b/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 @@ -0,0 +1,14 @@ +headers_to_add { +} +headers_to_add { + header { + key: "te" + value: "l" + } +} +headers_to_add { + header { + key: "te" + value: "@" + } +} diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index e25e7b79a4f62..29eaf276d9716 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -120,6 +120,26 @@ stat_prefix: header_test - header: key: "x-real-ip" value: "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%" + - name: append-same-headers + domains: ["append-same-headers.com"] + request_headers_to_add: + - header: + key: "x-foo" + value: "value1" + - header: + key: "authorization" + value: "token1" + routes: + - match: { prefix: "/test" } + route: + cluster: cluster_0 + request_headers_to_add: + - header: + key: "x-foo" + value: "value2" + - header: + key: "authorization" + value: "token2" )EOF"; } // namespace @@ -915,4 +935,39 @@ TEST_P(HeaderIntegrationTest, TestXFFParsing) { }); } +// Validates behavior around same header appending (both predefined headers and +// other). +TEST_P(HeaderIntegrationTest, TestAppendSameHeaders) { + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/test"}, + {":scheme", "http"}, + {":authority", "append-same-headers.com"}, + {"authorization", "token3"}, + {"x-foo", "value3"}, + }, + Http::TestHeaderMapImpl{ + {":authority", "append-same-headers.com"}, + {":path", "/test"}, + {":method", "GET"}, + {"authorization", "token3,token2,token1"}, + {"x-foo", "value3"}, + {"x-foo", "value2"}, + {"x-foo", "value1"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} + } // namespace Envoy From 243ceac7111aa0d19a2336c0f017c1594a5d186d Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 21 Aug 2018 13:20:51 -0400 Subject: [PATCH 2/3] Review feedback. Signed-off-by: Harvey Tuch --- docs/root/intro/version_history.rst | 6 +++- source/common/http/header_map_impl.cc | 18 +++++++----- test/common/http/header_map_impl_test.cc | 35 ++++++++++++++++++++---- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 4e9c9fd9ba2c4..0d176c3b8e454 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -41,7 +41,11 @@ Version history * http: fixed missing support for appending to predefined inline headers, e.g. *authorization*, in features that interact with request and response headers, e.g. :ref:`request_headers_to_add - `. + `. For example, a + request header *authorization: token1* will appear as *authorization: + token1,token2*, after having :ref:`request_headers_to_add + ` with *authorization: + token2* applied. * http: response filters not applied to early error paths such as http_parser generated 400s. * http: :ref:`hpack_table_size ` now controls dynamic table size of both: encoder and decoder. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 13d9bd257dbda..7f17f1e24b600 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -67,13 +67,15 @@ void HeaderString::freeDynamic() { void HeaderString::append(const char* data, uint32_t size) { switch (type_) { case Type::Reference: { - // Switch back to inline and fall through. We do not actually append to the static string - // currently which would require a copy. - type_ = Type::Inline; - buffer_.dynamic_ = inline_buffer_; - string_length_ = 0; - - FALLTHRU; + // Rather than be too clever and optimize this uncommon case, we dynamically + // allocate and copy. + type_ = Type::Dynamic; + dynamic_capacity_ = (string_length_ + size) * 2; + char* buf = static_cast(malloc(dynamic_capacity_)); + RELEASE_ASSERT(buf != nullptr, ""); + memcpy(buf, buffer_.ref_, string_length_); + buffer_.dynamic_ = buf; + break; } case Type::Inline: { @@ -94,6 +96,7 @@ void HeaderString::append(const char* data, uint32_t size) { RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), ""); buffer_.dynamic_ = static_cast(malloc(new_capacity)); memcpy(buffer_.dynamic_, inline_buffer_, string_length_); + RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); dynamic_capacity_ = new_capacity; type_ = Type::Dynamic; } else { @@ -101,6 +104,7 @@ void HeaderString::append(const char* data, uint32_t size) { // Need to reallocate. dynamic_capacity_ = (string_length_ + size) * 2; buffer_.dynamic_ = static_cast(realloc(buffer_.dynamic_, dynamic_capacity_)); + RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); } } } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 9163dd1116f71..7dd2e1251a8d3 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -110,7 +110,7 @@ TEST(HeaderStringTest, All) { HeaderString string(static_string); EXPECT_EQ(HeaderString::Type::Reference, string.type()); string.append("a", 1); - EXPECT_STREQ("a", string.c_str()); + EXPECT_STREQ("HELLOa", string.c_str()); } // Copy inline @@ -425,11 +425,34 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { } TEST(HeaderMapImplTest, DoubleInlineAdd) { - HeaderMapImpl headers; - headers.addReferenceKey(Headers::get().ContentLength, 5); - headers.addReferenceKey(Headers::get().ContentLength, 6); - EXPECT_STREQ("5,6", headers.ContentLength()->value().c_str()); - EXPECT_EQ(1UL, headers.size()); + { + HeaderMapImpl headers; + headers.addReference(Headers::get().ContentLength, "foo"); + headers.addReference(Headers::get().ContentLength, "bar"); + EXPECT_STREQ("foo,bar", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } + { + HeaderMapImpl headers; + headers.addReferenceKey(Headers::get().ContentLength, "foo"); + headers.addReferenceKey(Headers::get().ContentLength, "bar"); + EXPECT_STREQ("foo,bar", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } + { + HeaderMapImpl headers; + headers.addReferenceKey(Headers::get().ContentLength, 5); + headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_STREQ("5,6", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } + { + HeaderMapImpl headers; + headers.addReference(Headers::get().ContentLength, "foo"); + headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_STREQ("foo,6", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } } TEST(HeaderMapImplTest, DoubleInlineSet) { From 395649076f59edd15a008a2f39c7b899e50e5fa1 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 21 Aug 2018 17:57:30 -0400 Subject: [PATCH 3/3] Fix ASAN. Signed-off-by: Harvey Tuch --- test/common/http/header_map_impl_test.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 7dd2e1251a8d3..326b954002c09 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -427,8 +427,10 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { TEST(HeaderMapImplTest, DoubleInlineAdd) { { HeaderMapImpl headers; - headers.addReference(Headers::get().ContentLength, "foo"); - headers.addReference(Headers::get().ContentLength, "bar"); + const std::string foo("foo"); + const std::string bar("bar"); + headers.addReference(Headers::get().ContentLength, foo); + headers.addReference(Headers::get().ContentLength, bar); EXPECT_STREQ("foo,bar", headers.ContentLength()->value().c_str()); EXPECT_EQ(1UL, headers.size()); } @@ -448,7 +450,8 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { } { HeaderMapImpl headers; - headers.addReference(Headers::get().ContentLength, "foo"); + const std::string foo("foo"); + headers.addReference(Headers::get().ContentLength, foo); headers.addReferenceKey(Headers::get().ContentLength, 6); EXPECT_STREQ("foo,6", headers.ContentLength()->value().c_str()); EXPECT_EQ(1UL, headers.size());