From 1e6246716da8ba28a4361ac61a01702c5d7f4379 Mon Sep 17 00:00:00 2001 From: William Dauchy Date: Thu, 26 Feb 2026 12:49:21 +0100 Subject: [PATCH] eds: use-after-free in LEDS callback on subsequent EDS updates The LEDS subscription callback lambda captured `used_load_assignment` by value as a raw pointer to the object owned by the `cluster_load_assignment_` unique_ptr. When a subsequent EDS update reassigned `cluster_load_assignment_`, the old object was destroyed but existing LEDS subscriptions (not recreated for unchanged configs) still held the dangling pointer. When the LEDS subscription later fired its callback (e.g. onConfigUpdateFailed), dereferencing this pointer caused a segfault. Stack trace: #0: [0x77b9d6de8330] #1: Envoy::Upstream::EdsClusterImpl::BatchUpdateHelper::batchUpdate() #2: Envoy::Upstream::PrioritySetImpl::batchHostUpdate() #3: std::__1::__function::__func<>::operator()() #4: Envoy::Upstream::LedsSubscription::onConfigUpdateFailed() #5: Envoy::Config::GrpcSubscriptionImpl::onConfigUpdateFailed() #6: event_process_active_single_queue #7: event_base_loop #8: Envoy::Server::InstanceBase::run() Fix by capturing `this` and accessing `cluster_load_assignment_` directly, which always reflects the current valid assignment. Signed-off-by: William Dauchy --- source/extensions/clusters/eds/eds.cc | 6 +-- test/extensions/clusters/eds/eds_test.cc | 60 ++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/source/extensions/clusters/eds/eds.cc b/source/extensions/clusters/eds/eds.cc index ebb37c2e5fc33..20a013bc8569c 100644 --- a/source/extensions/clusters/eds/eds.cc +++ b/source/extensions/clusters/eds/eds.cc @@ -309,10 +309,10 @@ void EdsClusterImpl::update( // Create a new LEDS subscription and add it to the subscriptions map. LedsSubscriptionPtr leds_locality_subscription = std::make_unique( leds_config, edsServiceName(), *transport_factory_context_, info_->statsScope(), - [&, used_load_assignment]() { - // Called upon an update to the locality. + [this]() { if (validateAllLedsUpdated()) { - BatchUpdateHelper helper(*this, *used_load_assignment); + ASSERT(cluster_load_assignment_ != nullptr); + BatchUpdateHelper helper(*this, *cluster_load_assignment_); priority_set_.batchHostUpdate(helper); } }); diff --git a/test/extensions/clusters/eds/eds_test.cc b/test/extensions/clusters/eds/eds_test.cc index d4ca2961c1973..92bc8a04f48fe 100644 --- a/test/extensions/clusters/eds/eds_test.cc +++ b/test/extensions/clusters/eds/eds_test.cc @@ -2992,6 +2992,66 @@ TEST_F(EdsTest, OnConfigUpdateLedsAndEndpoints) { "(resource: xdstp://foo/leds/collection) and a list of endpoints."); } +class EdsWithLedsTest : public EdsTest { +public: + EdsWithLedsTest() { + ON_CALL(server_context_.cluster_manager_.subscription_factory_, + collectionSubscriptionFromUrl(_, _, _, _, _, _)) + .WillByDefault(Invoke( + [this](const xds::core::v3::ResourceLocator&, + const envoy::config::core::v3::ConfigSource&, absl::string_view, Stats::Scope&, + Envoy::Config::SubscriptionCallbacks& callbacks, + Envoy::Config::OpaqueResourceDecoderSharedPtr) -> Config::SubscriptionPtr { + auto ret = std::make_unique>(); + leds_callbacks_ = &callbacks; + return ret; + })); + } + + envoy::config::endpoint::v3::ClusterLoadAssignment + makeLedsClusterLoadAssignment(const std::string& cluster_name) { + envoy::config::endpoint::v3::ClusterLoadAssignment cla; + cla.set_cluster_name(cluster_name); + auto* endpoints = cla.add_endpoints(); + auto* locality = endpoints->mutable_locality(); + locality->set_region("us-east-1"); + locality->set_zone("us-east-1a"); + auto* leds_conf = endpoints->mutable_leds_cluster_locality_config(); + leds_conf->set_leds_collection_name( + "xdstp://test/envoy.config.endpoint.v3.LbEndpoint/foo-endpoints/*"); + auto* leds_config_source = leds_conf->mutable_leds_config()->mutable_api_config_source(); + leds_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::DELTA_GRPC); + leds_config_source->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name("xds_cluster"); + return cla; + } + + Config::SubscriptionCallbacks* leds_callbacks_{}; +}; + +// Regression test: a LEDS callback firing after a subsequent EDS update must +// not dereference the stale cluster_load_assignment_ pointer. +TEST_F(EdsWithLedsTest, LedsCallbackAfterSubsequentEdsUpdateNoCrash) { + initialize(); + + auto cla = makeLedsClusterLoadAssignment("fare"); + + // First EDS update creates the LEDS subscription (not yet updated). + doOnConfigUpdateVerifyNoThrow(cla); + ASSERT_NE(nullptr, leds_callbacks_); + + // Second EDS update destroys the old cluster_load_assignment_ but keeps + // the existing LEDS subscription. + doOnConfigUpdateVerifyNoThrow(cla); + + // LEDS failure triggers the callback; would segfault on the dangling pointer + // without the fix. + leds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, + nullptr); + + EXPECT_TRUE(initialized_); + EXPECT_EQ(0UL, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); +} + class EdsCachedAssignmentTest : public testing::Test { public: EdsCachedAssignmentTest() { resetCluster(); }