server: fix use-after-free in InitManagerImpl

source/server/init_manager_impl.cc

@@ -10,11 +10,24 @@
 namespace Envoy {
 namespace Server {
 
-InitManagerImpl::InitManagerImpl(absl::string_view description) : description_(description) {
+InitManagerImpl::InitManagerImpl(absl::string_view description)
+    : canceled_(std::make_shared<bool>(false)), description_(description) {
   TRACE_INIT_MANAGER("constructor");
 }
 
-InitManagerImpl::~InitManagerImpl() { TRACE_INIT_MANAGER("destructor"); }
+InitManagerImpl::~InitManagerImpl() {
+  switch (state_) {
+  case State::NotInitialized:
+    TRACE_INIT_MANAGER("destructor, not initialized");
+    break;
+  case State::Initializing:
+    TRACE_INIT_MANAGER("destructor, canceling initialization");
+    *canceled_ = true;
+    break;
+  case State::Initialized:
+    TRACE_INIT_MANAGER("destructor, initialized");
+  }
+}
 
 void InitManagerImpl::initialize(std::function<void()> callback) {
   ASSERT(state_ == State::NotInitialized);
@@ -38,14 +51,16 @@ void InitManagerImpl::initialize(std::function<void()> callback) {
 
 void InitManagerImpl::initializeTarget(TargetWithDescription& target) {
   TRACE_INIT_MANAGER("invoking initializeTarget {}", target.second);
-  target.first->initialize([this, &target]() -> void {
-    TRACE_INIT_MANAGER("completed initializeTarget {}", target.second);
-    ASSERT(std::find(targets_.begin(), targets_.end(), target) != targets_.end());
-    targets_.remove(target);
-    if (targets_.empty()) {
-      TRACE_INIT_MANAGER("initialized");
-      state_ = State::Initialized;
-      callback_();
+  target.first->initialize([canceled = canceled_, this, &target]() -> void {
+    if (!*canceled) {
+      TRACE_INIT_MANAGER("completed initializeTarget {}", target.second);
+      ASSERT(std::find(targets_.begin(), targets_.end(), target) != targets_.end());
+      targets_.remove(target);
+      if (targets_.empty()) {
+        TRACE_INIT_MANAGER("initialized");
+        state_ = State::Initialized;
+        callback_();
+      }
     }
   });
 }

source/server/init_manager_impl.h

@@ -29,6 +29,7 @@ class InitManagerImpl : public Init::Manager, Logger::Loggable<Logger::Id::init>
 
   void initializeTarget(TargetWithDescription& target);
 
+  std::shared_ptr<bool> canceled_; // For safe use by callbacks passed to init targets.
   std::list<TargetWithDescription> targets_;
   State state_{State::NotInitialized};
   std::function<void()> callback_;

test/integration/ads_integration_test.cc

@@ -606,6 +606,84 @@ TEST_P(AdsIntegrationTest, RdsAfterLdsWithRdsChange) {
   makeSingleRequest();
 }
 
+// Regression test for the use-after-free crash when a listener awaiting an RDS update is destroyed.
+TEST_P(AdsIntegrationTest, RdsAfterLdsInvalidated) {
+
+  initialize();
+
+  // STEP 1: Initial setup
+  // ---------------------
+
+  // Initial request for any cluster, respond with cluster_0 version 1
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}));
+  sendDiscoveryResponse<envoy::api::v2::Cluster>(Config::TypeUrl::get().Cluster,
+                                                 {buildCluster("cluster_0")}, "1");
+
+  // Initial request for load assignment for cluster_0, respond with version 1
+  EXPECT_TRUE(
+      compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", {"cluster_0"}));
+  sendDiscoveryResponse<envoy::api::v2::ClusterLoadAssignment>(
+      Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, "1");
+
+  // Request for updates to cluster_0 version 1, no response
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "1", {}));
+
+  // Initial request for any listener, respond with listener_0 version 1
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "", {}));
+  sendDiscoveryResponse<envoy::api::v2::Listener>(
+      Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0")}, "1");
+
+  // Request for updates to load assignment version 1, no response
+  EXPECT_TRUE(
+      compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1", {"cluster_0"}));
+
+  // Initial request for route_config_0 (referenced by listener_0), respond with version 1
+  EXPECT_TRUE(
+      compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "", {"route_config_0"}));
+  sendDiscoveryResponse<envoy::api::v2::RouteConfiguration>(
+      Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")},
+      "1");
+
+  // Wait for initial listener to be created successfully. Any subsequent listeners will then use
+  // the dynamic InitManager (see ListenerImpl::initManager).
+  test_server_->waitForCounterGe("listener_manager.listener_create_success", 1);
+
+  // STEP 2: Listener with dynamic InitManager
+  // -----------------------------------------
+
+  // Request for updates to listener_0 version 1, respond with version 2. Under the hood, this
+  // invokes RdsRouteConfigSubscription::registerInitTarget with the new ListenerImpl instance.
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "1", {}));
+  sendDiscoveryResponse<envoy::api::v2::Listener>(
+      Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_1")}, "2");
+
+  // Request for updates to route_config_0 version 1, and initial request for route_config_1
+  // (referenced by listener_0), don't respond yet!
+  EXPECT_TRUE(
+      compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "1", {"route_config_0"}));
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "1",
+                                      {"route_config_1", "route_config_0"}));
+
+  // STEP 3: "New listener, who dis?"
+  // --------------------------------
+
+  // Request for updates to listener_0 version 2, respond with version 3 (updated stats prefix).
+  // This should blow away the previous ListenerImpl instance, which is still waiting for
+  // route_config_1...
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "2", {}));
+  sendDiscoveryResponse<envoy::api::v2::Listener>(
+      Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_1", "omg")}, "3");
+
+  // Respond to prior request for route_config_1. Under the hood, this invokes
+  // RdsRouteConfigSubscription::runInitializeCallbackIfAny, which references the defunct
+  // ListenerImpl instance. We should not crash in this event!
+  sendDiscoveryResponse<envoy::api::v2::RouteConfiguration>(
+      Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_1", "cluster_0")},
+      "1");
+
+  test_server_->waitForCounterGe("listener_manager.listener_create_success", 2);
+}
+
 class AdsFailIntegrationTest : public Grpc::GrpcClientIntegrationParamTest,
                                public HttpIntegrationTest {
 public:

test/server/init_manager_impl_test.cc

@@ -64,6 +64,19 @@ TEST_F(InitManagerImplTest, TargetAfterInitializing) {
   target1.callback_();
 }
 
+// standalone test, not using manager_ and initialized_ from fixture
+TEST(StandaloneInitManagerImplTest, DeletedInitManagerImpl) {
+  auto manager = new InitManagerImpl("test");
+  Init::MockTarget target;
+
+  manager->registerTarget(target, "");
+  EXPECT_CALL(target, initialize(_));
+  manager->initialize([&]() -> void {});
+
+  delete manager;
+  target.callback_();
+}
+
 } // namespace
 } // namespace Server
 } // namespace Envoy