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>

1 files changed, 4 insertions, 2 deletions

diff --git a/src/core/or/dos.c b/src/core/or/dos.c
index 11a0edcc6a..8e0fe9e641 100644
--- a/src/core/or/dos.c
+++ b/src/core/or/dos.c
@@ -21,6 +21,7 @@
 #include "feature/relay/routermode.h"
 #include "feature/stats/geoip_stats.h"
 #include "lib/crypt_ops/crypto_rand.h"
+#include "lib/time/compat_time.h"
 
 #include "core/or/dos.h"
 #include "core/or/dos_sys.h"
@@ -528,7 +529,8 @@ conn_update_on_connect(conn_client_stats_t *stats, const tor_addr_t *addr)
   stats->concurrent_count++;
 
   /* Refill connect connection count. */
-  token_bucket_ctr_refill(&stats->connect_count, (uint32_t) approx_time());
+  token_bucket_ctr_refill(&stats->connect_count,
+                          (uint32_t) monotime_coarse_absolute_sec());
 
   /* Decrement counter for this new connection. */
   if (token_bucket_ctr_get(&stats->connect_count) > 0) {
@@ -808,7 +810,7 @@ dos_geoip_entry_init(clientmap_entry_t *geoip_ent)
    * can be enabled at runtime and these counters need to be valid. */
   token_bucket_ctr_init(&geoip_ent->dos_stats.conn_stats.connect_count,
                         dos_conn_connect_rate, dos_conn_connect_burst,
-                        (uint32_t) approx_time());
+                        (uint32_t) monotime_coarse_absolute_sec());
 }
 
 /** Note that the given channel has sent outbound the maximum amount of cell