diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 97285daa7e90b..28bf35fa34413 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,7 @@ Bug Fixes * aggregate cluster: fixed a crash due to a TLS initialization issue. * lua: fixed crash when Lua script contains streamInfo():downstreamSslConnection(). +* overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. * tls: fix detection of the upstream connection close event. Removed Config or Runtime diff --git a/source/common/event/scaled_range_timer_manager.cc b/source/common/event/scaled_range_timer_manager.cc index 10ac02b713cda..e90b0cc59c919 100644 --- a/source/common/event/scaled_range_timer_manager.cc +++ b/source/common/event/scaled_range_timer_manager.cc @@ -208,7 +208,11 @@ void ScaledRangeTimerManager::removeTimer(ScalingTimerHandle handle) { handle.queue_.range_timers_.erase(handle.iterator_); // Don't keep around empty queues if (handle.queue_.range_timers_.empty()) { - queues_.erase(handle.queue_); + // Skip erasing the queue if we're in the middle of processing timers for the queue. The + // queue will be erased in `onQueueTimerFired` after the queue entries have been processed. + if (!handle.queue_.processing_timers_) { + queues_.erase(handle.queue_); + } return; } @@ -238,12 +242,14 @@ void ScaledRangeTimerManager::onQueueTimerFired(Queue& queue) { // Pop and trigger timers until the one at the front isn't supposed to have expired yet (given the // current scale factor). + queue.processing_timers_ = true; while (!timers.empty() && computeTriggerTime(timers.front(), queue.duration_, scale_factor_) <= now) { auto item = std::move(queue.range_timers_.front()); queue.range_timers_.pop_front(); item.timer_.trigger(); } + queue.processing_timers_ = false; if (queue.range_timers_.empty()) { // Maintain the invariant that queues are never empty. diff --git a/source/common/event/scaled_range_timer_manager.h b/source/common/event/scaled_range_timer_manager.h index 1fbd51c8c86ee..db24c5a7d556d 100644 --- a/source/common/event/scaled_range_timer_manager.h +++ b/source/common/event/scaled_range_timer_manager.h @@ -81,6 +81,10 @@ class ScaledRangeTimerManager { // 2) on expiration // 3) when the scale factor changes const TimerPtr timer_; + + // A flag indicating whether the queue is currently processing timers. Used to guard against + // queue deletion during timer processing. + bool processing_timers_{false}; }; /** diff --git a/test/common/event/scaled_range_timer_manager_test.cc b/test/common/event/scaled_range_timer_manager_test.cc index ff9bcab087726..f0a6d2edab3f2 100644 --- a/test/common/event/scaled_range_timer_manager_test.cc +++ b/test/common/event/scaled_range_timer_manager_test.cc @@ -147,6 +147,56 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWhileScalingMax) { simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); } +TEST_F(ScaledRangeTimerManagerTest, InCallbackDisableLastTimerInSameQueue) { + ScaledRangeTimerManager manager(dispatcher_); + + MockFunction callback1; + auto timer1 = manager.createTimer(callback1.AsStdFunction()); + MockFunction callback2; + auto timer2 = manager.createTimer(callback2.AsStdFunction()); + + timer1->enableTimer(std::chrono::seconds(0), std::chrono::seconds(95)); + timer2->enableTimer(std::chrono::seconds(5), std::chrono::seconds(100)); + + simTime().advanceTimeAndRun(std::chrono::seconds(5), dispatcher_, Dispatcher::RunType::Block); + + EXPECT_TRUE(timer1->enabled()); + EXPECT_TRUE(timer2->enabled()); + + EXPECT_CALL(callback1, Call).WillOnce(Invoke([&]() { + timer2->disableTimer(); + timer2.reset(); + })); + + // Run the dispatcher to make sure nothing happens when it's not supposed to. + simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); +} + +TEST_F(ScaledRangeTimerManagerTest, InCallbackDisableTimerInOtherQueue) { + ScaledRangeTimerManager manager(dispatcher_); + + MockFunction callback1; + auto timer1 = manager.createTimer(callback1.AsStdFunction()); + MockFunction callback2; + auto timer2 = manager.createTimer(callback2.AsStdFunction()); + + timer1->enableTimer(std::chrono::seconds(5), std::chrono::seconds(95)); + timer2->enableTimer(std::chrono::seconds(5), std::chrono::seconds(100)); + + simTime().advanceTimeAndRun(std::chrono::seconds(5), dispatcher_, Dispatcher::RunType::Block); + + EXPECT_TRUE(timer1->enabled()); + EXPECT_TRUE(timer2->enabled()); + + EXPECT_CALL(callback1, Call).WillOnce(Invoke([&]() { + timer2->disableTimer(); + timer2.reset(); + })); + + // Run the dispatcher to make sure nothing happens when it's not supposed to. + simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); +} + TEST_F(ScaledRangeTimerManagerTest, DisableWithZeroMinTime) { ScaledRangeTimerManager manager(dispatcher_);