Clamp every elapsed time span at zero, using a set of helper functions

13 files changed, 187 insertions, 31 deletions

diff --git a/Mk/tests.mk b/Mk/tests.mk
index 2efdd72..22ea69e 100644
--- a/Mk/tests.mk
+++ b/Mk/tests.mk
@@ -4,8 +4,8 @@ BUILDDEPS+=Mk/tests.mk
 # may reach a static function by including the source. tests/stubs.c supplies
 # the few symbols those modules reference but the tests never reach.
 TESTBIN=tests/runner
-TEST_SOURCES=tests/runner.c tests/test.c tests/stubs.c tests/t_urlencoded.c tests/t_iovec.c tests/t_vector.c tests/t_random.c tests/t_hex.c tests/t_accesslist.c tests/t_scan.c tests/t_buf.c tests/t_ip.c tests/t_socket.c tests/t_mutex.c tests/t_trackerlogic.c tests/t_workqueue.c tests/t_batch.c tests/t_fdstate.c tests/t_event.c tests/t_fmt.c
-TEST_UNITS=src/ot_urlencoded.c src/ot_fmt.c src/ot_event.c src/ot_batch.c src/ot_fdstate.c src/ot_buf.c src/ot_ip.c src/ot_socket.c src/ot_iovec.c src/ot_vector.c src/ot_random.c src/ot_hex.c src/ot_scan.c src/ot_accesslist.c src/ot_mutex.c src/ot_workqueue.c src/trackerlogic.c
+TEST_SOURCES=tests/runner.c tests/test.c tests/stubs.c tests/t_urlencoded.c tests/t_iovec.c tests/t_vector.c tests/t_random.c tests/t_hex.c tests/t_accesslist.c tests/t_scan.c tests/t_buf.c tests/t_ip.c tests/t_socket.c tests/t_mutex.c tests/t_trackerlogic.c tests/t_clean.c tests/t_workqueue.c tests/t_batch.c tests/t_fdstate.c tests/t_event.c tests/t_fmt.c
+TEST_UNITS=src/ot_urlencoded.c src/ot_fmt.c src/ot_event.c src/ot_batch.c src/ot_fdstate.c src/ot_buf.c src/ot_ip.c src/ot_socket.c src/ot_iovec.c src/ot_vector.c src/ot_random.c src/ot_hex.c src/ot_scan.c src/ot_accesslist.c src/ot_mutex.c src/ot_workqueue.c src/ot_clean.c src/trackerlogic.c
 TEST_HEADERS=tests/test.h $(HEADERS)
 
 # A test binary is not a production binary, so it is built with the features
diff --git a/src/opentracker.c b/src/opentracker.c
index 60a6d8c..8de3fc1 100644
--- a/src/opentracker.c
+++ b/src/opentracker.c
@@ -151,7 +151,7 @@ static void *server_mainloop(void *args) {
     while ((sock = workqueue_popresult(&iovec_entries, &iovector, &is_partial)) != -1)
       http_sendiovecdata(sock, &ws, iovec_entries, iovector, is_partial);
 
-    if (time_now_seconds() > next_timeout_check) {
+    if (time_is_past(next_timeout_check)) {
       int cursor = 0;
 
       while ((sock = fdstate_next_expired(time_now_seconds(), &cursor)) != -1)
diff --git a/src/ot_accesslist.c b/src/ot_accesslist.c
index 86f9a7d..ea5bdba 100644
--- a/src/ot_accesslist.c
+++ b/src/ot_accesslist.c
@@ -91,7 +91,7 @@ static ot_accesslist *accesslist_make(ot_accesslist *next, size_t size) {
    This will never delete head, because that might still be in use. */
 static void accesslist_clean(ot_accesslist *accesslist) {
   while (accesslist && accesslist->next) {
-    if (accesslist->next->base + 5 < time_now_minutes())
+    if (time_since_minutes(accesslist->next->base) > 5)
       accesslist->next = accesslist_free(accesslist->next);
     accesslist = accesslist->next;
   }
diff --git a/src/ot_clean.c b/src/ot_clean.c
index f181e83..2ebfd42 100644
--- a/src/ot_clean.c
+++ b/src/ot_clean.c
@@ -45,7 +45,7 @@ static ssize_t clean_single_bucket(ot_peer *peers, size_t peer_count, size_t pee
 
 int clean_single_peer_list(ot_peerlist *peer_list, size_t peer_size) {
   ot_vector *peer_vector = &peer_list->peers;
-  time_t     timedout    = (time_t)(time_now_minutes() - peer_list->base);
+  ot_time    timedout    = time_since_minutes(peer_list->base);
   size_t     num_buckets = 1, removed_seeders = 0;
 
   /* No need to clean empty torrent */
diff --git a/src/ot_http.c b/src/ot_http.c
index 96b8f4e..533a876 100644
--- a/src/ot_http.c
+++ b/src/ot_http.c
@@ -414,7 +414,7 @@ static ssize_t http_handle_fullscrape(const int64_t sock, struct ot_workstruct *
       pthread_mutex_unlock(&g_modest_fullscrape_mutex);
       HTTPERROR_500;
     }
-    if (exactmatch && (this_peer.last_fullscrape - new_peer->last_fullscrape) < OT_MODEST_PEER_TIMEOUT) {
+    if (exactmatch && time_since_seconds(new_peer->last_fullscrape) < OT_MODEST_PEER_TIMEOUT) {
       pthread_mutex_unlock(&g_modest_fullscrape_mutex);
       HTTPERROR_402_NOTMODEST;
     }
diff --git a/src/ot_livesync.c b/src/ot_livesync.c
index 85577f7..7d7208e 100644
--- a/src/ot_livesync.c
+++ b/src/ot_livesync.c
@@ -178,13 +178,13 @@ void livesync_ticker(void) {
 
   /* livesync_issue_peersync sets g_next_packet_time */
   pthread_mutex_lock(&g_outbuf_mutex);
-  if (time_now_seconds() > g_v6_buf.next_packet_time && g_v6_buf.fill > sizeof(g_tracker_id) + sizeof(uint32_t))
+  if (time_is_past(g_v6_buf.next_packet_time) && g_v6_buf.fill > sizeof(g_tracker_id) + sizeof(uint32_t))
     livesync_issue_peersync(&g_v6_buf);
   else
     pthread_mutex_unlock(&g_outbuf_mutex);
 
   pthread_mutex_lock(&g_outbuf_mutex);
-  if (time_now_seconds() > g_v4_buf.next_packet_time && g_v4_buf.fill > sizeof(g_tracker_id) + sizeof(uint32_t))
+  if (time_is_past(g_v4_buf.next_packet_time) && g_v4_buf.fill > sizeof(g_tracker_id) + sizeof(uint32_t))
     livesync_issue_peersync(&g_v4_buf);
   else
     pthread_mutex_unlock(&g_outbuf_mutex);
diff --git a/src/ot_stats.c b/src/ot_stats.c
index 2d6335b..7b5b1b9 100644
--- a/src/ot_stats.c
+++ b/src/ot_stats.c
@@ -373,7 +373,7 @@ void stats_top_txt(char **reply, const char *reply_end, size_t amount) {
 static unsigned long events_per_time(unsigned long long events, time_t t) { return events / ((unsigned int)t ? (unsigned int)t : 1); }
 
 static void        stats_connections_mrtg(char **reply, const char *reply_end) {
-  ot_time t = time(NULL) - ot_start_time;
+  ot_time t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -386,7 +386,7 @@ static void        stats_connections_mrtg(char **reply, const char *reply_end) {
 }
 
 static void stats_udpconnections_mrtg(char **reply, const char *reply_end) {
-  ot_time t = time(NULL) - ot_start_time;
+  ot_time t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -398,7 +398,7 @@ static void stats_udpconnections_mrtg(char **reply, const char *reply_end) {
 }
 
 static void stats_tcpconnections_mrtg(char **reply, const char *reply_end) {
-  time_t t = time(NULL) - ot_start_time;
+  time_t t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -409,7 +409,7 @@ static void stats_tcpconnections_mrtg(char **reply, const char *reply_end) {
 }
 
 static void stats_scrape_mrtg(char **reply, const char *reply_end) {
-  time_t t = time(NULL) - ot_start_time;
+  time_t t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -420,7 +420,7 @@ static void stats_scrape_mrtg(char **reply, const char *reply_end) {
 }
 
 static void stats_fullscrapes_mrtg(char **reply, const char *reply_end) {
-  ot_time t = time(NULL) - ot_start_time;
+  ot_time t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -468,7 +468,7 @@ static void stats_return_renew_bucket(char **reply, const char *reply_end) {
 }
 
 static void stats_return_sync_mrtg(char **reply, const char *reply_end) {
-  ot_time t = time(NULL) - ot_start_time;
+  ot_time t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -479,7 +479,7 @@ static void stats_return_sync_mrtg(char **reply, const char *reply_end) {
 }
 
 static void stats_return_completed_mrtg(char **reply, const char *reply_end) {
-  ot_time t = time(NULL) - ot_start_time;
+  ot_time t = time_since_seconds(ot_start_time);
   unsigned long long total[OT_COUNTER_COUNT];
 
   counters_total(total);
@@ -510,7 +510,7 @@ static void stats_return_everything(char **reply, const char *reply_end) {
 
   fmt_append(reply, reply_end, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n");
   fmt_append(reply, reply_end, "<stats>\n");
-  fmt_append(reply, reply_end, "  <uptime>%llu</uptime>\n", (unsigned long long)(time(NULL) - ot_start_time));
+  fmt_append(reply, reply_end, "  <uptime>%llu</uptime>\n", (unsigned long long)time_since_seconds(ot_start_time));
   fmt_append(reply, reply_end, "  <torrents>\n");
   fmt_append(reply, reply_end, "    <count_mutex>%zd</count_mutex>\n", mutex_get_torrent_count());
   fmt_append(reply, reply_end, "    <count_iterator>%llu</count_iterator>\n", stats.torrent_count);
@@ -785,7 +785,7 @@ void stats_issue_event(struct ot_workstruct *ws, ot_status_event event, PROTO_FL
   case EVENT_FULLSCRAPE_REQUEST: {
     ot_ip6 *ip = (ot_ip6 *)event_data; /* ugly hack to transfer ip to stats */
     char    _debug[512];
-    int     off  = snprintf(_debug, sizeof(_debug), "[%08d] scrp:  ", (unsigned int)(time_now_seconds() - ot_start_time) / 60);
+    int     off  = snprintf(_debug, sizeof(_debug), "[%08d] scrp:  ", (unsigned int)time_since_seconds(ot_start_time) / 60);
     off         += ip_format(_debug + off, *ip);
     off         += snprintf(_debug + off, sizeof(_debug) - off, " - FULL SCRAPE\n");
     (void)write(2, _debug, off);
@@ -794,7 +794,7 @@ void stats_issue_event(struct ot_workstruct *ws, ot_status_event event, PROTO_FL
   case EVENT_FULLSCRAPE_REQUEST_GZIP: {
     ot_ip6 *ip = (ot_ip6 *)event_data; /* ugly hack to transfer ip to stats */
     char    _debug[512];
-    int     off  = snprintf(_debug, sizeof(_debug), "[%08d] scrp:  ", (unsigned int)(time_now_seconds() - ot_start_time) / 60);
+    int     off  = snprintf(_debug, sizeof(_debug), "[%08d] scrp:  ", (unsigned int)time_since_seconds(ot_start_time) / 60);
     off         += ip_format(_debug + off, *ip);
     off         += snprintf(_debug + off, sizeof(_debug) - off, " - FULL SCRAPE\n");
     (void)write(2, _debug, off);
@@ -804,7 +804,8 @@ void stats_issue_event(struct ot_workstruct *ws, ot_status_event event, PROTO_FL
     COUNT(OT_COUNTER_FAILED + event_data, 1);
     break;
   case EVENT_RENEW:
-    COUNT(OT_COUNTER_RENEWED + event_data, 1);
+    if (event_data < OT_PEER_TIMEOUT)
+      COUNT(OT_COUNTER_RENEWED + event_data, 1);
     break;
   case EVENT_SYNC:
     COUNT(OT_COUNTER_SYNCS, event_data);
diff --git a/src/ot_udp.c b/src/ot_udp.c
index 644b71d..e67ca1f 100644
--- a/src/ot_udp.c
+++ b/src/ot_udp.c
@@ -39,12 +39,12 @@ static void udp_make_connectionid(uint32_t connid[2], const ot_ip6 remoteip, int
   int      i;
   uint32_t current_key_of_the_hour;
 
-  if (time_now_minutes() - g_hour_of_the_key >= 60) {
+  if (time_since_minutes(g_hour_of_the_key) >= 60) {
     uint32_t old_key_of_the_hour = g_key_of_the_hour[0];
     uint32_t new_key_of_the_hour = random_uint32();
     /* If in the meantime another thread has performed
        key rotation, do not overwrite their results */
-    if (time_now_minutes() - g_hour_of_the_key >= 60) {
+    if (time_since_minutes(g_hour_of_the_key) >= 60) {
       /* Upgrade en bloc */
       g_hour_of_the_key    = time_now_minutes();
       g_key_of_the_hour[0] = new_key_of_the_hour;
diff --git a/src/trackerlogic.h b/src/trackerlogic.h
index 692b6af..c8d859c 100644
--- a/src/trackerlogic.h
+++ b/src/trackerlogic.h
@@ -61,10 +61,10 @@ typedef struct {
 #define OT_PEER_TIMEOUT               45
 
 /* We maintain a list of 1024 pointers to sorted list of ot_torrent structs
- Sort key is, of course, its hash */
-/* Set from the Makefile, 8 to 16. More buckets means shorter vectors and a
-   cheaper insert, which only pays for a tracker carrying millions of
-   torrents; see DESIGN. */
+ Sort key is, of course, its hash.
+ Can be tweked from the Makefile, sensible values are between 8 in 16. More
+ buckets means shorter vectors and a cheaper insert, which only pays for a
+ tracker carrying millions of torrents. Consult the DESIGN document. */
 #ifndef OT_BUCKET_COUNT_BITS
 #define OT_BUCKET_COUNT_BITS          10
 #endif
@@ -95,6 +95,22 @@ static inline ot_time time_now_minutes(void) { return time_now_seconds() / 60; }
 /* For the deadlines the tracker sets on itself, which read better as the
    span they allow than as an addition to the current second */
 static inline ot_time time_future_seconds(int span) { return time_now_seconds() + span; }
+
+/* How long ago a stamp was taken. */
+_Static_assert((ot_time)-1 < 0, "ot_time has to be signed for time_since_* to clamp");
+
+static inline ot_time time_since_seconds(ot_time then) {
+  ot_time now = time_now_seconds();
+  return now > then ? now - then : 0;
+}
+
+static inline ot_time time_since_minutes(ot_time then) {
+  ot_time now = time_now_minutes();
+  return now > then ? now - then : 0;
+}
+
+/* Has a deadline the tracker set on itself gone by? */
+static inline int time_is_past(ot_time deadline) { return time_now_seconds() > deadline; }
 /* Cleared from the interrupt handler and read by every worker */
 extern _Atomic int    g_opentracker_running;
 
diff --git a/tests/runner.c b/tests/runner.c
index ae2565b..2398991 100644
--- a/tests/runner.c
+++ b/tests/runner.c
@@ -27,6 +27,7 @@ static const struct module modules[] = {{"ot_urlencoded", test_urlencoded},
                                         {"ot_socket", test_socket},
                                         {"ot_mutex", test_mutex},
                                         {"trackerlogic", test_trackerlogic},
+                                        {"clean", test_clean},
                                         {"ot_workqueue", test_workqueue},
                                         {"ot_batch", test_batch},
                                         {"ot_fdstate", test_fdstate},
diff --git a/tests/stubs.c b/tests/stubs.c
index f7ac679..fd47a68 100644
--- a/tests/stubs.c
+++ b/tests/stubs.c
@@ -26,14 +26,11 @@ long io_trywrite(long d, const char *buf, long len) {
 /* trackerlogic.c is under test, and starting it up starts every module it
    leans on. None of them is driven by these tests, so they are answered here
    rather than linked in and left to run threads of their own. */
-void clean_init(void) {}
-void clean_deinit(void) {}
-int  clean_single_torrent(ot_torrent *torrent) {
-  (void)torrent;
-  return 0;
-}
+/* ot_clean.c is in the unit build for t_clean, so its thread control is the
+   real thing and only the stats hook it calls is stubbed here. */
 void fullscrape_init(void) {}
 void fullscrape_deinit(void) {}
+void stats_cleanup(void) {}
 void livesync_init(void) {}
 void livesync_deinit(void) {}
 void livesync_tell(struct ot_workstruct *ws) { (void)ws; }
diff --git a/tests/t_clean.c b/tests/t_clean.c
new file mode 100644
index 0000000..7eca9aa
--- /dev/null
+++ b/tests/t_clean.c
@@ -0,0 +1,140 @@
+/* This software was written by Dirk Engling <erdgeist@erdgeist.org>
+   It is considered beerware. Prost. Skol. Cheers or whatever. */
+
+#include <arpa/inet.h>
+#include <signal.h>
+#include <pthread.h>
+#include <string.h>
+
+#include "ot_clean.h"
+#include "ot_mutex.h"
+#include "trackerlogic.h"
+#include "test.h"
+
+/* A clock stepped backwards used to leave peer_list->base in the future, so
+   the span the cleaner works from came out negative. It was then added to a
+   peer's age and stored in the single byte that age lives in, where -1 reads
+   as 255 and the next pass threw the whole swarm away. */
+
+static void announce(struct ot_workstruct *ws, ot_hash *hash, uint8_t last_octet, int seeding) {
+  static const uint8_t v4mapped[12] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff};
+
+  /* The peer list is written into the reply, which workstruct_init leaves
+     for its caller to point somewhere */
+  ws->reply = ws->outbuf;
+
+  memset(&ws->peer, 0, sizeof(ws->peer));
+  memcpy(&ws->peer, v4mapped, sizeof(v4mapped));
+  ((uint8_t *)&ws->peer)[15] = last_octet;
+  OT_SETPORT(&ws->peer, &(uint16_t){6881});
+  if (seeding)
+    OT_PEERFLAG(ws->peer) |= PEER_FLAG_SEEDING;
+  ws->hash = hash;
+  add_peer_to_torrent_and_return_peers(FLAG_TCP, ws, 0);
+}
+
+static size_t peers_in(ot_hash *hash) {
+  char   reply[1024];
+  size_t len = return_udp_scrape_for_torrent(*hash, reply);
+  uint32_t seeders, leechers;
+
+  if (len < 12)
+    return 0;
+  memcpy(&seeders, reply, 4);
+  memcpy(&leechers, reply + 8, 4);
+  return (size_t)ntohl(seeders) + (size_t)ntohl(leechers);
+}
+
+static ot_torrent *torrent_of(ot_hash *hash) {
+  ot_vector  *bucket = mutex_bucket_lock_by_hash(*hash);
+  ot_torrent *t      = (ot_torrent *)bucket->data;
+  ot_torrent *found  = NULL;
+  size_t      i;
+
+  for (i = 0; i < bucket->size; ++i)
+    if (!memcmp(t[i].hash, hash, sizeof(ot_hash)))
+      found = t + i;
+  mutex_bucket_unlock_by_hash(*hash, 0);
+  return found;
+}
+
+/* What a clock stepped back by `minutes` leaves behind */
+static void step_clock_back(ot_torrent *torrent, int minutes) {
+  torrent->peer_list6->base += minutes;
+  torrent->peer_list4->base += minutes;
+}
+
+/* What the next minute of a well behaved clock looks like */
+static void age_by_a_minute(ot_torrent *torrent) {
+  torrent->peer_list6->base -= 1;
+  torrent->peer_list4->base -= 1;
+}
+
+static void case_a_clock_that_went_back_keeps_the_swarm(void) {
+  struct ot_workstruct ws;
+  ot_hash              hash;
+  ot_torrent          *torrent;
+
+  test_case("a swarm survives the clock being stepped backwards");
+  memset(hash, 0x31, sizeof(hash));
+  CHECK_INT(workstruct_init(&ws, G_INBUF_SIZE), 0);
+
+  announce(&ws, &hash, 1, 1);
+  announce(&ws, &hash, 2, 0);
+  CHECK_INT(peers_in(&hash), 2);
+
+  torrent = torrent_of(&hash);
+  CHECK(torrent != NULL);
+  step_clock_back(torrent, 5);
+
+  /* The pass that sees the step keeps everybody either way: unclamped it
+     writes a negative span into each peer's age byte, where it reads as 251
+     rather than as a removal. */
+  CHECK_INT(clean_single_torrent(torrent), 0);
+  CHECK_INT(peers_in(&hash), 2);
+
+  /* A minute later, and this is where a poisoned age byte took the swarm:
+     251 plus one minute is past any timeout. */
+  age_by_a_minute(torrent);
+  CHECK_INT(clean_single_torrent(torrent), 0);
+  CHECK_INT(peers_in(&hash), 2);
+
+  workstruct_deinit(&ws);
+}
+
+static void case_an_idle_swarm_still_ages_out(void) {
+  struct ot_workstruct ws;
+  ot_hash              hash;
+  ot_torrent          *torrent;
+
+  test_case("a swarm nobody touched is still cleaned away");
+  memset(hash, 0x32, sizeof(hash));
+  CHECK_INT(workstruct_init(&ws, G_INBUF_SIZE), 0);
+
+  announce(&ws, &hash, 3, 1);
+  CHECK_INT(peers_in(&hash), 1);
+
+  torrent = torrent_of(&hash);
+  CHECK(torrent != NULL);
+  step_clock_back(torrent, -OT_PEER_TIMEOUT);
+
+  CHECK_INT(clean_single_torrent(torrent), 0);
+  CHECK_INT(peers_in(&hash), 0);
+
+  workstruct_deinit(&ws);
+}
+
+void test_clean(void) {
+  sigset_t hup;
+
+  /* accesslist_deinit sends SIGHUP to its reader, trackerlogic_init starts
+     the accesslist, and the test process must not take it */
+  sigemptyset(&hup);
+  sigaddset(&hup, SIGHUP);
+  pthread_sigmask(SIG_BLOCK, &hup, NULL);
+
+  trackerlogic_init();
+  case_a_clock_that_went_back_keeps_the_swarm();
+  case_an_idle_swarm_still_ages_out();
+  trackerlogic_deinit();
+}
diff --git a/tests/test.h b/tests/test.h
index 82ec178..9b569c7 100644
--- a/tests/test.h
+++ b/tests/test.h
@@ -86,6 +86,7 @@ void test_ip(void);
 void test_socket(void);
 void test_mutex(void);
 void test_trackerlogic(void);
+void test_clean(void);
 void test_workqueue(void);
 void test_batch(void);
 void test_fdstate(void);