Skip to content

Commit 40aa08d

Browse files
committed
redis: Use local shared_ptr copies to prevent race conditions
The previous fix with null checks still had a race condition window between checking the pointer and using it. Even with the null check, the shared_ptr could be reset to null by another thread between the check and use. Solution: Make local copies of shared_ptr before use. This ensures the pointer remains valid throughout its usage in the current scope. Changes: 1. startResolveRedis(): Copy info_ to local variable before use 2. updateDnsStats(): Use local copy of info_ 3. DNS callbacks: Use local copy for stats updates 4. onResponse(), onUnexpectedResponse(), onFailure(): Use local copies 5. client_factory_.create(): Check and use local copy of info_ The pattern applied: auto info = parent_.info_; // Make local copy (ref count++) if (!info) { // Check if null return; } info->method(); // Safe to use - won't become null This prevents the crash at line 376 where info_ was becoming null between the check and the access, even with memory_order_acquire.
1 parent ab759f8 commit 40aa08d

File tree

1 file changed

+28
-18
lines changed

1 file changed

+28
-18
lines changed

source/extensions/clusters/redis/redis_cluster.cc

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,12 @@ void RedisCluster::DnsDiscoveryResolveTarget::startResolveDns() {
257257
active_query_ = nullptr;
258258
ENVOY_LOG(trace, "async DNS resolution complete for {}", dns_address_);
259259
if (status == Network::DnsResolver::ResolutionStatus::Failure || response.empty()) {
260-
if (parent_.info_) {
260+
auto info = parent_.info_;
261+
if (info) {
261262
if (status == Network::DnsResolver::ResolutionStatus::Failure) {
262-
parent_.info_->configUpdateStats().update_failure_.inc();
263+
info->configUpdateStats().update_failure_.inc();
263264
} else {
264-
parent_.info_->configUpdateStats().update_empty_.inc();
265+
info->configUpdateStats().update_empty_.inc();
265266
}
266267
}
267268

@@ -368,16 +369,17 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() {
368369
return;
369370
}
370371

371-
// Also check if info_ is still valid
372-
if (!parent_.info_) {
372+
// Make a local copy of the shared_ptr to prevent it from becoming null between check and use
373+
auto info = parent_.info_;
374+
if (!info) {
373375
return;
374376
}
375377

376-
parent_.info_->configUpdateStats().update_attempt_.inc();
378+
info->configUpdateStats().update_attempt_.inc();
377379
// If a resolution is currently in progress, skip it.
378380
if (current_request_) {
379381
ENVOY_LOG(debug, "redis cluster slot request is already in progress for '{}'",
380-
parent_.info_ ? parent_.info_->name() : "unknown");
382+
info ? info->name() : "unknown");
381383
return;
382384
}
383385

@@ -400,25 +402,30 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() {
400402
if (!client) {
401403
client = std::make_unique<RedisDiscoveryClient>(*this);
402404
client->host_ = current_host_address_;
405+
auto parent_info = parent_.info_;
406+
if (!parent_info) {
407+
return;
408+
}
403409
client->client_ = client_factory_.create(host, dispatcher_, shared_from_this(),
404-
redis_command_stats_, parent_.info()->statsScope(),
410+
redis_command_stats_, parent_info->statsScope(),
405411
parent_.auth_username_, parent_.auth_password_, false);
406412
client->client_->addConnectionCallbacks(*client);
407413
}
408414
ENVOY_LOG(debug, "executing redis cluster slot request for '{}'",
409-
parent_.info_ ? parent_.info_->name() : "unknown");
415+
info ? info->name() : "unknown");
410416
current_request_ = client->client_->makeRequest(ClusterSlotsRequest::instance_, *this);
411417
}
412418

413419
void RedisCluster::RedisDiscoverySession::updateDnsStats(
414420
Network::DnsResolver::ResolutionStatus status, bool empty_response) {
415-
if (!parent_.info_) {
421+
auto info = parent_.info_;
422+
if (!info) {
416423
return;
417424
}
418425
if (status == Network::DnsResolver::ResolutionStatus::Failure) {
419-
parent_.info_->configUpdateStats().update_failure_.inc();
426+
info->configUpdateStats().update_failure_.inc();
420427
} else if (empty_response) {
421-
parent_.info_->configUpdateStats().update_empty_.inc();
428+
info->configUpdateStats().update_empty_.inc();
422429
}
423430
}
424431

@@ -579,8 +586,9 @@ void RedisCluster::RedisDiscoverySession::onResponse(
579586
return;
580587
}
581588

589+
auto info = parent_.info_;
582590
ENVOY_LOG(debug, "redis cluster slot request for '{}' succeeded",
583-
parent_.info_ ? parent_.info_->name() : "unknown");
591+
info ? info->name() : "unknown");
584592
current_request_ = nullptr;
585593

586594
const uint32_t SlotRangeStart = 0;
@@ -714,8 +722,9 @@ void RedisCluster::RedisDiscoverySession::onUnexpectedResponse(
714722
}
715723

716724
ENVOY_LOG(warn, "Unexpected response to cluster slot command: {}", value->toString());
717-
if (this->parent_.info_) {
718-
this->parent_.info_->configUpdateStats().update_failure_.inc();
725+
auto info = this->parent_.info_;
726+
if (info) {
727+
info->configUpdateStats().update_failure_.inc();
719728
}
720729
if (resolve_timer_) {
721730
resolve_timer_->enableTimer(parent_.cluster_refresh_rate_);
@@ -730,14 +739,15 @@ void RedisCluster::RedisDiscoverySession::onFailure() {
730739
return;
731740
}
732741

742+
auto info = parent_.info_;
733743
ENVOY_LOG(debug, "redis cluster slot request for '{}' failed",
734-
parent_.info_ ? parent_.info_->name() : "unknown");
744+
info ? info->name() : "unknown");
735745
if (!current_host_address_.empty()) {
736746
auto client_to_delete = client_map_.find(current_host_address_);
737747
client_to_delete->second->client_->close();
738748
}
739-
if (parent_.info_) {
740-
parent_.info_->configUpdateStats().update_failure_.inc();
749+
if (info) {
750+
info->configUpdateStats().update_failure_.inc();
741751
}
742752
if (resolve_timer_) {
743753
resolve_timer_->enableTimer(parent_.cluster_refresh_rate_);

0 commit comments

Comments
 (0)