diff --git a/changelogs/current/bug_fixes/redis_proxy__zone-discovery-use-after-free.rst b/changelogs/current/bug_fixes/redis_proxy__zone-discovery-use-after-free.rst new file mode 100644 index 0000000000000..b3ed2c181cc3b --- /dev/null +++ b/changelogs/current/bug_fixes/redis_proxy__zone-discovery-use-after-free.rst @@ -0,0 +1,4 @@ +Fixed a use-after-free in the Redis cluster ``CLUSTER SLOTS`` discovery. A cluster refresh +(periodic resolve timer or DNS update) that arrived after ``CLUSTER SLOTS`` completed but while the +zone-discovery ``INFO`` requests it triggered were still in flight could start a second discovery, +overwrite the in-flight callbacks and free memory still referenced by the outstanding requests. diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 7d33baf2d9208..e02f8f12c3d85 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -364,6 +364,17 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() { return; } + // current_request_ is cleared as soon as CLUSTER SLOTS completes, while the + // zone-discovery INFO requests it triggers are still in flight. Re-entering + // here in that window would start another zone discovery and overwrite + // zone_callbacks_, freeing the callbacks that the in-flight INFO requests + // still reference (use-after-free). Skip until zone discovery finishes. + if (pending_zone_requests_.load() > 0) { + ENVOY_LOG(debug, "redis cluster zone discovery is already in progress for '{}'", + parent_.info_->name()); + return; + } + // If hosts is empty, we haven't received a successful result from the CLUSTER SLOTS call // yet. So, pick a random discovery address from dns and make a request. Upstream::HostSharedPtr host; diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 34ca3346cb5b5..0f6f9a4641701 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -1801,6 +1801,47 @@ TEST_F(RedisClusterTest, ZoneDiscoveryMakeRequestReturnsNull) { cluster_->prioritySet().hostSetsPerPriority()[0]->hosts()[0]->locality().zone().empty()); } +// A refresh (periodic resolve timer, DNS update) that arrives after CLUSTER +// SLOTS completes but before the zone-discovery INFO replies return must not +// start a second resolution. current_request_ is already null in that window, +// so without gating on pending_zone_requests_ the refresh starts another zone +// discovery and overwrites zone_callbacks_, freeing the callbacks that the +// in-flight INFO requests still reference (use-after-free). +TEST_F(RedisClusterTest, ZoneDiscoveryRefreshWhileInfoInFlightIsSkipped) { + auto* zone_client = setupZoneDiscoveryWithTwoNodes(); + + // Zone discovery is in flight (two INFO requests outstanding). A refresh in + // this window must not issue another CLUSTER SLOTS request. + EXPECT_CALL(*client_, makeRequest_(Ref(RedisCluster::ClusterSlotsRequest::instance_), _)) + .Times(0); + EXPECT_CALL(*zone_client, makeRequest_(Ref(RedisCluster::ClusterSlotsRequest::instance_), _)) + .Times(0); + resolve_timer_->invokeCallback(); + + // The original in-flight INFO callbacks are still valid and complete normally. + NetworkFilters::Common::Redis::RespValuePtr info_resp_1( + new NetworkFilters::Common::Redis::RespValue()); + info_resp_1->type(NetworkFilters::Common::Redis::RespType::BulkString); + info_resp_1->asString() = "# Server\navailability_zone:us-east-1a\n"; + auto zone_cb_1 = std::next(client_->client_callbacks_.begin()); + (*zone_cb_1)->onResponse(std::move(info_resp_1)); + + NetworkFilters::Common::Redis::RespValuePtr info_resp_2( + new NetworkFilters::Common::Redis::RespValue()); + info_resp_2->type(NetworkFilters::Common::Redis::RespType::BulkString); + info_resp_2->asString() = "# Server\navailability_zone:us-east-1b\n"; + zone_client->client_callbacks_.front()->onResponse(std::move(info_resp_2)); + + EXPECT_EQ(2UL, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); + for (const auto& host : cluster_->prioritySet().hostSetsPerPriority()[0]->hosts()) { + if (host->address()->asString() == "127.0.0.1:22120") { + EXPECT_EQ("us-east-1a", host->locality().zone()); + } else { + EXPECT_EQ("us-east-1b", host->locality().zone()); + } + } +} + } // namespace Redis } // namespace Clusters } // namespace Extensions