token_bucket_ctr: replace 32-bit wallclock time with monotime

This started as a response to ticket #40792 where Coverity is
complaining about a potential year 2038 bug where we cast time_t from
approx_time() to uint32_t for use in token_bucket_ctr.

There was a larger can of worms though, since token_bucket really
doesn't want to be using wallclock time here. I audited the call sites
for approx_time() and changed any that used a 32-bit cast or made
inappropriate use of wallclock time. Things like certificate lifetime,
consensus intervals, etc. need wallclock time. Measurements of rates
over time, however, are better served with a monotonic timer that does
not try and sync with wallclock ever.

Looking closer at token_bucket, its design is a bit odd because it was
initially intended for use with tick units but later forked into
token_bucket_rw which uses ticks to count bytes per second, and
token_bucket_ctr which uses seconds to count slower events. The rates
represented by either token bucket can't be lower than 1 per second, so
the slower timer in 'ctr' is necessary to represent the slower rates of
things like connections or introduction packets or rendezvous attempts.

I considered modifying token_bucket to use 64-bit timestamps overall
instead of 32-bit, but that seemed like an unnecessarily invasive change
that would grant some peace of mind but probably not help much. I was
more interested in removing the dependency on wallclock time. The
token_bucket_rw timer already uses monotonic time. This patch converts
token_bucket_ctr to use monotonic time as well. It introduces a new
monotime_coarse_absolute_sec(), which is currently the same as nsec
divided by a billion but could be optimized easily if we ever need to.

This patch also might fix a rollover bug.. I haven't tested this
extensively but I don't think the previous version of the rollover code
on either token bucket was correct, and I would expect it to get stuck
after the first rollover.

Signed-off-by: Micah Elizabeth Scott <beth@torproject.org>

2 files changed, 32 insertions, 1 deletions

diff --git a/src/lib/time/compat_time.c b/src/lib/time/compat_time.c
index eb716259c4..57a1e45b84 100644
--- a/src/lib/time/compat_time.c
+++ b/src/lib/time/compat_time.c
@@ -812,6 +812,12 @@ monotime_absolute_msec(void)
   return monotime_absolute_nsec() / ONE_MILLION;
 }
 
+uint64_t
+monotime_absolute_sec(void)
+{
+  return monotime_absolute_nsec() / ONE_BILLION;
+}
+
 #ifdef MONOTIME_COARSE_FN_IS_DIFFERENT
 uint64_t
 monotime_coarse_absolute_nsec(void)
@@ -836,6 +842,17 @@ monotime_coarse_absolute_msec(void)
 {
   return monotime_coarse_absolute_nsec() / ONE_MILLION;
 }
+
+uint64_t
+monotime_coarse_absolute_sec(void)
+{
+  /* Note: Right now I'm not too concerned about 64-bit division, but if this
+   * ever becomes a hotspot we need to optimize, we can modify this to grab
+   * tv_sec directly from CLOCK_MONOTONIC_COARSE on linux at least. Right now
+   * I'm choosing to make this simpler and easier to test, but this
+   * optimization is available easily if we need it. */
+  return monotime_coarse_absolute_nsec() / ONE_BILLION;
+}
 #else /* !defined(MONOTIME_COARSE_FN_IS_DIFFERENT) */
 #define initialized_at_coarse initialized_at
 #endif /* defined(MONOTIME_COARSE_FN_IS_DIFFERENT) */
diff --git a/src/lib/time/compat_time.h b/src/lib/time/compat_time.h
index da96023894..eaf676ae84 100644
--- a/src/lib/time/compat_time.h
+++ b/src/lib/time/compat_time.h
@@ -89,6 +89,13 @@
  * A: In general, regular monotime uses something that requires a system call.
  * On platforms where system calls are cheap, you win!  Otherwise, you lose.
  *
+ *   XXX: This hasn't been true for a long time. Expect both coarse and fine
+ *        monotime won't require a syscall, but they will have different
+ *        costs in terms of low-level synchronization inside the vDSO and
+ *        the hardware. The basic guidelines here still apply, but we aren't
+ *        really worrying about system calls any more, and the integer div
+ *        concerns are becoming nearly unimportant as well.
+ *
  * On Windows, monotonic time uses QuereyPerformanceCounter.  Storing
  * monotime_t costs 8 bytes.
  *
@@ -232,7 +239,12 @@ MOCK_DECL(uint64_t, monotime_absolute_usec,(void));
  * Fractional units are truncated, not rounded.
  */
 uint64_t monotime_absolute_msec(void);
-
+/**
+ * Return the number of seconds since the timer system was initialized.
+ * The returned value may be equal to zero.
+ * Fractional units are truncated, not rounded.
+ */
+uint64_t monotime_absolute_sec(void);
 /**
  * Set <b>out</b> to zero.
  */
@@ -259,11 +271,13 @@ void monotime_coarse_get(monotime_coarse_t *out);
 uint64_t monotime_coarse_absolute_nsec(void);
 uint64_t monotime_coarse_absolute_usec(void);
 uint64_t monotime_coarse_absolute_msec(void);
+uint64_t monotime_coarse_absolute_sec(void);
 #else /* !defined(MONOTIME_COARSE_FN_IS_DIFFERENT) */
 #define monotime_coarse_get monotime_get
 #define monotime_coarse_absolute_nsec monotime_absolute_nsec
 #define monotime_coarse_absolute_usec monotime_absolute_usec
 #define monotime_coarse_absolute_msec monotime_absolute_msec
+#define monotime_coarse_absolute_sec monotime_absolute_sec
 #endif /* defined(MONOTIME_COARSE_FN_IS_DIFFERENT) */
 
 /**