diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 769fa75f82e54..0a1de40e6e03d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -13,6 +13,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. + Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/event/scaled_range_timer_manager_impl.cc b/source/common/event/scaled_range_timer_manager_impl.cc index 6aab81486929c..e4dc2cec2d8c1 100644 --- a/source/common/event/scaled_range_timer_manager_impl.cc +++ b/source/common/event/scaled_range_timer_manager_impl.cc @@ -212,7 +212,11 @@ void ScaledRangeTimerManagerImpl::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; } @@ -242,12 +246,14 @@ void ScaledRangeTimerManagerImpl::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_impl.h b/source/common/event/scaled_range_timer_manager_impl.h index f9bcc7242b7d1..7c3673d9da284 100644 --- a/source/common/event/scaled_range_timer_manager_impl.h +++ b/source/common/event/scaled_range_timer_manager_impl.h @@ -63,6 +63,10 @@ class ScaledRangeTimerManagerImpl : public 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_impl_test.cc b/test/common/event/scaled_range_timer_manager_impl_test.cc index 824a285ee4f61..c6f7476c8af5b 100644 --- a/test/common/event/scaled_range_timer_manager_impl_test.cc +++ b/test/common/event/scaled_range_timer_manager_impl_test.cc @@ -151,6 +151,60 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWhileScalingMax) { simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); } +TEST_F(ScaledRangeTimerManagerTest, InCallbackDisableLastTimerInSameQueue) { + ScaledRangeTimerManagerImpl manager(dispatcher_); + + MockFunction callback1; + auto timer1 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(0)), callback1.AsStdFunction()); + MockFunction callback2; + auto timer2 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback2.AsStdFunction()); + + timer1->enableTimer(std::chrono::seconds(95)); + timer2->enableTimer(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) { + ScaledRangeTimerManagerImpl manager(dispatcher_); + + MockFunction callback1; + auto timer1 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback1.AsStdFunction()); + MockFunction callback2; + auto timer2 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback2.AsStdFunction()); + + timer1->enableTimer(std::chrono::seconds(95)); + timer2->enableTimer(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) { ScaledRangeTimerManagerImpl manager(dispatcher_);