Track where microdescs are referenced to prevent free errs

On IRC, wanoskarnet notes that if we ever do microdesc_free() on a
microdesc that's in the nodelist, we're in trouble.  Also, we're in
trouble if we free one that's still in the microdesc_cache map.

This code adds a flag to microdesc_t to note where the microdesc is
referenced from, and checks those flags from microdesc_free().  I
don't believe we have any errors here now, but if we introduce some
later, let's log and recover from them rather than introducing
heisenbugs later on.

Addresses bug 3153.

3 files changed, 61 insertions, 3 deletions

diff --git a/src/or/microdesc.c b/src/or/microdesc.c
index 717488bded..ba4532ef64 100644
--- a/src/or/microdesc.c
+++ b/src/or/microdesc.c
@@ -238,6 +238,7 @@ microdescs_add_list_to_cache(microdesc_cache_t *cache,
     md->no_save = no_save;
 
     HT_INSERT(microdesc_map, &cache->map, md);
+    md->held_in_map = 1;
     smartlist_add(added, md);
     ++cache->n_seen;
     cache->total_len_seen += md->bodylen;
@@ -266,6 +267,7 @@ microdesc_cache_clear(microdesc_cache_t *cache)
   for (entry = HT_START(microdesc_map, &cache->map); entry; entry = next) {
     microdesc_t *md = *entry;
     next = HT_NEXT_RMV(microdesc_map, &cache->map, entry);
+    md->held_in_map = 0;
     microdesc_free(md);
   }
   HT_CLEAR(microdesc_map, &cache->map);
@@ -354,6 +356,7 @@ microdesc_cache_clean(microdesc_cache_t *cache, time_t cutoff, int force)
       ++dropped;
       victim = *mdp;
       mdp = HT_NEXT_RMV(microdesc_map, &cache->map, mdp);
+      victim->held_in_map = 0;
       bytes_dropped += victim->bodylen;
       microdesc_free(victim);
     } else {
@@ -502,7 +505,43 @@ microdesc_free(microdesc_t *md)
 {
   if (!md)
     return;
-  /* Must be removed from hash table! */
+
+  /* Make sure that the microdesc was really removed from the appropriate data
+     structures. */
+  if (md->held_in_map) {
+    microdesc_cache_t *cache = get_microdesc_cache();
+    microdesc_t *md2 = HT_FIND(microdesc_map, &cache->map, md);
+    if (md2 == md) {
+      log_warn(LD_BUG, "microdesc_free() called, but md was still in "
+               "microdesc_map");
+      HT_REMOVE(microdesc_map, &cache->map, md);
+    } else {
+      log_warn(LD_BUG, "microdesc_free() called with held_in_map set, but "
+               "microdesc was not in the map.");
+    }
+    tor_fragile_assert();
+  }
+  if (md->held_by_node) {
+    int found=0;
+    const smartlist_t *nodes = nodelist_get_list();
+    SMARTLIST_FOREACH(nodes, node_t *, node, {
+        if (node->md == md) {
+          ++found;
+          node->md = NULL;
+        }
+      });
+    if (found) {
+      log_warn(LD_BUG, "microdesc_free() called, but md was still referenced "
+               "%d node(s)", found);
+    } else {
+      log_warn(LD_BUG, "microdesc_free() called with held_by_node set, but "
+               "md was not refrenced by any nodes");
+    }
+    tor_fragile_assert();
+  }
+  //tor_assert(md->held_in_map == 0);
+  //tor_assert(md->held_by_node == 0);
+
   if (md->onion_pkey)
     crypto_free_pk_env(md->onion_pkey);
   if (md->body && md->saved_location != SAVED_IN_CACHE)
diff --git a/src/or/nodelist.c b/src/or/nodelist.c
index a736dc3b8c..29bd7e0d9b 100644
--- a/src/or/nodelist.c
+++ b/src/or/nodelist.c
@@ -158,8 +158,12 @@ nodelist_add_microdesc(microdesc_t *md)
   if (rs == NULL)
     return NULL;
   node = node_get_mutable_by_id(rs->identity_digest);
-  if (node)
+  if (node) {
+    if (node->md)
+      node->md->held_by_node = 0;
     node->md = md;
+    md->held_by_node = 1;
+  }
   return node;
 }
 
@@ -184,8 +188,12 @@ nodelist_set_consensus(networkstatus_t *ns)
     if (ns->flavor == FLAV_MICRODESC) {
       if (node->md == NULL ||
           0!=memcmp(node->md->digest,rs->descriptor_digest,DIGEST256_LEN)) {
+        if (node->md)
+          node->md->held_by_node = 0;
         node->md = microdesc_cache_lookup_by_digest256(NULL,
                                                        rs->descriptor_digest);
+        if (node->md)
+          node->md->held_by_node = 1;
       }
     }
 
@@ -240,8 +248,10 @@ void
 nodelist_remove_microdesc(const char *identity_digest, microdesc_t *md)
 {
   node_t *node = node_get_mutable_by_id(identity_digest);
-  if (node && node->md == md)
+  if (node && node->md == md) {
     node->md = NULL;
+    md->held_by_node = 0;
+  }
 }
 
 /** Tell the nodelist that <b>ri</b> is no longer in the routerlist. */
@@ -288,6 +298,8 @@ node_free(node_t *node)
 {
   if (!node)
     return;
+  if (node->md)
+    node->md->held_by_node = 0;
   tor_assert(node->nodelist_idx == -1);
   tor_free(node);
 }
@@ -373,6 +385,8 @@ nodelist_assert_ok(void)
         microdesc_t *md =
           microdesc_cache_lookup_by_digest256(NULL, rs->descriptor_digest);
         tor_assert(md == node->md);
+        if (md)
+          tor_assert(md->held_by_node == 1);
       }
     } SMARTLIST_FOREACH_END(rs);
   }
diff --git a/src/or/or.h b/src/or/or.h
index f58876e496..ddc03be87c 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -1709,6 +1709,11 @@ typedef struct microdesc_t {
   saved_location_t saved_location : 3;
   /** If true, do not attempt to cache this microdescriptor on disk. */
   unsigned int no_save : 1;
+  /** If true, this microdesc is attached to a node_t. */
+  unsigned int held_by_node : 1;
+  /** If true, this microdesc has an entry in the microdesc_map */
+  unsigned int held_in_map : 1;
+
   /** If saved_location == SAVED_IN_CACHE, this field holds the offset of the
    * microdescriptor in the cache. */
   off_t off;