diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index f3807edd5285..c9bf39570cae 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}; -void validateCapacity(size_t new_capacity) { +uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { + return (static_cast(existing_capacity) + size_to_append) * 2; +} + +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(), ""); + RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), + "Trying to allocate overly large headers."); ASSERT(new_capacity >= MinDynamicCapacity); } @@ -86,9 +91,13 @@ 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)); - validateCapacity(dynamic_capacity_); + const uint64_t new_capacity = newCapacity(string_length_, size); + if (new_capacity > MinDynamicCapacity) { + validateCapacity(new_capacity); + dynamic_capacity_ = new_capacity; + } else { + dynamic_capacity_ = MinDynamicCapacity; + } char* buf = static_cast(malloc(dynamic_capacity_)); RELEASE_ASSERT(buf != nullptr, ""); memcpy(buf, buffer_.ref_, string_length_); @@ -97,7 +106,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; } @@ -108,7 +118,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 uint64_t new_capacity = newCapacity(string_length_, size); validateCapacity(new_capacity); buffer_.dynamic_ = static_cast(malloc(new_capacity)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); @@ -117,9 +127,11 @@ void HeaderString::append(const char* data, uint32_t size) { type_ = Type::Dynamic; } else { if (size + 1 + string_length_ > dynamic_capacity_) { + const uint64_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/source/common/ratelimit/ratelimit_impl.cc b/source/common/ratelimit/ratelimit_impl.cc index 9403838bfdfb..ab3a695b6724 100644 --- a/source/common/ratelimit/ratelimit_impl.cc +++ b/source/common/ratelimit/ratelimit_impl.cc @@ -103,6 +103,8 @@ GrpcFactoryImpl::GrpcFactoryImpl(const envoy::config::ratelimit::v2::RateLimitSe // TODO(junr03): legacy rate limit is deprecated. Remove this warning after 1.8.0. if (!use_data_plane_proto_) { + // Force link time dependency on deprecated message type. + pb::lyft::ratelimit::RateLimit _ignore; ENVOY_LOG_MISC(warn, "legacy rate limit client is deprecated, update your service to support " "the data-plane-api defined rate limit service"); } diff --git a/source/server/server.cc b/source/server/server.cc index 5d11b4c67216..76867df2bd98 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -97,6 +97,17 @@ InstanceImpl::~InstanceImpl() { // Stop logging to file before all the AccessLogManager and its dependencies are // destructed to avoid crashing at shutdown. file_logger_.reset(); + + // Destruct the ListenerManager explicitly, before InstanceImpl's local init_manager_ is + // destructed. + // + // The ListenerManager's DestinationPortsMap contains FilterChainSharedPtrs. There is a rare race + // condition where one of these FilterChains contains an HttpConnectionManager, which contains an + // RdsRouteConfigProvider, which contains an RdsRouteConfigSubscriptionSharedPtr. Since + // RdsRouteConfigSubscription is an Init::Target, ~RdsRouteConfigSubscription triggers a callback + // set at initialization, which goes to unregister it from the top-level InitManager, which has + // already been destructed (use-after-free) causing a segfault. + listener_manager_.reset(); } Upstream::ClusterManager& InstanceImpl::clusterManager() { return *config_->clusterManager(); } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index dc55e0d2e7ae..8a1df0f6369f 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -704,6 +704,19 @@ TEST(HeaderMapImplTest, TestAppendHeader) { } } +TEST(HeaderMapImplTest, TestHeaderLengthChecks) { + HeaderString value; + value.setCopy("some;", 5); + 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_LOG_TO_STDERR(reference.append(nullptr, std::numeric_limits::max()), + "Trying to allocate overly large headers."); +} + TEST(HeaderMapImplTest, PseudoHeaderOrder) { typedef testing::MockFunction MockCb; MockCb cb; diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 9cd76e4b877a..9c9048e6194e 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -162,6 +162,31 @@ TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariable) { testFormatting(request_info, "UPSTREAM_METADATA([\"namespace\", \"nested\", \"list_key\"])", ""); } +// 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; + 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'); + 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); + header->mutable_append()->set_value(true); + } + + HeaderParserPtr req_header_parser = HeaderParser::configure(to_add); + + Http::TestHeaderMapImpl header_map{{":method", "POST"}}; + NiceMock request_info; + EXPECT_DEATH_LOG_TO_STDERR(req_header_parser->evaluateHeaders(header_map, request_info), + "Trying to allocate overly large headers."); +} + TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariableMissingHost) { NiceMock request_info; std::shared_ptr> host; @@ -421,14 +446,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; } } @@ -448,10 +473,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) { @@ -470,15 +495,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) { @@ -497,11 +522,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) { @@ -541,7 +566,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)); @@ -565,34 +590,34 @@ match: { prefix: "/new_endpoint" } ON_CALL(request_info, perRequestState()).WillByDefault(ReturnRef(per_request_state)); ON_CALL(Const(request_info), perRequestState()).WillByDefault(ReturnRef(per_request_state)); - 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")); - EXPECT_TRUE(headerMap.has("x-per-request")); - EXPECT_EQ("test_value", headerMap.get_("x-per-request")); + EXPECT_TRUE(header_map.has("x-per-request")); + EXPECT_EQ("test_value", header_map.get_("x-per-request")); } TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { @@ -634,29 +659,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()}; @@ -711,29 +736,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 diff --git a/test/integration/BUILD b/test/integration/BUILD index 7d6409f32778..343bc1e86ac8 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -344,6 +344,8 @@ envoy_cc_test( srcs = [ "echo_integration_test.cc", ], + # This test must be run in exclusive mode: see comments in AddRemoveListener + tags = ["exclusive"], deps = [ ":integration_lib", "//source/extensions/filters/network/echo:config", diff --git a/test/integration/echo_integration_test.cc b/test/integration/echo_integration_test.cc index 342ba5866f0d..12445e7b51a0 100644 --- a/test/integration/echo_integration_test.cc +++ b/test/integration/echo_integration_test.cc @@ -118,10 +118,25 @@ TEST_P(EchoIntegrationTest, AddRemoveListener) { listener_removed.waitReady(); // Now connect. This should fail. - RawConnectionDriver connection2( - new_listener_port, buffer, - [&](Network::ClientConnection&, const Buffer::Instance&) -> void { FAIL(); }, version_); - connection2.run(); + // Allow for a few attempts, in order to handle a race (likely due to lack of + // LEV_OPT_CLOSE_ON_FREE, which would break listener reuse) + // + // In order for this test to work, it must be tagged as "exclusive" in its + // build file. Otherwise, it's possible that when the listener is destroyed + // above, another test would start listening on the released port, and this + // connect would unexpectedly succeed. + bool connect_fail = false; + for (int i = 0; i < 10; ++i) { + RawConnectionDriver connection2( + new_listener_port, buffer, + [&](Network::ClientConnection&, const Buffer::Instance&) -> void { FAIL(); }, version_); + connection2.run(Event::Dispatcher::RunType::NonBlock); + if (connection2.connection().state() == Network::Connection::State::Closed) { + connect_fail = true; + break; + } + } + ASSERT_TRUE(connect_fail); } } // namespace Envoy diff --git a/test/integration/utility.cc b/test/integration/utility.cc index f2440ff0893e..ff815315c653 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -122,7 +122,7 @@ RawConnectionDriver::RawConnectionDriver(uint32_t port, Buffer::Instance& initia RawConnectionDriver::~RawConnectionDriver() {} -void RawConnectionDriver::run() { dispatcher_->run(Event::Dispatcher::RunType::Block); } +void RawConnectionDriver::run(Event::Dispatcher::RunType run_type) { dispatcher_->run(run_type); } void RawConnectionDriver::close() { client_->close(Network::ConnectionCloseType::FlushWrite); } diff --git a/test/integration/utility.h b/test/integration/utility.h index a1e1d7c9e449..a1bd07f5ece9 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -61,7 +61,8 @@ class RawConnectionDriver { RawConnectionDriver(uint32_t port, Buffer::Instance& initial_data, ReadCallback data_callback, Network::Address::IpVersion version); ~RawConnectionDriver(); - void run(); + const Network::Connection& connection() { return *client_; } + void run(Event::Dispatcher::RunType run_type = Event::Dispatcher::RunType::Block); void close(); private: