From b1540ba2a386472ac0a91d94064dfd3dfe02bff7 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Tue, 10 Nov 2020 16:59:55 -0500 Subject: [PATCH] Prevent SEGFAULT when disabling listener (#13515) (#13903) This prevents the stop_listening overload action from causing segmentation faults that can occur if the action is enabled after the listener has already shut down. Signed-off-by: Alex Konradi Signed-off-by: Christoph Pakulski --- docs/root/version_history/current.rst | 1 + source/common/common/utility.cc | 2 +- source/common/http/utility.cc | 2 +- source/common/router/retry_state_impl.cc | 8 ++++---- source/common/runtime/runtime_impl.cc | 2 +- .../filters/http/common/compressor/compressor.cc | 2 +- source/server/connection_handler_impl.cc | 12 ++++++++++++ source/server/connection_handler_impl.h | 4 ++-- test/server/connection_handler_test.cc | 16 ++++++++++++++++ 9 files changed, 39 insertions(+), 10 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index caac70356b9a..bfee20b0dfe2 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -3,3 +3,4 @@ Changes ------- +* listener: fix crash when disabling or re-enabling listeners due to overload while processing LDS updates. diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index eb7ba3619a39..fce6f4e385ae 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -306,7 +306,7 @@ bool StringUtil::findToken(absl::string_view source, absl::string_view delimiter absl::string_view key_token, bool trim_whitespace) { const auto tokens = splitToken(source, delimiters, trim_whitespace); if (trim_whitespace) { - for (const auto token : tokens) { + for (const auto& token : tokens) { if (key_token == trim(token)) { return true; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index a48bea5d08ae..204743ff52f6 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -329,7 +329,7 @@ std::string Utility::parseCookieValue(const HeaderMap& headers, const std::strin if (header.key() == Http::Headers::get().Cookie.get()) { // Split the cookie header into individual cookies. - for (const auto s : StringUtil::splitToken(header.value().getStringView(), ";")) { + for (const auto& s : StringUtil::splitToken(header.value().getStringView(), ";")) { // Find the key part of the cookie (i.e. the name of the cookie). size_t first_non_space = s.find_first_not_of(" "); size_t equals_index = s.find('='); diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 09d3b1016804..7180d4d92d90 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -113,7 +113,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, } if (request_headers.EnvoyRetriableStatusCodes()) { - for (const auto code : + for (const auto& code : StringUtil::splitToken(request_headers.getEnvoyRetriableStatusCodesValue(), ",")) { unsigned int out; if (absl::SimpleAtoi(code, &out)) { @@ -128,7 +128,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, // to provide HeaderMatcher serialized into a string. To avoid this extra // complexity we only support name-only header matchers via request // header. Anything more sophisticated needs to be provided via config. - for (const auto header_name : StringUtil::splitToken( + for (const auto& header_name : StringUtil::splitToken( request_headers.EnvoyRetriableHeaderNames()->value().getStringView(), ",")) { envoy::config::route::v3::HeaderMatcher header_matcher; header_matcher.set_name(std::string(absl::StripAsciiWhitespace(header_name))); @@ -152,7 +152,7 @@ void RetryStateImpl::enableBackoffTimer() { std::pair RetryStateImpl::parseRetryOn(absl::string_view config) { uint32_t ret = 0; bool all_fields_valid = true; - for (const auto retry_on : StringUtil::splitToken(config, ",", false, true)) { + for (const auto& retry_on : StringUtil::splitToken(config, ",", false, true)) { if (retry_on == Http::Headers::get().EnvoyRetryOnValues._5xx) { ret |= RetryPolicy::RETRY_ON_5XX; } else if (retry_on == Http::Headers::get().EnvoyRetryOnValues.GatewayError) { @@ -180,7 +180,7 @@ std::pair RetryStateImpl::parseRetryOn(absl::string_view config) std::pair RetryStateImpl::parseRetryGrpcOn(absl::string_view retry_grpc_on_header) { uint32_t ret = 0; bool all_fields_valid = true; - for (const auto retry_on : StringUtil::splitToken(retry_grpc_on_header, ",", false, true)) { + for (const auto& retry_on : StringUtil::splitToken(retry_grpc_on_header, ",", false, true)) { if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.Cancelled) { ret |= RetryPolicy::RETRY_ON_GRPC_CANCELLED; } else if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.DeadlineExceeded) { diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 6eb490d2015b..21f2440ebf44 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -411,7 +411,7 @@ void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix // Comments are useful for placeholder files with no value. const std::string text_file{api.fileSystem().fileReadToEnd(full_path)}; const auto lines = StringUtil::splitToken(text_file, "\n"); - for (const auto line : lines) { + for (const auto& line : lines) { if (!line.empty() && line.front() == '#') { continue; } diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index 482abaf348dd..3ed87b4b34aa 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -214,7 +214,7 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const { } // Find all encodings accepted by the user agent and adjust the list of allowed compressors. - for (const auto token : StringUtil::splitToken(*accept_encoding_, ",", false /* keep_empty */)) { + for (const auto& token : StringUtil::splitToken(*accept_encoding_, ",", false /* keep_empty */)) { EncPair pair = std::make_pair(StringUtil::trim(StringUtil::cropRight(token, ";")), static_cast(1)); const auto params = StringUtil::cropLeft(token, ";"); diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 24b694f82659..0b7a1c8bbb65 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -375,6 +375,18 @@ void emitLogs(Network::ListenerConfig& config, StreamInfo::StreamInfo& stream_in } } // namespace +void ConnectionHandlerImpl::ActiveTcpListener::pauseListening() { + if (listener_ != nullptr) { + listener_->disable(); + } +} + +void ConnectionHandlerImpl::ActiveTcpListener::resumeListening() { + if (listener_ != nullptr) { + listener_->enable(); + } +} + void ConnectionHandlerImpl::ActiveTcpListener::newConnection( Network::ConnectionSocketPtr&& socket, const envoy::config::core::v3::Metadata& dynamic_metadata) { diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index df6fa758bd5b..7c5e7581e265 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -134,8 +134,8 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, // ActiveListenerImplBase Network::Listener* listener() override { return listener_.get(); } - void pauseListening() override { listener_->disable(); } - void resumeListening() override { listener_->enable(); } + void pauseListening() override; + void resumeListening() override; void shutdownListener() override { listener_.reset(); } // Network::BalancedConnectionHandler diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index a7f942f95d42..e8257798b934 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -380,6 +380,22 @@ TEST_F(ConnectionHandlerTest, AddDisabledListener) { handler_->addListener(absl::nullopt, *test_listener); } +TEST_F(ConnectionHandlerTest, DisableListenerAfterStop) { + InSequence s; + + Network::ListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, false, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + EXPECT_CALL(*listener, onDestroy()); + + handler_->stopListeners(); + handler_->disableListeners(); +} + TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { InSequence s;