Skip to content

fix: resolve use-after-free in RawClientImpl config access #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
johnlanni merged 1 commit into from
Nov 13, 2025
Merged

fix: resolve use-after-free in RawClientImpl config access #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/common/redis/async_client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ AsyncClientImpl::threadLocalActiveClient(Upstream::HostConstSharedPtr host) {
client = std::make_unique<ThreadLocalActiveClient>(*this);
client->host_ = host;
client->redis_client_ =
client_factory_.create(host, dispatcher_, *config_, redis_command_stats_, *(stats_scope_),
client_factory_.create(host, dispatcher_, config_, redis_command_stats_, *(stats_scope_),
auth_username_, auth_password_, params_);
client->redis_client_->addConnectionCallbacks(*client);
}
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/common/redis/raw_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class RawClientFactory {
virtual ~RawClientFactory() = default;

virtual RawClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
const Config& config,
ConfigSharedPtr config,
const RedisCommandStatsSharedPtr& redis_command_stats,
Stats::Scope& scope, const std::string& auth_username,
const std::string& auth_password,
Expand Down
20 changes: 10 additions & 10 deletions source/extensions/filters/network/common/redis/raw_client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const std::string& RedisDBParamKey = "db";

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

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

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

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

return &pending_requests_.back();
Expand Down Expand Up @@ -125,7 +125,7 @@ void RawClientImpl::onData(Buffer::Instance& data) {
}

void RawClientImpl::putOutlierEvent(Upstream::Outlier::Result result) {
if (!config_.disableOutlierEvents()) {
if (!config_->disableOutlierEvents()) {
host_->outlierDetector().putResult(result);
}
}
Expand Down Expand Up @@ -157,7 +157,7 @@ void RawClientImpl::onEvent(Network::ConnectionEvent event) {
} else if (event == Network::ConnectionEvent::Connected) {
connected_ = true;
ASSERT(!pending_requests_.empty());
connect_or_op_timer_->enableTimer(config_.opTimeout());
connect_or_op_timer_->enableTimer(config_->opTimeout());
}

if (event == Network::ConnectionEvent::RemoteClose && !connected_) {
Expand Down Expand Up @@ -193,7 +193,7 @@ void RawClientImpl::onRawResponse(std::string&& response) {
if (pending_requests_.empty()) {
connect_or_op_timer_->disableTimer();
} else {
connect_or_op_timer_->enableTimer(config_.opTimeout());
connect_or_op_timer_->enableTimer(config_->opTimeout());
}

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

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

RawClientFactoryImpl RawClientFactoryImpl::instance_;

RawClientPtr RawClientFactoryImpl::create(Upstream::HostConstSharedPtr host,
Event::Dispatcher& dispatcher, const Config& config,
Event::Dispatcher& dispatcher, ConfigSharedPtr config,
const RedisCommandStatsSharedPtr& redis_command_stats,
Stats::Scope& scope, const std::string& auth_username,
const std::string& auth_password,
Expand Down
9 changes: 5 additions & 4 deletions source/extensions/filters/network/common/redis/raw_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ class RawClientImpl : public RawClient,
public:
static RawClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
RawEncoderPtr&& encoder, RawDecoderFactory& decoder_factory,
const Config& config,
ConfigSharedPtr config,
const RedisCommandStatsSharedPtr& redis_command_stats,
Stats::Scope& scope);

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

Expand Down Expand Up @@ -84,7 +84,7 @@ class RawClientImpl : public RawClient,
RawEncoderPtr encoder_;
Buffer::OwnedImpl encoder_buffer_;
DecoderPtr decoder_;
const Config& config_;
ConfigSharedPtr config_;
std::list<PendingRequest> pending_requests_;
Event::TimerPtr connect_or_op_timer_;
bool connected_{};
Expand All @@ -97,7 +97,8 @@ class RawClientImpl : public RawClient,
class RawClientFactoryImpl : public RawClientFactory {
public:
RawClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher,
const Config& config, const RedisCommandStatsSharedPtr& redis_command_stats,
ConfigSharedPtr config,
const RedisCommandStatsSharedPtr& redis_command_stats,
Stats::Scope& scope, const std::string& auth_username,
const std::string& auth_password,
const std::map<std::string, std::string>& params) override;
Expand Down
8 changes: 4 additions & 4 deletions test/extensions/filters/network/common/redis/client_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,7 +1262,7 @@ class RedisRawClientImplTest : public testing::Test,
}

void setup() {
config_ = std::make_unique<RedisRawClientDefaultConfig>();
config_ = std::make_shared<RedisRawClientDefaultConfig>();
finishSetup();
}

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

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

EXPECT_CALL(*host, createConnection_(_, _)).WillOnce(Return(conn_info));
NiceMock<Event::MockDispatcher> dispatcher;
ConfigImpl config(createConnPoolSettings());
ConfigSharedPtr config = std::make_shared<ConfigImpl>(createConnPoolSettings());
Stats::IsolatedStoreImpl stats_;
auto redis_command_stats =
Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable());
Expand Down