router: keep ECDS filter config provider manager alive for its subscriptions - #46538
Merged
yanavlasov merged 1 commit intoAug 4, 2026
Merged
Conversation
…ions
Filter::FilterConfigSubscription stores the provider manager as a raw
reference and writes through it from its destructor:
FilterConfigSubscription::~FilterConfigSubscription() {
init_target_.ready();
filter_config_provider_manager_.subscriptions_.erase(subscription_id_);
}
The manager must therefore outlive every subscription it created. Most
owners get this right (ClusterInfoImpl and UdpProxyFilterConfigImpl both
declare the manager before the factory lists; the listener-side managers
live in ProdListenerComponentFactory, which outlives the listener lists),
but two call sites obtain the manager as a local shared_ptr and drop it:
- Router::FilterConfig, for `upstream_http_filters`. The upstream manager
singleton is registered *unpinned*, so Singleton::ManagerImpl only holds
a weak_ptr. Its sole other holder is ClusterInfoImpl. When the router
config is built while no cluster exists -- a static listener with
CDS-only clusters, for example -- the local reference is the only one,
and the manager dies at the end of the constructor. Every subscription
created for those filters then holds a dangling reference for the life
of the process and writes through it at teardown, corrupting the heap.
- Composite's ExecuteFilterAction, for `dynamic_config` actions. Same
pattern; currently masked for the upstream variant by ClusterInfoImpl
happening to hold the same singleton.
Fix both by retaining the manager, declared so that it is destroyed after
the providers whose subscriptions reference it.
Note that ClusterInfoImpl destruction is deferred via
Dispatcher::deleteInDispatcherThread, and that queue is not drained until
dispatcher_->shutdown() -- after listener_manager_.reset() in
~InstanceBase. Plain server shutdown is therefore safe today only by
accident of that ordering, not by design.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Samuel Adelman <samuel.adelman@gmail.com>
adelsam
requested review from
mattklein123,
tyxia and
yanavlasov
as code owners
August 4, 2026 13:11
Contributor
|
See also #46439 |
yanavlasov
approved these changes
Aug 4, 2026
yanavlasov
enabled auto-merge (squash)
August 4, 2026 14:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: router: keep ECDS filter config provider manager alive for its subscriptions
Additional Description:
Filter::FilterConfigSubscriptionstores its provider manager as a raw reference and writes throughit from its destructor:
The manager must therefore outlive every subscription it created. The upstream HTTP filter config
provider manager is a singleton registered without
pin, soSingleton::ManagerImplretains only aweak_ptrand the documented contract inenvoy/singleton/manager.happlies:ClusterInfoImpl(upstream_impl.h) andUdpProxyFilterConfigImpl(udp_proxy/config.h) comply,each holding the manager as a member declared before their filter factory lists.
Router::FilterConfigdid not: it obtained the manager as a constructor-localshared_ptranddropped it on return, while the ECDS subscriptions created by
processFilters()live on inupstream_http_filter_factories_. When no other holder exists at that moment, the manager isdestroyed as the constructor returns and those subscriptions hold a dangling reference for the
remaining life of the process, dereferencing it on teardown.
ClusterInfoImplcreates this manager unconditionally, so an existing cluster normally masks thedefect. The reachable window is a router config built while no cluster exists — for example a
statically configured listener with CDS-supplied clusters.
This change retains the manager in a member declared before
upstream_http_filter_factories_, soreverse-declaration destruction tears down the providers and their subscriptions first and releases
the manager last, matching the existing
ClusterInfoImpland UDP proxy pattern.Composite's
ExecuteFilterActionhas the same constructor-local pattern fordynamic_configactionsand is hardened identically. That instance is currently masked by
ClusterInfoImplholding the samesingleton, so it is defense-in-depth rather than a live defect.
Risk Level: Low — adds one member and promotes one constructor-local to it. No logic, ordering, or
behavior change on any request path.
Testing:
New regression test
RouterUpstreamFilterTest.DynamicFilterKeepsConfigProviderManagerAlive.manager.expired()istrue)heap-use-after-free(below)//test/extensions/filters/http/composite:filter_testpasses (60/60), covering the composite change.AddressSanitizer against the unmodified code:
The container in frame #0 is
FilterConfigProviderManagerImplBase::subscriptions_.One detail stated precisely: the "freed by" frame in this trace is the test's own re-lookup of the
singleton, not the constructor. Because the manager is created via
make_shared, the object isdestructed when the constructor's local reference drops — running
~flat_hash_mapand releasing themap's internal buffer — while the enclosing block survives until
Singleton::ManagerImploverwritesits stale
weak_ptr. Production therefore hits use-after-destruction of the same member slightlyearlier than this trace's free point. Same dangling reference, same container, different moment.
Sanitizer runs were performed on macOS/arm64 with Apple clang, because
--config=asancurrentlyhardcodes an x86-64 Linux runtime path. Equivalent flags were used:
--copt=-fsanitize=address --copt=-fno-omit-frame-pointer --copt=-O1 --linkopt=-fsanitize=address --strip=never --dynamic_mode=off.Docs Changes: n/a
Release Notes: added
changelogs/current/bug_fixes/router__upstream-http-filter-config-provider-manager-lifetime.rstPlatform Specific Features: n/a
Per the generative AI policy:
this investigation, patch, and test were produced with AI assistance (Claude). I have reviewed and
understand the change.