server: fix use-after-free in InitManagerImpl

include/envoy/init/init.h

@@ -22,6 +22,11 @@ class Target {
    *        initialization.
    */
   virtual void initialize(std::function<void()> callback) PURE;
+
+  /**
+   * Called when the Manager is no longer interested in this target's initialization.
+   */
+  virtual void cancel() PURE;
 };
 
 /**

source/common/config/config_provider_impl.h

@@ -149,6 +149,8 @@ class ConfigSubscriptionInstanceBase : public Init::Target,
     start();
   }
 
+  void cancel() override { initialize_callback_ = nullptr; }
+
   /**
    * Starts the subscription corresponding to a config source.
    * A derived class must own the configuration proto specific Envoy::Config::Subscription to be

source/common/router/rds_impl.h

@@ -106,6 +106,7 @@ class RdsRouteConfigSubscription
     subscription_->start({route_config_name_}, *this);
   }
 
+  void cancel() override { initialize_callback_ = nullptr; }
   // Config::SubscriptionCallbacks
   void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override;
   void onConfigUpdateFailed(const EnvoyException* e) override;

source/common/secret/sds_api.cc

@@ -40,6 +40,8 @@ void SdsApi::initialize(std::function<void()> callback) {
   subscription_->start({sds_config_name_}, *this);
 }
 
+void SdsApi::cancel() { initialize_callback_ = nullptr; }
+
 void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) {
   if (resources.empty()) {
     throw EnvoyException(

source/common/secret/sds_api.h

@@ -38,6 +38,7 @@ class SdsApi : public Init::Target,
 
   // Init::Target
   void initialize(std::function<void()> callback) override;
+  void cancel() override;
 
   // Config::SubscriptionCallbacks
   void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override;

source/server/init_manager_impl.cc

@@ -14,7 +14,15 @@ InitManagerImpl::InitManagerImpl(absl::string_view description) : description_(d
   TRACE_INIT_MANAGER("constructor");
 }
 
-InitManagerImpl::~InitManagerImpl() { TRACE_INIT_MANAGER("destructor"); }
+InitManagerImpl::~InitManagerImpl() {
+  TRACE_INIT_MANAGER("destructor");
+  if (state_ == State::Initializing) {
+    for (auto& target : targets_) {
+      TRACE_INIT_MANAGER("canceling {}", target.second);
+      target.first->cancel();
+    }
+  }
+}
 
 void InitManagerImpl::initialize(std::function<void()> callback) {
   ASSERT(state_ == State::NotInitialized);

source/server/lds_api.cc

@@ -35,6 +35,8 @@ void LdsApiImpl::initialize(std::function<void()> callback) {
   subscription_->start({}, *this);
 }
 
+void LdsApiImpl::cancel() { initialize_callback_ = nullptr; }
+
 void LdsApiImpl::onConfigUpdate(const ResourceVector& resources, const std::string& version_info) {
   cm_.adsMux().pause(Config::TypeUrl::get().RouteConfiguration);
   Cleanup rds_resume([this] { cm_.adsMux().resume(Config::TypeUrl::get().RouteConfiguration); });

source/server/lds_api.h

@@ -32,6 +32,7 @@ class LdsApiImpl : public LdsApi,
 
   // Init::Target
   void initialize(std::function<void()> callback) override;
+  void cancel() override;
 
   // Config::SubscriptionCallbacks
   void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override;

test/integration/ads_integration_test.cc

@@ -605,6 +605,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/mocks/init/mocks.cc

@@ -17,6 +17,10 @@ MockTarget::MockTarget() {
         EXPECT_EQ(nullptr, callback_);
         callback_ = callback;
       }));
+  ON_CALL(*this, cancel()).WillByDefault(Invoke([this]() {
+    EXPECT_NE(nullptr, callback_);
+    callback_ = nullptr;
+  }));
 }
 
 MockTarget::~MockTarget() {}

test/mocks/init/mocks.h

@@ -18,6 +18,7 @@ class MockTarget : public Target {
   ~MockTarget();
 
   MOCK_METHOD1(initialize, void(std::function<void()> callback));
+  MOCK_METHOD0(cancel, void());
 
   std::function<void()> callback_;
 };

test/server/init_manager_impl_test.cc

@@ -64,6 +64,18 @@ TEST_F(InitManagerImplTest, TargetAfterInitializing) {
   target1.callback_();
 }
 
+TEST_F(InitManagerImplTest, Cancel) {
+  InitManagerImpl* manager = new InitManagerImpl("test");
+  Init::MockTarget target;
+
+  manager->registerTarget(target, "");
+  EXPECT_CALL(target, initialize(_));
+  manager->initialize([&]() -> void { initialized_.ready(); });
+
+  EXPECT_CALL(target, cancel());
+  delete manager;
+}
+
 } // namespace
 } // namespace Server
 } // namespace Envoy