[tor-commits] [tor/master] sched: Don't cast to int32_t the monotime_diff_msec() result

[nickm at torproject.org]

commit 77cc97cf0a20ed0a062a1cb87bef6c40941e4cff
Author: David Goulet <dgoulet at torproject.org>
Date:   Mon Sep 18 10:47:05 2017 -0400

    sched: Don't cast to int32_t the monotime_diff_msec() result
    
    When the KIST schedule() is called, it computes a diff value between the last
    scheduler run and the current monotonic time. If tha value is below the run
    interval, the libevent even is updated else the event is run.
    
    It turned out that casting to int32_t the returned int64_t value for the very
    first scheduler run (which is set to 0) was creating an overflow on the 32 bit
    value leading to adding the event with a gigantic usec value. The scheduler
    was simply never running for a while.
    
    First of all, a BUG() is added for a diff value < 0 because if the time is
    really monotonic, we should never have a now time that is lower than the last
    scheduler run time. And we will try to recover with a diff value to 0.
    
    Second, the diff value is changed to int64_t so we avoid this "bootstrap
    overflow" and future casting overflow problems.
    
    Fixes #23558
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 src/or/scheduler_kist.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c
index ba8d416db..e4ae07107 100644
--- a/src/or/scheduler_kist.c
+++ b/src/or/scheduler_kist.c
@@ -507,16 +507,25 @@ kist_scheduler_schedule(void)
 {
   struct monotime_t now;
   struct timeval next_run;
-  int32_t diff;
+  int64_t diff;
 
   if (!have_work()) {
     return;
   }
   monotime_get(&now);
-  diff = (int32_t) monotime_diff_msec(&scheduler_last_run, &now);
+
+  /* If time is really monotonic, we can never have now being smaller than the
+   * last scheduler run. The scheduler_last_run at first is set to 0. */
+  diff = monotime_diff_msec(&scheduler_last_run, &now);
+  IF_BUG_ONCE(diff < 0) {
+    diff = 0;
+  }
   if (diff < sched_run_interval) {
     next_run.tv_sec = 0;
-    /* 1000 for ms -> us */
+    /* Takes 1000 ms -> us. This will always be valid because diff can NOT be
+     * negative and can NOT be smaller than sched_run_interval so values can
+     * only go from 1000 usec (diff set to interval - 1) to 100000 usec (diff
+     * set to 0) for the maximum allowed run interval (100ms). */
     next_run.tv_usec = (sched_run_interval - diff) * 1000;
     /* Readding an event reschedules it. It does not duplicate it. */
     scheduler_ev_add(&next_run);