From 824c486ec1548af9bbfd9ad442b4393088609d26 Mon Sep 17 00:00:00 2001
From: Roger Dingledine <arma@torproject.org>
Date: Tue, 9 Aug 2022 09:24:29 -0400
Subject: [PATCH] debugging / bulletproofing for #40637

---
 src/core/or/dos.c               | 34 +++++++++++++++++++++++++++++++--
 src/core/or/dos.h               |  1 +
 src/feature/stats/geoip_stats.c |  2 +-
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/src/core/or/dos.c b/src/core/or/dos.c
index 2eb5782481..47e0ef35a3 100644
--- a/src/core/or/dos.c
+++ b/src/core/or/dos.c
@@ -718,8 +718,7 @@ dos_geoip_entry_about_to_free(const clientmap_entry_t *geoip_ent)
   SMARTLIST_FOREACH_BEGIN(get_connection_array(), connection_t *, conn) {
     if (conn->type == CONN_TYPE_OR) {
       or_connection_t *or_conn = TO_OR_CONN(conn);
-      if (!tor_addr_compare(&geoip_ent->addr, &TO_CONN(or_conn)->addr,
-                            CMP_EXACT)) {
+      if (!tor_addr_compare(&geoip_ent->addr, &conn->addr, CMP_EXACT)) {
         or_conn->tracked_for_dos_mitigation = 0;
       }
     }
@@ -729,6 +728,37 @@ dos_geoip_entry_about_to_free(const clientmap_entry_t *geoip_ent)
   return;
 }
 
+/** Return true iff we think there are established connections from <b>ent</b>
+ * at this moment. Else return false if there aren't any. Used for deciding
+ * whether it's time to free a clientmap stats entry. */
+bool
+dos_geoip_any_existing_conns_on_clientmap(struct clientmap_entry_t *ent)
+{
+  if (!dos_enabled() || !ent->dos_stats.conn_stats.concurrent_count) {
+    goto end; /* all set to clean+free it */
+  }
+
+  /* We could just return 1 here, because concurrent_count is non-zero.
+   * But for bullet-proofing we do the expensive checks below. */
+  SMARTLIST_FOREACH_BEGIN(get_connection_array(), const connection_t *, conn) {
+    if (conn->type == CONN_TYPE_OR) {
+      const or_connection_t *or_conn = CONST_TO_OR_CONN(conn);
+      if (tor_addr_eq(&ent->addr, &conn->addr) &&
+          or_conn->tracked_for_dos_mitigation) {
+        return true;
+      }
+    }
+  } SMARTLIST_FOREACH_END(conn);
+
+  /* If we get here, it means we have a bug: concurrent_count was non-zero
+   * but we couldn't find any conns that were being tracked! */
+  log_warn(LD_BUG, "When cleaning clientmap, we expected %d conns "
+                   "but we found none.",
+           ent->dos_stats.conn_stats.concurrent_count);
+ end:
+  return false;
+}
+
 /** A new geoip client entry has been allocated, initialize its DoS object. */
 void
 dos_geoip_entry_init(clientmap_entry_t *geoip_ent)
diff --git a/src/core/or/dos.h b/src/core/or/dos.h
index 178ec11096..71385cad6e 100644
--- a/src/core/or/dos.h
+++ b/src/core/or/dos.h
@@ -78,6 +78,7 @@ int dos_enabled(void);
 void dos_log_heartbeat(void);
 void dos_geoip_entry_init(struct clientmap_entry_t *geoip_ent);
 void dos_geoip_entry_about_to_free(const struct clientmap_entry_t *geoip_ent);
+bool dos_geoip_any_existing_conns_on_clientmap(struct clientmap_entry_t *ent);
 
 void dos_new_client_conn(or_connection_t *or_conn,
                          const char *transport_name);
diff --git a/src/feature/stats/geoip_stats.c b/src/feature/stats/geoip_stats.c
index 16b360d624..47972ceba2 100644
--- a/src/feature/stats/geoip_stats.c
+++ b/src/feature/stats/geoip_stats.c
@@ -338,7 +338,7 @@ static int
 remove_old_client_helper_(struct clientmap_entry_t *ent, void *_cutoff)
 {
   if (!client_entry_is_still_fresh(ent, *(time_t*)_cutoff) &&
-      (!dos_enabled() || !ent->dos_stats.conn_stats.concurrent_count)) {
+      !dos_geoip_any_existing_conns_on_clientmap(ent)) {
     clientmap_entry_free(ent);
     return 1;
   }