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.
This commit is contained in:
Nick Mathewson 2011-05-12 11:10:35 -04:00
parent 3968e8d14b
commit 4ba9f3e317
4 changed files with 66 additions and 3 deletions

5
changes/bug3153 Normal file
View File

@ -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.

View File

@ -238,6 +238,7 @@ microdescs_add_list_to_cache(microdesc_cache_t *cache,
md->no_save = no_save; md->no_save = no_save;
HT_INSERT(microdesc_map, &cache->map, md); HT_INSERT(microdesc_map, &cache->map, md);
md->held_in_map = 1;
smartlist_add(added, md); smartlist_add(added, md);
++cache->n_seen; ++cache->n_seen;
cache->total_len_seen += md->bodylen; 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) { for (entry = HT_START(microdesc_map, &cache->map); entry; entry = next) {
microdesc_t *md = *entry; microdesc_t *md = *entry;
next = HT_NEXT_RMV(microdesc_map, &cache->map, entry); next = HT_NEXT_RMV(microdesc_map, &cache->map, entry);
md->held_in_map = 0;
microdesc_free(md); microdesc_free(md);
} }
HT_CLEAR(microdesc_map, &cache->map); HT_CLEAR(microdesc_map, &cache->map);
@ -354,6 +356,7 @@ microdesc_cache_clean(microdesc_cache_t *cache, time_t cutoff, int force)
++dropped; ++dropped;
victim = *mdp; victim = *mdp;
mdp = HT_NEXT_RMV(microdesc_map, &cache->map, mdp); mdp = HT_NEXT_RMV(microdesc_map, &cache->map, mdp);
victim->held_in_map = 0;
bytes_dropped += victim->bodylen; bytes_dropped += victim->bodylen;
microdesc_free(victim); microdesc_free(victim);
} else { } else {
@ -502,7 +505,43 @@ microdesc_free(microdesc_t *md)
{ {
if (!md) if (!md)
return; 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) if (md->onion_pkey)
crypto_free_pk_env(md->onion_pkey); crypto_free_pk_env(md->onion_pkey);
if (md->body && md->saved_location != SAVED_IN_CACHE) if (md->body && md->saved_location != SAVED_IN_CACHE)

View File

@ -158,8 +158,12 @@ nodelist_add_microdesc(microdesc_t *md)
if (rs == NULL) if (rs == NULL)
return NULL; return NULL;
node = node_get_mutable_by_id(rs->identity_digest); 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; node->md = md;
md->held_by_node = 1;
}
return node; return node;
} }
@ -184,8 +188,12 @@ nodelist_set_consensus(networkstatus_t *ns)
if (ns->flavor == FLAV_MICRODESC) { if (ns->flavor == FLAV_MICRODESC) {
if (node->md == NULL || if (node->md == NULL ||
0!=memcmp(node->md->digest,rs->descriptor_digest,DIGEST256_LEN)) { 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, node->md = microdesc_cache_lookup_by_digest256(NULL,
rs->descriptor_digest); 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) nodelist_remove_microdesc(const char *identity_digest, microdesc_t *md)
{ {
node_t *node = node_get_mutable_by_id(identity_digest); node_t *node = node_get_mutable_by_id(identity_digest);
if (node && node->md == md) if (node && node->md == md) {
node->md = NULL; node->md = NULL;
md->held_by_node = 0;
}
} }
/** Tell the nodelist that <b>ri</b> is no longer in the routerlist. */ /** Tell the nodelist that <b>ri</b> is no longer in the routerlist. */
@ -288,6 +298,8 @@ node_free(node_t *node)
{ {
if (!node) if (!node)
return; return;
if (node->md)
node->md->held_by_node = 0;
tor_assert(node->nodelist_idx == -1); tor_assert(node->nodelist_idx == -1);
tor_free(node); tor_free(node);
} }
@ -373,6 +385,8 @@ nodelist_assert_ok(void)
microdesc_t *md = microdesc_t *md =
microdesc_cache_lookup_by_digest256(NULL, rs->descriptor_digest); microdesc_cache_lookup_by_digest256(NULL, rs->descriptor_digest);
tor_assert(md == node->md); tor_assert(md == node->md);
if (md)
tor_assert(md->held_by_node == 1);
} }
} SMARTLIST_FOREACH_END(rs); } SMARTLIST_FOREACH_END(rs);
} }

View File

@ -1709,6 +1709,11 @@ typedef struct microdesc_t {
saved_location_t saved_location : 3; saved_location_t saved_location : 3;
/** If true, do not attempt to cache this microdescriptor on disk. */ /** If true, do not attempt to cache this microdescriptor on disk. */
unsigned int no_save : 1; 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 /** If saved_location == SAVED_IN_CACHE, this field holds the offset of the
* microdescriptor in the cache. */ * microdescriptor in the cache. */
off_t off; off_t off;