backport to 1.13: Prevent SEGFAULT when disabling listener (#13515)

docs/root/intro/version_history.rst

@@ -5,6 +5,7 @@ Version history
 ================
 Changes
 -------
+* listener: fix crash when disabling or re-enabling listeners due to overload while processing LDS updates.
 
 1.13.6 (September 29, 2020)
 ===========================

source/common/common/utility.cc

@@ -277,7 +277,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;
       }

source/common/http/utility.cc

@@ -159,7 +159,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('=');

source/common/router/retry_state_impl.cc

@@ -113,7 +113,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap&
   }
 
   if (request_headers.EnvoyRetriableStatusCodes()) {
-    for (const auto code : StringUtil::splitToken(
+    for (const auto& code : StringUtil::splitToken(
              request_headers.EnvoyRetriableStatusCodes()->value().getStringView(), ",")) {
       unsigned int out;
       if (absl::SimpleAtoi(code, &out)) {
@@ -128,7 +128,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap&
     // 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, ",")) {
+  for (const auto& retry_on : StringUtil::splitToken(config, ",")) {
     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, ",")) {
+  for (const auto& retry_on : StringUtil::splitToken(retry_grpc_on_header, ",")) {
     if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.Cancelled) {
       ret |= RetryPolicy::RETRY_ON_GRPC_CANCELLED;
     } else if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.DeadlineExceeded) {

source/common/runtime/runtime_impl.cc

@@ -414,7 +414,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;
         }

source/extensions/filters/network/dubbo_proxy/hessian_utils.cc

@@ -29,7 +29,7 @@ typename std::enable_if<std::is_signed<T>::value, T>::type leftShift(T left, uin
 inline void addByte(Buffer::Instance& buffer, const uint8_t value) { buffer.add(&value, 1); }
 
 void addSeq(Buffer::Instance& buffer, const std::initializer_list<uint8_t>& values) {
-  for (const int8_t& value : values) {
+  for (const uint8_t& value : values) {
     buffer.add(&value, 1);
   }
 }

source/extensions/stat_sinks/hystrix/hystrix.cc

@@ -50,7 +50,7 @@ void HystrixSink::addHistogramToStream(const QuantileLatencyMap& latency_map, ab
   // TODO: Consider if we better use join here
   ss << ", \"" << key << "\": {";
   bool is_first = true;
-  for (const std::pair<double, double>& element : latency_map) {
+  for (const auto& element : latency_map) {
     const std::string quantile = fmt::sprintf("%g", element.first * 100);
     HystrixSink::addDoubleToStream(quantile, element.second, ss, is_first);
     is_first = false;

source/server/connection_handler_impl.cc

@@ -330,6 +330,18 @@ void ConnectionHandlerImpl::ActiveTcpListener::onAcceptWorker(
   }
 }
 
+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) {
   // Find matching filter chain.

source/server/connection_handler_impl.h

@@ -130,6 +130,9 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler,
     // ActiveListenerImplBase
     Network::Listener* listener() override { return listener_.get(); }
     void destroy() override { listener_.reset(); }
+    void pauseListening() override;
+    void resumeListening() override;
+    void shutdownListener() override { listener_.reset(); }
 
     // Network::BalancedConnectionHandler
     uint64_t numConnections() const override { return num_listener_connections_; }

test/server/connection_handler_test.cc

@@ -357,6 +357,22 @@ TEST_F(ConnectionHandlerTest, AddDisabledListener) {
   handler_->addListener(*test_listener);
 }
 
+TEST_F(ConnectionHandlerTest, DisableListenerAfterStop) {
+  InSequence s;
+
+  Network::TcpListenerCallbacks* 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;