From 176472ea9713bd00e66f1ba4e7e61a8f02d6f468 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 27 Aug 2018 15:16:25 -0400 Subject: [PATCH 1/6] one more OOM vector Signed-off-by: Alyssa Wilk --- source/common/http/header_map_impl.cc | 19 ++++++++++++----- test/common/router/header_formatter_test.cc | 23 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index f3807edd5285c..dd836c0d381c7 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -19,10 +19,15 @@ constexpr size_t MinDynamicCapacity{32}; // This includes the NULL (StringUtil::itoa technically only needs 21). constexpr size_t MaxIntegerLength{32}; +size_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { + return (static_cast(existing_capacity) + size_to_append) * 2; +} + void validateCapacity(size_t new_capacity) { // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421) - RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), ""); + RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), + "Trying to allocate overly large headers."); ASSERT(new_capacity >= MinDynamicCapacity); } @@ -88,7 +93,9 @@ void HeaderString::append(const char* data, uint32_t size) { type_ = Type::Dynamic; dynamic_capacity_ = std::max(MinDynamicCapacity, static_cast((string_length_ + size) * 2)); - validateCapacity(dynamic_capacity_); + if (dynamic_capacity_ != MinDynamicCapacity) { + validateCapacity(newCapacity(string_length_, size)); + } char* buf = static_cast(malloc(dynamic_capacity_)); RELEASE_ASSERT(buf != nullptr, ""); memcpy(buf, buffer_.ref_, string_length_); @@ -108,7 +115,7 @@ void HeaderString::append(const char* data, uint32_t size) { case Type::Dynamic: { // We can get here either because we didn't fit in inline or we are already dynamic. if (type_ == Type::Inline) { - const size_t new_capacity = (string_length_ + size) * 2; + const size_t new_capacity = newCapacity(string_length_, size); validateCapacity(new_capacity); buffer_.dynamic_ = static_cast(malloc(new_capacity)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); @@ -117,9 +124,11 @@ void HeaderString::append(const char* data, uint32_t size) { type_ = Type::Dynamic; } else { if (size + 1 + string_length_ > dynamic_capacity_) { + const size_t new_capacity = newCapacity(string_length_, size); + validateCapacity(new_capacity); + // Need to reallocate. - dynamic_capacity_ = (string_length_ + size) * 2; - validateCapacity(dynamic_capacity_); + dynamic_capacity_ = new_capacity; buffer_.dynamic_ = static_cast(realloc(buffer_.dynamic_, dynamic_capacity_)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); } diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index ceaad6c6b4835..b5e60fe6b6b8e 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -159,6 +159,29 @@ TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariable) { testFormatting(request_info, "UPSTREAM_METADATA([\"namespace\", \"nested\", \"list_key\"])", ""); } +TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) { + // This must be an inline header to get the append-in-place semantics. + const char* header_name = "connection"; + Protobuf::RepeatedPtrField to_add; + int loop_factor = 10; + uint64_t length = std::numeric_limits::max() / loop_factor; + std::string really_long_string(length + 1, 'a'); + std::cerr << "really long string " << really_long_string.size() << std::endl; + for (int i = 0; i < loop_factor; ++i) { + envoy::api::v2::core::HeaderValueOption* header = to_add.Add(); + header->mutable_header()->set_key(header_name); + header->mutable_header()->set_value(really_long_string); + header->mutable_append()->set_value(true); + } + + HeaderParserPtr req_header_parser = HeaderParser::configure(to_add); + + Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; + NiceMock request_info; + EXPECT_DEATH(req_header_parser->evaluateHeaders(headerMap, request_info), + "Trying to allocate overly large headers."); +} + TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariableMissingHost) { NiceMock request_info; std::shared_ptr> host; From f668193d4788da2f45cd5102dfc8c035fd82775e Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 27 Aug 2018 15:37:32 -0400 Subject: [PATCH 2/6] reviewer comments Signed-off-by: Alyssa Wilk --- source/common/http/header_map_impl.cc | 16 +-- test/common/router/header_formatter_test.cc | 131 ++++++++++---------- 2 files changed, 73 insertions(+), 74 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index dd836c0d381c7..e192438992ee3 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -19,11 +19,11 @@ constexpr size_t MinDynamicCapacity{32}; // This includes the NULL (StringUtil::itoa technically only needs 21). constexpr size_t MaxIntegerLength{32}; -size_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { - return (static_cast(existing_capacity) + size_to_append) * 2; +uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { + return (static_cast(existing_capacity) + size_to_append) * 2; } -void validateCapacity(size_t new_capacity) { +void validateCapacity(uint64_t new_capacity) { // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421) RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), @@ -91,10 +91,10 @@ void HeaderString::append(const char* data, uint32_t size) { // Rather than be too clever and optimize this uncommon case, we dynamically // allocate and copy. type_ = Type::Dynamic; - dynamic_capacity_ = - std::max(MinDynamicCapacity, static_cast((string_length_ + size) * 2)); + const uint64_t new_capacity = newCapacity(string_length_, size); + dynamic_capacity_ = std::max(MinDynamicCapacity, new_capacity); if (dynamic_capacity_ != MinDynamicCapacity) { - validateCapacity(newCapacity(string_length_, size)); + validateCapacity(new_capacity); } char* buf = static_cast(malloc(dynamic_capacity_)); RELEASE_ASSERT(buf != nullptr, ""); @@ -115,7 +115,7 @@ void HeaderString::append(const char* data, uint32_t size) { case Type::Dynamic: { // We can get here either because we didn't fit in inline or we are already dynamic. if (type_ == Type::Inline) { - const size_t new_capacity = newCapacity(string_length_, size); + const uint64_t new_capacity = newCapacity(string_length_, size); validateCapacity(new_capacity); buffer_.dynamic_ = static_cast(malloc(new_capacity)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); @@ -124,7 +124,7 @@ void HeaderString::append(const char* data, uint32_t size) { type_ = Type::Dynamic; } else { if (size + 1 + string_length_ > dynamic_capacity_) { - const size_t new_capacity = newCapacity(string_length_, size); + const uint64_t new_capacity = newCapacity(string_length_, size); validateCapacity(new_capacity); // Need to reallocate. diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index b5e60fe6b6b8e..e5baa8041e086 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -163,11 +163,10 @@ TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) { // This must be an inline header to get the append-in-place semantics. const char* header_name = "connection"; Protobuf::RepeatedPtrField to_add; - int loop_factor = 10; - uint64_t length = std::numeric_limits::max() / loop_factor; + const uint32_t num_header_chunks = 10; + const uint64_t length = std::numeric_limits::max() / num_header_chunks; std::string really_long_string(length + 1, 'a'); - std::cerr << "really long string " << really_long_string.size() << std::endl; - for (int i = 0; i < loop_factor; ++i) { + for (uint32_t i = 0; i < num_header_chunks; ++i) { envoy::api::v2::core::HeaderValueOption* header = to_add.Add(); header->mutable_header()->set_key(header_name); header->mutable_header()->set_value(really_long_string); @@ -176,9 +175,9 @@ TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) { HeaderParserPtr req_header_parser = HeaderParser::configure(to_add); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; NiceMock request_info; - EXPECT_DEATH(req_header_parser->evaluateHeaders(headerMap, request_info), + EXPECT_DEATH(req_header_parser->evaluateHeaders(header_map, request_info), "Trying to allocate overly large headers."); } @@ -383,14 +382,14 @@ TEST(HeaderParserTest, TestParseInternal) { HeaderParserPtr req_header_parser = HeaderParser::configure(to_add); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; - req_header_parser->evaluateHeaders(headerMap, request_info); + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; + req_header_parser->evaluateHeaders(header_map, request_info); std::string descriptor = fmt::format("for test case input: {}", test_case.input_); - EXPECT_TRUE(headerMap.has("x-header")) << descriptor; + EXPECT_TRUE(header_map.has("x-header")) << descriptor; EXPECT_TRUE(test_case.expected_output_) << descriptor; - EXPECT_EQ(test_case.expected_output_.value(), headerMap.get_("x-header")) << descriptor; + EXPECT_EQ(test_case.expected_output_.value(), header_map.get_("x-header")) << descriptor; } } @@ -410,10 +409,10 @@ TEST(HeaderParserTest, EvaluateHeaders) { )EOF"; HeaderParserPtr req_header_parser = HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add()); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; NiceMock request_info; - req_header_parser->evaluateHeaders(headerMap, request_info); - EXPECT_TRUE(headerMap.has("x-client-ip")); + req_header_parser->evaluateHeaders(header_map, request_info); + EXPECT_TRUE(header_map.has("x-client-ip")); } TEST(HeaderParserTest, EvaluateEmptyHeaders) { @@ -432,15 +431,15 @@ TEST(HeaderParserTest, EvaluateEmptyHeaders) { )EOF"; HeaderParserPtr req_header_parser = HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add()); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; std::shared_ptr> host( new NiceMock()); NiceMock request_info; auto metadata = std::make_shared(); ON_CALL(request_info, upstreamHost()).WillByDefault(Return(host)); ON_CALL(*host, metadata()).WillByDefault(Return(metadata)); - req_header_parser->evaluateHeaders(headerMap, request_info); - EXPECT_FALSE(headerMap.has("x-key")); + req_header_parser->evaluateHeaders(header_map, request_info); + EXPECT_FALSE(header_map.has("x-key")); } TEST(HeaderParserTest, EvaluateStaticHeaders) { @@ -459,11 +458,11 @@ TEST(HeaderParserTest, EvaluateStaticHeaders) { )EOF"; HeaderParserPtr req_header_parser = HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add()); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; NiceMock request_info; - req_header_parser->evaluateHeaders(headerMap, request_info); - EXPECT_TRUE(headerMap.has("static-header")); - EXPECT_EQ("static-value", headerMap.get_("static-header")); + req_header_parser->evaluateHeaders(header_map, request_info); + EXPECT_TRUE(header_map.has("static-header")); + EXPECT_EQ("static-value", header_map.get_("static-header")); } TEST(HeaderParserTest, EvaluateCompoundHeaders) { @@ -500,7 +499,7 @@ match: { prefix: "/new_endpoint" } HeaderParserPtr req_header_parser = HeaderParser::configure(parseRouteFromV2Yaml(yaml).route().request_headers_to_add()); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}}; + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; NiceMock request_info; absl::optional protocol = Envoy::Http::Protocol::Http11; ON_CALL(request_info, protocol()).WillByDefault(ReturnPointee(&protocol)); @@ -519,31 +518,31 @@ match: { prefix: "/new_endpoint" } )EOF")); ON_CALL(*host, metadata()).WillByDefault(Return(metadata)); - req_header_parser->evaluateHeaders(headerMap, request_info); + req_header_parser->evaluateHeaders(header_map, request_info); - EXPECT_TRUE(headerMap.has("x-prefix")); - EXPECT_EQ("prefix-127.0.0.1", headerMap.get_("x-prefix")); + EXPECT_TRUE(header_map.has("x-prefix")); + EXPECT_EQ("prefix-127.0.0.1", header_map.get_("x-prefix")); - EXPECT_TRUE(headerMap.has("x-suffix")); - EXPECT_EQ("127.0.0.1-suffix", headerMap.get_("x-suffix")); + EXPECT_TRUE(header_map.has("x-suffix")); + EXPECT_EQ("127.0.0.1-suffix", header_map.get_("x-suffix")); - EXPECT_TRUE(headerMap.has("x-both")); - EXPECT_EQ("prefix-127.0.0.1-suffix", headerMap.get_("x-both")); + EXPECT_TRUE(header_map.has("x-both")); + EXPECT_EQ("prefix-127.0.0.1-suffix", header_map.get_("x-both")); - EXPECT_TRUE(headerMap.has("x-escaping-1")); - EXPECT_EQ("%127.0.0.1%", headerMap.get_("x-escaping-1")); + EXPECT_TRUE(header_map.has("x-escaping-1")); + EXPECT_EQ("%127.0.0.1%", header_map.get_("x-escaping-1")); - EXPECT_TRUE(headerMap.has("x-escaping-2")); - EXPECT_EQ("%%%", headerMap.get_("x-escaping-2")); + EXPECT_TRUE(header_map.has("x-escaping-2")); + EXPECT_EQ("%%%", header_map.get_("x-escaping-2")); - EXPECT_TRUE(headerMap.has("x-multi")); - EXPECT_EQ("HTTP/1.1 from 127.0.0.1", headerMap.get_("x-multi")); + EXPECT_TRUE(header_map.has("x-multi")); + EXPECT_EQ("HTTP/1.1 from 127.0.0.1", header_map.get_("x-multi")); - EXPECT_TRUE(headerMap.has("x-multi-back-to-back")); - EXPECT_EQ("HTTP/1.1127.0.0.1", headerMap.get_("x-multi-back-to-back")); + EXPECT_TRUE(header_map.has("x-multi-back-to-back")); + EXPECT_EQ("HTTP/1.1127.0.0.1", header_map.get_("x-multi-back-to-back")); - EXPECT_TRUE(headerMap.has("x-metadata")); - EXPECT_EQ("value", headerMap.get_("x-metadata")); + EXPECT_TRUE(header_map.has("x-metadata")); + EXPECT_EQ("value", header_map.get_("x-metadata")); } TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { @@ -585,29 +584,29 @@ TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { HeaderParserPtr req_header_parser = Router::HeaderParser::configure(route_action.request_headers_to_add()); - Http::TestHeaderMapImpl headerMap{ + Http::TestHeaderMapImpl header_map{ {":method", "POST"}, {"static-header", "old-value"}, {"x-client-ip", "0.0.0.0"}}; NiceMock request_info; const SystemTime start_time(std::chrono::microseconds(1522796769123456)); EXPECT_CALL(request_info, startTime()).Times(3).WillRepeatedly(Return(start_time)); - req_header_parser->evaluateHeaders(headerMap, request_info); - EXPECT_TRUE(headerMap.has("static-header")); - EXPECT_EQ("static-value", headerMap.get_("static-header")); - EXPECT_TRUE(headerMap.has("x-client-ip")); - EXPECT_EQ("127.0.0.1", headerMap.get_("x-client-ip")); - EXPECT_TRUE(headerMap.has("x-request-start")); - EXPECT_EQ("1522796769123", headerMap.get_("x-request-start")); - EXPECT_TRUE(headerMap.has("x-request-start-default")); - EXPECT_EQ("2018-04-03T23:06:09.123Z", headerMap.get_("x-request-start-default")); - EXPECT_TRUE(headerMap.has("x-request-start-range")); + req_header_parser->evaluateHeaders(header_map, request_info); + EXPECT_TRUE(header_map.has("static-header")); + EXPECT_EQ("static-value", header_map.get_("static-header")); + EXPECT_TRUE(header_map.has("x-client-ip")); + EXPECT_EQ("127.0.0.1", header_map.get_("x-client-ip")); + EXPECT_TRUE(header_map.has("x-request-start")); + EXPECT_EQ("1522796769123", header_map.get_("x-request-start")); + EXPECT_TRUE(header_map.has("x-request-start-default")); + EXPECT_EQ("2018-04-03T23:06:09.123Z", header_map.get_("x-request-start-default")); + EXPECT_TRUE(header_map.has("x-request-start-range")); EXPECT_EQ("123456000, 1, 12, 123, 1234, 12345, 123456, 1234560, 12345600, 123456000", - headerMap.get_("x-request-start-range")); + header_map.get_("x-request-start-range")); typedef std::map CountMap; CountMap counts; - headerMap.iterate( + header_map.iterate( [](const Http::HeaderEntry& header, void* cb_v) -> Http::HeaderMap::Iterate { CountMap* m = static_cast(cb_v); std::string key = std::string{header.key().c_str()}; @@ -662,29 +661,29 @@ match: { prefix: "/new_endpoint" } const auto route = parseRouteFromV2Yaml(yaml).route(); HeaderParserPtr resp_header_parser = HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); - Http::TestHeaderMapImpl headerMap{{":method", "POST"}, {"x-safe", "safe"}, {"x-nope", "nope"}}; + Http::TestHeaderMapImpl header_map{{":method", "POST"}, {"x-safe", "safe"}, {"x-nope", "nope"}}; NiceMock request_info; // Initialize start_time as 2018-04-03T23:06:09.123Z in microseconds. const SystemTime start_time(std::chrono::microseconds(1522796769123456)); EXPECT_CALL(request_info, startTime()).Times(7).WillRepeatedly(Return(start_time)); - resp_header_parser->evaluateHeaders(headerMap, request_info); - EXPECT_TRUE(headerMap.has("x-client-ip")); - EXPECT_TRUE(headerMap.has("x-request-start-multiple")); - EXPECT_TRUE(headerMap.has("x-safe")); - EXPECT_FALSE(headerMap.has("x-nope")); - EXPECT_TRUE(headerMap.has("x-request-start")); - EXPECT_EQ("1522796769.123", headerMap.get_("x-request-start")); + resp_header_parser->evaluateHeaders(header_map, request_info); + EXPECT_TRUE(header_map.has("x-client-ip")); + EXPECT_TRUE(header_map.has("x-request-start-multiple")); + EXPECT_TRUE(header_map.has("x-safe")); + EXPECT_FALSE(header_map.has("x-nope")); + EXPECT_TRUE(header_map.has("x-request-start")); + EXPECT_EQ("1522796769.123", header_map.get_("x-request-start")); EXPECT_EQ("1522796769.123 2018-04-03T23:06:09.123Z 1522796769", - headerMap.get_("x-request-start-multiple")); - EXPECT_TRUE(headerMap.has("x-request-start-f")); - EXPECT_EQ("f", headerMap.get_("x-request-start-f")); - EXPECT_TRUE(headerMap.has("x-request-start-default")); - EXPECT_EQ("2018-04-03T23:06:09.123Z", headerMap.get_("x-request-start-default")); - EXPECT_TRUE(headerMap.has("x-request-start-range")); + header_map.get_("x-request-start-multiple")); + EXPECT_TRUE(header_map.has("x-request-start-f")); + EXPECT_EQ("f", header_map.get_("x-request-start-f")); + EXPECT_TRUE(header_map.has("x-request-start-default")); + EXPECT_EQ("2018-04-03T23:06:09.123Z", header_map.get_("x-request-start-default")); + EXPECT_TRUE(header_map.has("x-request-start-range")); EXPECT_EQ("123456000, 1, 12, 123, 1234, 12345, 123456, 1234560, 12345600, 123456000", - headerMap.get_("x-request-start-range")); + header_map.get_("x-request-start-range")); } } // namespace Router From f39ec4c9867bd4c7fa2f0138a02632d704f0ce9f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 27 Aug 2018 15:56:54 -0400 Subject: [PATCH 3/6] Hopefully unbreaking the mac build. will write test tomorrow Signed-off-by: Alyssa Wilk --- source/common/http/header_map_impl.cc | 10 +++++++--- test/common/http/header_map_impl_test.cc | 7 +++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index e192438992ee3..98097cc976ee1 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -92,9 +92,12 @@ void HeaderString::append(const char* data, uint32_t size) { // allocate and copy. type_ = Type::Dynamic; const uint64_t new_capacity = newCapacity(string_length_, size); - dynamic_capacity_ = std::max(MinDynamicCapacity, new_capacity); - if (dynamic_capacity_ != MinDynamicCapacity) { + if (new_capacity > MinDynamicCapacity) { + // TODO(alyssawilk) unit test. validateCapacity(new_capacity); + dynamic_capacity_ = new_capacity; + } else { + dynamic_capacity_ = MinDynamicCapacity; } char* buf = static_cast(malloc(dynamic_capacity_)); RELEASE_ASSERT(buf != nullptr, ""); @@ -104,7 +107,8 @@ void HeaderString::append(const char* data, uint32_t size) { } case Type::Inline: { - if (size + 1 + string_length_ <= sizeof(inline_buffer_)) { + const uint64_t new_capacity = static_cast(size) + 1 + string_length_; + if (new_capacity <= sizeof(inline_buffer_)) { // Already inline and the new value fits in inline storage. break; } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index dc55e0d2e7ae0..70c16ae507e86 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -704,6 +704,13 @@ TEST(HeaderMapImplTest, TestAppendHeader) { } } +TEST(HeaderMapImplTest, TestHeaderLengthChecks) { + HeaderString value; + value.setCopy("some;", 5); + EXPECT_DEATH(value.append(nullptr, std::numeric_limits::max()), + "Trying to allocate overly large headers."); +} + TEST(HeaderMapImplTest, PseudoHeaderOrder) { typedef testing::MockFunction MockCb; MockCb cb; From 8745ebd276171b6139936ac113e2108a0752525b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 28 Aug 2018 11:37:55 -0400 Subject: [PATCH 4/6] one more test, fixing EXPECT_DEATH macro for asan/tsan Signed-off-by: Alyssa Wilk --- source/common/http/header_map_impl.cc | 1 - test/common/http/header_map_impl_test.cc | 6 ++++++ test/common/router/header_formatter_test.cc | 4 ++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 98097cc976ee1..c9bf39570cae2 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -93,7 +93,6 @@ void HeaderString::append(const char* data, uint32_t size) { type_ = Type::Dynamic; const uint64_t new_capacity = newCapacity(string_length_, size); if (new_capacity > MinDynamicCapacity) { - // TODO(alyssawilk) unit test. validateCapacity(new_capacity); dynamic_capacity_ = new_capacity; } else { diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 70c16ae507e86..c8e09518643c9 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -709,6 +709,12 @@ TEST(HeaderMapImplTest, TestHeaderLengthChecks) { value.setCopy("some;", 5); EXPECT_DEATH(value.append(nullptr, std::numeric_limits::max()), "Trying to allocate overly large headers."); + + std::string source("hello"); + HeaderString reference; + reference.setReference(source); + EXPECT_DEATH(reference.append(nullptr, std::numeric_limits::max()), + "Trying to allocate overly large headers."); } TEST(HeaderMapImplTest, PseudoHeaderOrder) { diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index e5baa8041e086..b0b280de1f91b 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -177,8 +177,8 @@ TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) { Http::TestHeaderMapImpl header_map{{":method", "POST"}}; NiceMock request_info; - EXPECT_DEATH(req_header_parser->evaluateHeaders(header_map, request_info), - "Trying to allocate overly large headers."); + EXPECT_DEATH_LOG_TO_STDERR(req_header_parser->evaluateHeaders(header_map, request_info), + "Trying to allocate overly large headers."); } TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariableMissingHost) { From 580e760e24e6cc1e9167f4123cf83a8afa792333 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 28 Aug 2018 12:33:32 -0400 Subject: [PATCH 5/6] disabling proof of concept test Signed-off-by: Alyssa Wilk --- test/common/router/header_formatter_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index b0b280de1f91b..5bda07d18e0ea 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -159,7 +159,10 @@ TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariable) { testFormatting(request_info, "UPSTREAM_METADATA([\"namespace\", \"nested\", \"list_key\"])", ""); } -TEST_F(RequestInfoHeaderFormatterTest, UserDefinedHeadersConsideredHarmful) { +// Breaks tsan/asan builds by trying to allocate a lot of memory. +// Works on debug builds and needs to be fixed. See +// https://github.com/envoyproxy/envoy/issues/4268 +TEST_F(RequestInfoHeaderFormatterTest, DISABLED_UserDefinedHeadersConsideredHarmful) { // This must be an inline header to get the append-in-place semantics. const char* header_name = "connection"; Protobuf::RepeatedPtrField to_add; From d3ffa9d7929aa54cc2200813f5ab988f2f3da3a2 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 28 Aug 2018 13:57:13 -0400 Subject: [PATCH 6/6] trying to fix coverage build Signed-off-by: Alyssa Wilk --- test/common/http/header_map_impl_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index c8e09518643c9..8a1df0f6369f4 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -707,14 +707,14 @@ TEST(HeaderMapImplTest, TestAppendHeader) { TEST(HeaderMapImplTest, TestHeaderLengthChecks) { HeaderString value; value.setCopy("some;", 5); - EXPECT_DEATH(value.append(nullptr, std::numeric_limits::max()), - "Trying to allocate overly large headers."); + EXPECT_DEATH_LOG_TO_STDERR(value.append(nullptr, std::numeric_limits::max()), + "Trying to allocate overly large headers."); std::string source("hello"); HeaderString reference; reference.setReference(source); - EXPECT_DEATH(reference.append(nullptr, std::numeric_limits::max()), - "Trying to allocate overly large headers."); + EXPECT_DEATH_LOG_TO_STDERR(reference.append(nullptr, std::numeric_limits::max()), + "Trying to allocate overly large headers."); } TEST(HeaderMapImplTest, PseudoHeaderOrder) {