debugging / bulletproofing for #40637

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,
+      if (!tor_addr_compare(&geoip_ent->addr, &conn->addr, CMP_EXACT)) {
-                            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)

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);

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;
   }