Skip to content

Commit 9079b87

Browse files
johnlannijohnlanni
committed
fix: resolve use-after-free in RawClientImpl config access
Fixed a critical use-after-free bug where RawClientImpl would crash when accessing config_.disableOutlierEvents() after the config object was destroyed. Root Cause: - RawClientImpl held a const Config& reference without reference counting - When AsyncClientImpl::initialize() created a new config_ shared_ptr, the old config object was destroyed - RawClientImpl continued processing async network data and accessed the destroyed config reference, causing a crash Solution: Changed RawClientImpl to hold ConfigSharedPtr instead of const Config&, ensuring proper lifetime management through reference counting. Changes: - Modified RawClientFactory::create() to accept ConfigSharedPtr - Updated RawClientImpl to store ConfigSharedPtr instead of reference - Changed all config_.method() calls to config_->method() - Updated AsyncClientImpl to pass config_ directly (already shared_ptr) - Fixed unit tests to use ConfigSharedPtr Test Plan: - bazel build //source/exe:envoy-static - PASS - bazel test //test/extensions/filters/network/common/redis:all - PASS (3/3) - bazel test //test/extensions/filters/network/common/redis:client_impl_test - PASS Change-Id: I0c16f7930714eeee4859238ef46e620948fea9e2 Co-developed-by: Claude <noreply@anthropic.com>
1 parent 2f61478 commit 9079b87

File tree

5 files changed

+21
-20
lines changed

5 files changed

+21
-20
lines changed

‎source/common/redis/async_client_impl.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ AsyncClientImpl::threadLocalActiveClient(Upstream::HostConstSharedPtr host) {
275275
client = std::make_unique<ThreadLocalActiveClient>(*this);
276276
client->host_ = host;
277277
client->redis_client_ =
278-
client_factory_.create(host, dispatcher_, *config_, redis_command_stats_, *(stats_scope_),
278+
client_factory_.create(host, dispatcher_, config_, redis_command_stats_, *(stats_scope_),
279279
auth_username_, auth_password_, params_);
280280
client->redis_client_->addConnectionCallbacks(*client);
281281
}

‎source/extensions/filters/network/common/redis/raw_client.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ class RawClientFactory {
7474
virtual ~RawClientFactory() = default;
7575

7676
virtual RawClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
77-
const Config& config,
77+
ConfigSharedPtr config,
7878
const RedisCommandStatsSharedPtr& redis_command_stats,
7979
Stats::Scope& scope, const std::string& auth_username,
8080
const std::string& auth_password,

‎source/extensions/filters/network/common/redis/raw_client_impl.cc‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ const std::string& RedisDBParamKey = "db";
1616

1717
RawClientPtr RawClientImpl::create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
1818
RawEncoderPtr&& encoder, RawDecoderFactory& decoder_factory,
19-
const Config& config,
19+
ConfigSharedPtr config,
2020
const RedisCommandStatsSharedPtr& redis_command_stats,
2121
Stats::Scope& scope) {
2222
auto client = std::make_unique<RawClientImpl>(
@@ -31,7 +31,7 @@ RawClientPtr RawClientImpl::create(Upstream::HostConstSharedPtr host, Event::Dis
3131

3232
RawClientImpl::RawClientImpl(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
3333
RawEncoderPtr&& encoder, RawDecoderFactory& decoder_factory,
34-
const Config& config,
34+
ConfigSharedPtr config,
3535
const RedisCommandStatsSharedPtr& redis_command_stats,
3636
Stats::Scope& scope)
3737
: host_(host), encoder_(std::move(encoder)), decoder_(decoder_factory.create(*this)),
@@ -77,10 +77,10 @@ PoolRequest* RawClientImpl::makeRawRequest(std::string_view request,
7777
encoder_->encode(request, encoder_buffer_);
7878

7979
// If buffer is full, flush. If the buffer was empty before the request, start the timer.
80-
if (encoder_buffer_.length() >= config_.maxBufferSizeBeforeFlush()) {
80+
if (encoder_buffer_.length() >= config_->maxBufferSizeBeforeFlush()) {
8181
flushBufferAndResetTimer();
8282
} else if (empty_buffer) {
83-
flush_timer_->enableTimer(std::chrono::milliseconds(config_.bufferFlushTimeoutInMs()));
83+
flush_timer_->enableTimer(std::chrono::milliseconds(config_->bufferFlushTimeoutInMs()));
8484
}
8585

8686
// Only boost the op timeout if:
@@ -90,7 +90,7 @@ PoolRequest* RawClientImpl::makeRawRequest(std::string_view request,
9090
// - This is the first request on the pipeline. Otherwise the timeout would effectively start on
9191
// the last operation.
9292
if (connected_ && pending_requests_.size() == 1) {
93-
connect_or_op_timer_->enableTimer(config_.opTimeout());
93+
connect_or_op_timer_->enableTimer(config_->opTimeout());
9494
}
9595

9696
return &pending_requests_.back();
@@ -125,7 +125,7 @@ void RawClientImpl::onData(Buffer::Instance& data) {
125125
}
126126

127127
void RawClientImpl::putOutlierEvent(Upstream::Outlier::Result result) {
128-
if (!config_.disableOutlierEvents()) {
128+
if (!config_->disableOutlierEvents()) {
129129
host_->outlierDetector().putResult(result);
130130
}
131131
}
@@ -157,7 +157,7 @@ void RawClientImpl::onEvent(Network::ConnectionEvent event) {
157157
} else if (event == Network::ConnectionEvent::Connected) {
158158
connected_ = true;
159159
ASSERT(!pending_requests_.empty());
160-
connect_or_op_timer_->enableTimer(config_.opTimeout());
160+
connect_or_op_timer_->enableTimer(config_->opTimeout());
161161
}
162162

163163
if (event == Network::ConnectionEvent::RemoteClose && !connected_) {
@@ -193,7 +193,7 @@ void RawClientImpl::onRawResponse(std::string&& response) {
193193
if (pending_requests_.empty()) {
194194
connect_or_op_timer_->disableTimer();
195195
} else {
196-
connect_or_op_timer_->enableTimer(config_.opTimeout());
196+
connect_or_op_timer_->enableTimer(config_->opTimeout());
197197
}
198198

199199
putOutlierEvent(Upstream::Outlier::Result::ExtOriginRequestSuccess);
@@ -239,15 +239,15 @@ void RawClientImpl::initialize(const std::string& auth_username, const std::stri
239239
makeRawRequest(select_request, null_raw_client_callbacks);
240240
}
241241

242-
if (config_.readPolicy() != Common::Redis::Client::ReadPolicy::Primary) {
242+
if (config_->readPolicy() != Common::Redis::Client::ReadPolicy::Primary) {
243243
makeRawRequest(Utility::makeRawReadOnlyRequest(), null_raw_client_callbacks);
244244
}
245245
}
246246

247247
RawClientFactoryImpl RawClientFactoryImpl::instance_;
248248

249249
RawClientPtr RawClientFactoryImpl::create(Upstream::HostConstSharedPtr host,
250-
Event::Dispatcher& dispatcher, const Config& config,
250+
Event::Dispatcher& dispatcher, ConfigSharedPtr config,
251251
const RedisCommandStatsSharedPtr& redis_command_stats,
252252
Stats::Scope& scope, const std::string& auth_username,
253253
const std::string& auth_password,

‎source/extensions/filters/network/common/redis/raw_client_impl.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,12 @@ class RawClientImpl : public RawClient,
1717
public:
1818
static RawClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
1919
RawEncoderPtr&& encoder, RawDecoderFactory& decoder_factory,
20-
const Config& config,
20+
ConfigSharedPtr config,
2121
const RedisCommandStatsSharedPtr& redis_command_stats,
2222
Stats::Scope& scope);
2323

2424
RawClientImpl(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
25-
RawEncoderPtr&& encoder, RawDecoderFactory& decoder_factory, const Config& config,
25+
RawEncoderPtr&& encoder, RawDecoderFactory& decoder_factory, ConfigSharedPtr config,
2626
const RedisCommandStatsSharedPtr& redis_command_stats, Stats::Scope& scope);
2727
~RawClientImpl() override;
2828

@@ -84,7 +84,7 @@ class RawClientImpl : public RawClient,
8484
RawEncoderPtr encoder_;
8585
Buffer::OwnedImpl encoder_buffer_;
8686
DecoderPtr decoder_;
87-
const Config& config_;
87+
ConfigSharedPtr config_;
8888
std::list<PendingRequest> pending_requests_;
8989
Event::TimerPtr connect_or_op_timer_;
9090
bool connected_{};
@@ -97,7 +97,8 @@ class RawClientImpl : public RawClient,
9797
class RawClientFactoryImpl : public RawClientFactory {
9898
public:
9999
RawClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
100-
const Config& config, const RedisCommandStatsSharedPtr& redis_command_stats,
100+
ConfigSharedPtr config,
101+
const RedisCommandStatsSharedPtr& redis_command_stats,
101102
Stats::Scope& scope, const std::string& auth_username,
102103
const std::string& auth_password,
103104
const std::map<std::string, std::string>& params) override;

‎test/extensions/filters/network/common/redis/client_impl_test.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1262,7 +1262,7 @@ class RedisRawClientImplTest : public testing::Test,
12621262
}
12631263

12641264
void setup() {
1265-
config_ = std::make_unique<RedisRawClientDefaultConfig>();
1265+
config_ = std::make_shared<RedisRawClientDefaultConfig>();
12661266
finishSetup();
12671267
}
12681268

@@ -1291,7 +1291,7 @@ class RedisRawClientImplTest : public testing::Test,
12911291
Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable());
12921292

12931293
client_ = RawClientImpl::create(host_, dispatcher_, Common::Redis::RawEncoderPtr{encoder_},
1294-
*this, *config_, redis_command_stats_, *stats_.rootScope());
1294+
*this, config_, redis_command_stats_, *stats_.rootScope());
12951295
EXPECT_EQ(1UL, host_->cluster_.traffic_stats_->upstream_cx_total_.value());
12961296
EXPECT_EQ(1UL, host_->stats_.cx_total_.value());
12971297
EXPECT_EQ(false, client_->active());
@@ -1346,7 +1346,7 @@ class RedisRawClientImplTest : public testing::Test,
13461346
Common::Redis::RawDecoderCallbacks* callbacks_{};
13471347
NiceMock<Network::MockClientConnection>* upstream_connection_{};
13481348
Network::ReadFilterSharedPtr upstream_read_filter_;
1349-
std::unique_ptr<Config> config_;
1349+
ConfigSharedPtr config_;
13501350
RawClientPtr client_;
13511351
NiceMock<Stats::MockIsolatedStatsStore> stats_;
13521352
Stats::ScopeSharedPtr stats_scope_;
@@ -1416,7 +1416,7 @@ TEST(RedisRawClientFactoryImplTest, Basic) {
14161416

14171417
EXPECT_CALL(*host, createConnection_(_, _)).WillOnce(Return(conn_info));
14181418
NiceMock<Event::MockDispatcher> dispatcher;
1419-
ConfigImpl config(createConnPoolSettings());
1419+
ConfigSharedPtr config = std::make_shared<ConfigImpl>(createConnPoolSettings());
14201420
Stats::IsolatedStoreImpl stats_;
14211421
auto redis_command_stats =
14221422
Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable());

0 commit comments

Comments
 (0)