backport to 1.14: 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.14.5 (September 29, 2020)
 ===========================

source/common/common/utility.cc

@@ -283,7 +283,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

@@ -310,7 +310,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

@@ -114,7 +114,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy,
   }
 
   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)) {
@@ -129,7 +129,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)));
@@ -153,7 +153,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) {
@@ -181,7 +181,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) {

source/common/runtime/runtime_impl.cc

@@ -406,7 +406,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/http/common/compressor/compressor.cc

@@ -202,7 +202,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, ";");
@@ -242,7 +242,7 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {
   // Find intersection of encodings accepted by the user agent and provided
   // by the allowed compressors and choose the one with the highest q-value.
   EncPair choice{Http::Headers::get().AcceptEncodingValues.Identity, static_cast<float>(0)};
-  for (const auto pair : pairs) {
+  for (const auto& pair : pairs) {
     if ((pair.second > choice.second) &&
         (allowed_compressors.count(std::string(pair.first)) ||
          pair.first == Http::Headers::get().AcceptEncodingValues.Identity ||

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

@@ -339,6 +339,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) {
   auto stream_info = std::make_unique<StreamInfo::StreamInfoImpl>(parent_.dispatcher_.timeSource());

source/server/connection_handler_impl.h

@@ -130,8 +130,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

test/server/connection_handler_test.cc

@@ -363,6 +363,22 @@ TEST_F(ConnectionHandlerTest, AddDisabledListener) {
   handler_->addListener(*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(*test_listener);
+
+  EXPECT_CALL(*listener, onDestroy());
+
+  handler_->stopListeners();
+  handler_->disableListeners();
+}
+
 TEST_F(ConnectionHandlerTest, DestroyCloseConnections) {
   InSequence s;