MINOR: tasks: catch TICK_ETERNITY with BUG_ON() in __task_queue()

__task_queue() must absolutely not be called with TICK_ETERNITY or it
will place a never-expiring node upfront in the timers queue, preventing
any timer from expiring until the process is restarted. Code was found
to cause this using "task_schedule(task, now_ms)" which does this one
millisecond every 49.7 days, so let's add a condition against this. It
must never trigger since any process susceptible to trigger it would
already accumulate tasks until it dies.

An extra test was added in wake_expired_tasks() to detect tasks whose
timeout would have been changed after being queued.

An improvement over this could be in the future to use a non-scalar
type (union/struct) for expiration dates so as to avoid the risk of
using them directly like this. But now_ms is already such a valid
time and this specific construct would still not be caught.

This could even be backported to stable versions to help detect other
occurrences if any.

diff --git a/include/haproxy/task.h b/include/haproxy/task.h
index 2fe5f74af1a801d14953b8d3e3f7fed5bf6f1609..360fd87784f3419b381f5145dddd8eebcedb7de8 100644 (file)
--- a/include/haproxy/task.h
+++ b/include/haproxy/task.h
@@ -553,6 +553,10 @@ static inline void tasklet_set_tid(struct tasklet *tl, int tid)
 /* Ensure <task> will be woken up at most at <when>. If the task is already in
  * the run queue (but not running), nothing is done. It may be used that way
  * with a delay :  task_schedule(task, tick_add(now_ms, delay));
+ * It MUST NOT be used with a timer in the past, and even less with
+ * TICK_ETERNITY (which would block all timers). Note that passing it directly
+ * now_ms without using tick_add() will definitely make this happen once every
+ * 49.7 days.
  */
 static inline void task_schedule(struct task *task, int when)
 {

diff --git a/src/task.c b/src/task.c
index 7dc0cd41cfa9db413a14dcf0f9d2b5d02ad91431..7ce07dadc788770d73a55e12d97685dce76c2dfa 100644 (file)
--- a/src/task.c
+++ b/src/task.c
@@ -251,7 +251,7 @@ void __task_wakeup(struct task *t)
  *
  * Inserts a task into wait queue <wq> at the position given by its expiration
  * date. It does not matter if the task was already in the wait queue or not,
- * as it will be unlinked. The task must not have an infinite expiration timer.
+ * as it will be unlinked. The task MUST NOT have an infinite expiration timer.
  * Last, tasks must not be queued further than the end of the tree, which is
  * between <now_ms> and <now_ms> + 2^31 ms (now+24days in 32bit).
  *
@@ -268,6 +268,10 @@ void __task_queue(struct task *task, struct eb_root *wq)
               (wq == &sched->timers && (task->state & TASK_SHARED_WQ)) ||
               (wq != &timers && wq != &sched->timers));
 #endif
+       /* if this happens the process is doomed anyway, so better catch it now
+        * so that we have the caller in the stack.
+        */
+       BUG_ON(task->expire == TICK_ETERNITY);
 
        if (likely(task_in_wq(task)))
                __task_unlink_wq(task);
@@ -341,7 +345,8 @@ void wake_expired_tasks()
                                __task_queue(task, &tt->timers);
                }
                else {
-                       /* task not expired and correctly placed */
+                       /* task not expired and correctly placed. It may not be eternal. */
+                       BUG_ON(task->expire == TICK_ETERNITY);
                        break;
                }
        }
@@ -411,7 +416,8 @@ void wake_expired_tasks()
                        goto lookup_next;
                }
                else {
-                       /* task not expired and correctly placed */
+                       /* task not expired and correctly placed. It may not be eternal. */
+                       BUG_ON(task->expire == TICK_ETERNITY);
                        break;
                }
        }