backport 1.16: scaled range timer: guard against queue deletion during timer fire (#…

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

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.

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};
   };
 
   /**

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<TimerCb> callback1;
+  auto timer1 = manager.createTimer(callback1.AsStdFunction());
+  MockFunction<TimerCb> 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<TimerCb> callback1;
+  auto timer1 = manager.createTimer(callback1.AsStdFunction());
+  MockFunction<TimerCb> 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_);