commit 4ba9f3e31702020eaa336ccb33294568cda256eb Author: Nick Mathewson nickm@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;