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 <akonradi@google.com> Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
envoyproxy · Nov 10, 2020 · b1540ba · b1540ba
1 parent affac20
commit b1540ba
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<uint32_t, bool> 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<uint32_t, bool> RetryStateImpl::parseRetryOn(absl::string_view config)
 std::pair<uint32_t, bool> 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
@@ -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
@@ -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<float>(1));
     const auto params = StringUtil::cropLeft(token, ";");

diff --git 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
@@ -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
@@ -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<Network::MockListener>();
+  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;