Skip to content

Commit 6e6dcf3

Browse files
TAOXUYphlax
authored andcommitted
ProcessRateLimiter: fix use after free (#43325)
Commit Message: [listener_factory_context and access_logs](https://github.com/envoyproxy/envoy/blob/29a553cd459786eb6bece9b5dd2e0602e9b4b921/source/common/listener_manager/listener_impl.h#L468-L483) are both owned by ListenerImpl while listener_factory_context get destructed first so this PR just replace listener_factory_context with main_thread_dispatcher which is used in destructor Additional Description: Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: Signed-off-by: Xuyang Tao <taoxuy@google.com>
1 parent 7e6ab61 commit 6e6dcf3

3 files changed

Lines changed: 13 additions & 7 deletions

File tree

changelogs/current.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ bug_fixes:
2121
change: |
2222
Fixed a bug where the internal redirect logic may hang up a request when the request buffer is
2323
overflowed.
24+
- area: access_log
25+
change: |
26+
Fixed a crash on listener removal with a process-level access log rate limiter
27+
:ref:`ProcessRateLimitFilter <envoy_v3_api_msg_extensions.access_loggers.filters.process_ratelimit.v3.ProcessRateLimitFilter>`.
28+
29+
Note: Release notes show this to be fixed on 1.37.1 branch, but it was not backported in that release.
2430
2531
removed_config_or_runtime:
2632
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/extensions/access_loggers/filters/process_ratelimit/filter.cc

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ ProcessRateLimitFilter::ProcessRateLimitFilter(
1717
const envoy::extensions::access_loggers::filters::process_ratelimit::v3::ProcessRateLimitFilter&
1818
config)
1919
: setter_key_(reinterpret_cast<intptr_t>(this)),
20-
cancel_cb_(std::make_shared<std::atomic<bool>>(false)), context_(context),
20+
cancel_cb_(std::make_shared<std::atomic<bool>>(false)),
21+
main_thread_dispatcher_(context.serverFactoryContext().mainThreadDispatcher()),
2122
stats_({ALL_PROCESS_RATELIMIT_FILTER_STATS(POOL_COUNTER_PREFIX(
2223
context.serverFactoryContext().scope(), "access_log.process_ratelimit."))}) {
2324
auto setter =
@@ -44,11 +45,10 @@ ProcessRateLimitFilter::~ProcessRateLimitFilter() {
4445
// The `cancel_cb_` is set to true to prevent the `limiter` from being set in
4546
// the `setter` from the main thread.
4647
cancel_cb_->store(true);
47-
context_.serverFactoryContext().mainThreadDispatcher().post(
48-
[limiter = std::move(rate_limiter_), setter_key = setter_key_] {
49-
// remove the setter for this filter.
50-
limiter->getSubscription()->removeSetter(setter_key);
51-
});
48+
main_thread_dispatcher_.post([limiter = std::move(rate_limiter_), setter_key = setter_key_] {
49+
// remove the setter for this filter.
50+
limiter->getSubscription()->removeSetter(setter_key);
51+
});
5252
}
5353

5454
bool ProcessRateLimitFilter::evaluate(const Formatter::Context&,

source/extensions/access_loggers/filters/process_ratelimit/filter.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class ProcessRateLimitFilter : public AccessLog::Filter {
3737
private:
3838
const intptr_t setter_key_;
3939
std::shared_ptr<std::atomic<bool>> cancel_cb_;
40-
Server::Configuration::GenericFactoryContext& context_;
40+
Event::Dispatcher& main_thread_dispatcher_;
4141
ProcessRateLimitFilterStats stats_;
4242
mutable Envoy::Extensions::Filters::Common::LocalRateLimit::RateLimiterProviderSingleton::
4343
RateLimiterWrapperPtr rate_limiter_;

0 commit comments

Comments
 (0)