scaled range timer: guard against queue deletion during timer fire (envoyproxy#14799)

Fix a potential use-after-free error in ScaledRangeTimerManagerImpl by adding a processing_timers_ flag to the timer queues that is set during onQueueTimerFired processing. This flag is checked when a timer is removed to ensure that the timer's queue isn't deleted while it is in a callback triggered by onQueueTimerFired.

Signed-off-by: Craig Radcliffe <[email protected]>

Author: Craig Radcliffe

source/common/event/scaled_range_timer_manager_impl.cc

@@ -225,7 +225,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;
   }
 
@@ -255,12 +259,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.

source/common/event/scaled_range_timer_manager_impl.h

@@ -68,6 +68,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};
   };
 
   /**

test/common/event/scaled_range_timer_manager_impl_test.cc

@@ -157,6 +157,60 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWhileScalingMax) {
   simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block);
 }
 
+TEST_F(ScaledRangeTimerManagerTest, InCallbackDisableLastTimerInSameQueue) {
+  ScaledRangeTimerManagerImpl manager(dispatcher_);
+
+  MockFunction<TimerCb> callback1;
+  auto timer1 =
+      manager.createTimer(AbsoluteMinimum(std::chrono::seconds(0)), callback1.AsStdFunction());
+  MockFunction<TimerCb> 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<TimerCb> callback1;
+  auto timer1 =
+      manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback1.AsStdFunction());
+  MockFunction<TimerCb> 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_);