Skip to content

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

Merged
johnlanni merged 1 commit intohigress-group:envoy-1.27from
johnlanni:fix-redis-init-bug
Nov 13, 2025
Merged

fix: resolve use-after-free in RawClientImpl config access#15
johnlanni merged 1 commit intohigress-group:envoy-1.27from
johnlanni:fix-redis-init-bug

Conversation

@johnlanni
Copy link
Copy Markdown

@johnlanni johnlanni commented Nov 13, 2025

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

Co-developed-by: Claude noreply@anthropic.com

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]


Note

Switch RawClient to hold and receive ConfigSharedPtr, updating factory, impl, callers, and tests to avoid lifetime issues.

  • Redis client internals:
    • Replace const Config& with ConfigSharedPtr across RawClientFactory::create, RawClientImpl::{create,ctor} and member config_.
    • Update all config access from config_.method() to config_->method().
  • Async client:
    • Pass shared config_ directly to client_factory_.create(...) in AsyncClientImpl::threadLocalActiveClient.
  • Tests:
    • Update Redis raw client tests and factory tests to construct and pass ConfigSharedPtr and match new interfaces.

Written by Cursor Bugbot for commit 9079b87. This will update automatically on new commits. Configure here.

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>
@johnlanni johnlanni merged commit f1e3f3b into higress-group:envoy-1.27 Nov 13, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant