rds: workaround for the use-after-free crash.

This doesn't fix the underlying issue (that factory_context_ is being referenced after being freed), but it stops Envoy from crashing. *Risk Level*: Low *Testing*: bazel test //test/... (new test crashing without this patch) *Docs Changes*: n/a *Release Notes*: n/a Workaround for envoyproxy#3953. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
PiotrSikora · Jul 26, 2018 · a5d68d4 · a5d68d4
1 parent 3ee3aa3
commit a5d68d4
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 6 deletions.
diff --git a/source/common/router/BUILD b/source/common/router/BUILD
@@ -84,6 +84,7 @@ envoy_cc_library(
         "//include/envoy/thread_local:thread_local_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:minimal_logger_lib",
+        "//source/common/common:utility_lib",
         "//source/common/config:rds_json_lib",
         "//source/common/config:subscription_factory_lib",
         "//source/common/config:utility_lib",

diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -11,6 +11,7 @@
 
 #include "common/common/assert.h"
 #include "common/common/fmt.h"
+#include "common/common/utility.h"
 #include "common/config/rds_json.h"
 #include "common/config/subscription_factory.h"
 #include "common/config/utility.h"
@@ -97,7 +98,7 @@ Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() {
 
 void RdsRouteConfigProviderImpl::onConfigUpdate(const ResourceVector& resources,
                                                 const std::string& version_info) {
-  last_updated_ = factory_context_.systemTimeSource().currentTime();
+  last_updated_ = ProdSystemTimeSource::instance_.currentTime();
 
   if (resources.empty()) {
     ENVOY_LOG(debug, "Missing RouteConfiguration for {} in onConfigUpdate()", route_config_name_);

diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc
@@ -198,9 +198,10 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest,
                     fake_upstreams_[0]->localAddress()->ip()->port()));
   }
 
-  envoy::api::v2::Listener buildListener(const std::string& name, const std::string& route_config) {
-    return TestUtility::parseYaml<envoy::api::v2::Listener>(
-        fmt::format(R"EOF(
+  envoy::api::v2::Listener buildListener(const std::string& name, const std::string& route_config,
+                                         const std::string& stat_prefix = "ads_test") {
+    return TestUtility::parseYaml<envoy::api::v2::Listener>(fmt::format(
+        R"EOF(
       name: {}
       address:
         socket_address:
@@ -210,14 +211,14 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest,
         filters:
         - name: envoy.http_connection_manager
           config:
-            stat_prefix: ads_test
+            stat_prefix: {}
             codec_type: HTTP2
             rds:
               route_config_name: {}
               config_source: {{ ads: {{}} }}
             http_filters: [{{ name: envoy.router }}]
     )EOF",
-                    name, Network::Test::getLoopbackAddressString(ipVersion()), route_config));
+        name, Network::Test::getLoopbackAddressString(ipVersion()), stat_prefix, route_config));
   }
 
   envoy::api::v2::RouteConfiguration buildRouteConfig(const std::string& name,
@@ -471,6 +472,37 @@ TEST_P(AdsIntegrationTest, Failure) {
   makeSingleRequest();
 }
 
+// Regression test for the use-after-free crash when processing RDS update (#3953).
+TEST_P(AdsIntegrationTest, RdsCrash) {
+  initialize();
+
+  // Send initial configuration.
+  sendDiscoveryResponse<envoy::api::v2::Cluster>(Config::TypeUrl::get().Cluster,
+                                                 {buildCluster("cluster_0")}, "1");
+  sendDiscoveryResponse<envoy::api::v2::ClusterLoadAssignment>(
+      Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, "1");
+  sendDiscoveryResponse<envoy::api::v2::Listener>(
+      Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0")}, "1");
+  sendDiscoveryResponse<envoy::api::v2::RouteConfiguration>(
+      Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")},
+      "1");
+  test_server_->waitForCounterGe("listener_manager.listener_create_success", 1);
+
+  // Validate that we can process a request.
+  makeSingleRequest();
+
+  // Update existing LDS (change stat_prefix).
+  sendDiscoveryResponse<envoy::api::v2::Listener>(
+      Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0", "rds_crash")},
+      "2");
+  test_server_->waitForCounterGe("listener_manager.listener_create_success", 2);
+
+  // Update existing RDS (no changes).
+  sendDiscoveryResponse<envoy::api::v2::RouteConfiguration>(
+      Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")},
+      "2");
+}
+
 class AdsFailIntegrationTest : public AdsIntegrationBaseTest,
                                public Grpc::GrpcClientIntegrationParamTest {
 public: