Hi,
a problem with the current timer implementation in Click leads to execution of
unscheduled timers or access to already deleted timers, which can result in any
kind of unexpected behavior including segmentation faults at unrelated code
sections, which made it fun to debug :) Possible patch attached.
The problem occurs only under certain conditions:
1) A lot of timers (more than max_timers = 64) must be used that are
scheduled with close expiration times.
2) Timer handler functions delete (or unschedule) other timers.
3) High load helps to trigger this bug fast.
The problematic code section is in Master::run_timers(), right after the usual
handling of scheduled timers. There, all scheduled and expired timers are taken
out of the heap, stored in a vector and then run sequentially, while missing a
check whether the current timer is still alive (could be deleted/unscheduled by
one of the previously run timers).
Nadi
diff --git a/include/click/timer.hh b/include/click/timer.hh
index 4f4d692..a9ffa42 100644
--- a/include/click/timer.hh
+++ b/include/click/timer.hh
@@ -36,6 +36,8 @@ class Timer { public:
/** @brief Destroy a Timer, unscheduling it first if necessary. */
inline ~Timer() {
+ if (_alive)
+ *_alive = 0;
if (scheduled())
unschedule();
}
@@ -255,6 +257,7 @@ class Timer { public:
TimerCallback _hook;
void *_thunk;
Element *_owner;
+ char *_alive; /* 0: deleted, 1: scheduled, 2: unscheduled */
Timer(const Timer &x);
Timer &operator=(const Timer &x);
diff --git a/lib/master.cc b/lib/master.cc
index eedbc47..f936da6 100644
--- a/lib/master.cc
+++ b/lib/master.cc
@@ -518,22 +518,41 @@ Master::run_timers()
if (max_timers < 0 && !_stopper) {
Vector<Timer*> v;
v.reserve(32);
+ Vector<char*> av;
+ av.reserve(32);
do {
timer_reheapify_from(0, _timer_heap.back(), true);
t->_schedpos = -1;
_timer_heap.pop_back();
+ char* alive = new char(1);
+ assert(alive);
+ t->_alive = alive;
+ av.push_back(alive);
v.push_back(t);
} while (_timer_heap.size() > 0
&& (t = _timer_heap.at_u(0), t->_expiry <= _timer_check));
Vector<Timer*>::iterator i = v.begin();
- for (; !_stopper && i != v.end(); ++i)
- run_one_timer(*i);
+ Vector<char*>::iterator a = av.begin();
+ for (; !_stopper && i != v.end(); ++i, ++a) {
+ if (**a) {
+ (*i)->_alive = 0;
+ if (**a == 1)
+ run_one_timer(*i);
+ }
+ delete *a;
+ }
// reschedule unrun timers if stopped early
- for (; i != v.end(); ++i)
- (*i)->schedule_at((*i)->_expiry);
+ for (; i != v.end(); ++i, ++a) {
+ if (**a) {
+ (*i)->_alive = 0;
+ if (**a == 1)
+ (*i)->schedule_at((*i)->_expiry);
+ }
+ delete *a;
+ }
}
}
diff --git a/lib/timer.cc b/lib/timer.cc
index 95fb4b0..bd5e25c 100644
--- a/lib/timer.cc
+++ b/lib/timer.cc
@@ -192,22 +192,22 @@ Timer::task_hook(Timer *, void *thunk)
Timer::Timer()
- : _schedpos(-1), _hook(empty_hook), _thunk(0), _owner(0)
+ : _schedpos(-1), _hook(empty_hook), _thunk(0), _owner(0), _alive(0)
{
}
Timer::Timer(TimerCallback f, void *user_data)
- : _schedpos(-1), _hook(f), _thunk(user_data), _owner(0)
+ : _schedpos(-1), _hook(f), _thunk(user_data), _owner(0), _alive(0)
{
}
Timer::Timer(Element* element)
- : _schedpos(-1), _hook(element_hook), _thunk(element), _owner(0)
+ : _schedpos(-1), _hook(element_hook), _thunk(element), _owner(0), _alive(0)
{
}
Timer::Timer(Task* task)
- : _schedpos(-1), _hook(task_hook), _thunk(task), _owner(0)
+ : _schedpos(-1), _hook(task_hook), _thunk(task), _owner(0), _alive(0)
{
}
@@ -252,6 +252,8 @@ Timer::schedule_after(const Timestamp &delta)
void
Timer::unschedule()
{
+ if (_alive)
+ *_alive = 2;
if (!scheduled())
return;
Master* master = _owner->master();
_______________________________________________
click mailing list
[email protected]
https://amsterdam.lcs.mit.edu/mailman/listinfo/click
