scaled range timer: guard against queue deletion during timer fire by chradcliffe · Pull Request #14799 · envoyproxy/envoy

chradcliffe · 2021-01-22T19:41:54Z

Commit Message:
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.

Additional Description: N/A
Risk Level: Low - the issue does not appear possible in current Envoy code
Testing: unit test that reproduces the issue under ASAN
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Issue #14798

…ssing a callback Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

Add 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 <craig.radcliffe@broadcom.com>

chradcliffe · 2021-01-22T19:42:05Z

cc @akonradi

akonradi · 2021-01-22T19:43:30Z

Sweet, thanks for the fix!

akonradi · 2021-01-22T20:52:02Z

test/common/event/scaled_range_timer_manager_impl_test.cc

+  timer1->enableTimer(std::chrono::seconds(100));
+  timer2->enableTimer(std::chrono::seconds(100));


Since these timers are enabled for the same duration, they can fire in either order. Consider making timer1 fire in 95 seconds with an absolute minimum of 0.

We need both timers to end up in the same queues so their durations should be the same.

I think the solution is to do an advance of 5 seconds between the first enable and the second enable.

Either of those will work. The duration that matters is (max - min) which is (100 - 5) or (95 - 0) as adjusted.

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

antoniovicente · 2021-01-22T21:04:45Z

source/common/event/scaled_range_timer_manager_impl.cc

    queue.range_timers_.pop_front();
    item.timer_.trigger();
  }
+  queue.processing_timers_ = false;


So processing_timers_ needs to be per queue since otherwise the queue not empty invariant would not hold if a timer from queue A removes a timer from queue B. It would be good to also cover that case in tests.

Also, is there code somewhere that verifies the invariant that all active queues are non-empty?

Also, is there code somewhere that verifies the invariant that all active queues are non-empty?

I don't believe there is currently. The number of queues and sizes is not currently exposed. I'm okay to keep this PR focused on the bugfix and add additional testing in a follow-on.

023d9ec adds a test for cancelling a timer from a different queue in the context of a timer callback.

cc: @asraa

@akonradi Please do followup. Regarding ASSERTs, I think failing this one will result in a crash in the next line.

envoy/source/common/event/scaled_range_timer_manager_impl.cc

Line 244 in bf84a7b

ASSERT(!queue.range_timers_.empty());

…ent queue Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

antoniovicente

Thanks for reporting this bug and providing a fix.

I would like to keep the issue you filed open until we manage to followup on some of the test improvements mentioned in various review comments.

chradcliffe · 2021-01-23T14:23:37Z

Does this make sense as a candidate for a backport to 1.17? That is where I originally discovered this issue.

antoniovicente · 2021-01-23T18:17:45Z

Tagged for backport.

@envoyproxy/senior-maintainers could one of you review the PR?

antoniovicente · 2021-01-25T19:57:09Z

ping @envoyproxy/senior-maintainers for review since previous mention was over the weekend.

…nvoyproxy#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 <craig.radcliffe@broadcom.com> Signed-off-by: Shikugawa <rei@tetrate.io>

Signed-off-by: Alex Konradi <akonradi@google.com>

The bug is triggered when a scaled timer callback disables another scaled timer, which was causing the shared queue to be deleted before being referenced again by the triggered timer. Signed-off-by: Alex Konradi <akonradi@google.com>

…g timer fire (#14799) (#15007) 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 <craig.radcliffe@broadcom.com> Signed-off-by: Shikugawa <rei@tetrate.io>

…g timer fire (#14799) (#15006) 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 <craig.radcliffe@broadcom.com> Signed-off-by: Shikugawa <rei@tetrate.io>

Craig Radcliffe added 2 commits January 22, 2021 12:55

Adding unit test that reproduces the issue with disabling while proce…

6666db7

…ssing a callback Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

akonradi previously approved these changes Jan 22, 2021

View reviewed changes

akonradi reviewed Jan 22, 2021

View reviewed changes

antoniovicente assigned akonradi and antoniovicente Jan 22, 2021

antoniovicente self-requested a review January 22, 2021 20:55

Adjusting the test timers so they have a guaranteed firing order

bf84a7b

Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

chradcliffe dismissed akonradi’s stale review via bf84a7b January 22, 2021 20:56

antoniovicente reviewed Jan 22, 2021

View reviewed changes

Adding test for cancellation of timer in callback for timer in differ…

023d9ec

…ent queue Signed-off-by: Craig Radcliffe <craig.radcliffe@broadcom.com>

antoniovicente mentioned this pull request Jan 22, 2021

scaled range timer manager: potential use-after-free when disabling timer during another timer's callback #14798

Closed

antoniovicente approved these changes Jan 22, 2021

View reviewed changes

antoniovicente added the backport/review Request to backport to stable releases label Jan 23, 2021

antoniovicente assigned akonradi, ggreenway and antoniovicente and unassigned akonradi and antoniovicente Jan 27, 2021

ggreenway approved these changes Jan 27, 2021

View reviewed changes

ggreenway merged commit f5468fa into envoyproxy:main Jan 27, 2021

chradcliffe deleted the chradcliffe/scaled-range-timer branch January 27, 2021 17:47

chradcliffe restored the chradcliffe/scaled-range-timer branch January 27, 2021 17:47

Shikugawa added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Feb 1, 2021

akonradi added a commit to akonradi/envoy that referenced this pull request Feb 10, 2021

Add release notes for envoyproxy#14799

9c6eeee

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi mentioned this pull request Feb 10, 2021

docs: add release notes for #14799 #15018

Merged

		timer1->enableTimer(std::chrono::seconds(100));
		timer2->enableTimer(std::chrono::seconds(100));

Conversation

chradcliffe commented Jan 22, 2021 • edited by antoniovicente Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chradcliffe commented Jan 22, 2021

Uh oh!

akonradi commented Jan 22, 2021

Uh oh!

akonradi Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

akonradi Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

akonradi Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

chradcliffe Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

chradcliffe commented Jan 23, 2021

Uh oh!

antoniovicente commented Jan 23, 2021

Uh oh!

antoniovicente commented Jan 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chradcliffe commented Jan 22, 2021 •

edited by antoniovicente

Loading