[tor-commits] [tor/master] Track where microdescs are referenced to prevent free errs
nickm at torproject.org
nickm at torproject.org
Thu Jul 7 15:16:06 UTC 2011
commit 4ba9f3e31702020eaa336ccb33294568cda256eb
Author: Nick Mathewson <nickm at torproject.org>
Date: Thu May 12 11:10:35 2011 -0400
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.
---
changes/bug3153 | 5 +++++
src/or/microdesc.c | 41 ++++++++++++++++++++++++++++++++++++++++-
src/or/nodelist.c | 18 ++++++++++++++++--
src/or/or.h | 5 +++++
4 files changed, 66 insertions(+), 3 deletions(-)
diff --git a/changes/bug3153 b/changes/bug3153
new file mode 100644
index 0000000..8754b3d
--- /dev/null
+++ b/changes/bug3153
@@ -0,0 +1,5 @@
+ o Minor features:
+ - Check for and recover from inconsistency in the microdescriptor
+ cache. This will make it harder for us to accidentally free a
+ microdescriptor without removing it from the appropriate data
+ structures. Fixes issue 3135; issue noted by wanoskarnet.
diff --git a/src/or/microdesc.c b/src/or/microdesc.c
index 717488b..ba4532e 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 a736dc3..29bd7e0 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 f58876e..ddc03be 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;
More information about the tor-commits
mailing list