- tor-commits - lists.torproject.org

[tor/master] Track where microdescs are referenced to prevent free errs
by nickm＠torproject.org 07 Jul '11

07 Jul '11

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

1 0

[tor/master] don't mark our descriptor dirty if our onion key hasn't changed
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit af0e8d834e4b1da029f7505745225f848e96def6 Author: Roger Dingledine <arma(a)torproject.org> Date: Sat May 21 19:23:27 2011 -0400 don't mark our descriptor dirty if our onion key hasn't changed --- changes/bug3263 | 4 ++++ src/or/router.c | 5 +++++ 2 files changed, 9 insertions(+), 0 deletions(-) diff --git a/changes/bug3263 b/changes/bug3263 new file mode 100644 index 0000000..43202c2 --- /dev/null +++ b/changes/bug3263 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Don't publish a new relay descriptor when we reload our onion key, + unless the onion key has actually changed. Fixes bug 3263 and + resolves another cause of bug 1810. Bugfix on 0.1.1.11-alpha. diff --git a/src/or/router.c b/src/or/router.c index 184715b..f6728aa 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -82,6 +82,11 @@ static authority_cert_t *legacy_key_certificate = NULL; static void set_onion_key(crypto_pk_env_t *k) { + if (onionkey && !crypto_pk_cmp_keys(onionkey, k)) { + /* k is already our onion key; free it and return */ + crypto_free_pk_env(k); + return; + } tor_mutex_acquire(key_lock); crypto_free_pk_env(onionkey); onionkey = k;

1 0

[tor/master] Have transitions in public_server_mode count as affects_descriptor
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit bc3c54a07f035c69b81cbf7817b8071938abf2ca Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 7 11:05:06 2011 -0400 Have transitions in public_server_mode count as affects_descriptor Previously, we'd get a new descriptor for free when public_server_mode() changed, since it would count as affects_workers, which would call init_keys(), which would make us regenerate a new descriptor. But now that we fixed bug 3263, init_keys() is no longer necessarily a new descriptor, and so we need to make sure that public_server_mode() counts as a descriptor transition. --- src/or/config.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 36fb991..4bdb038 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -3847,7 +3847,8 @@ options_transition_affects_descriptor(or_options_t *old_options, !opt_streq(old_options->ContactInfo, new_options->ContactInfo) || !opt_streq(old_options->MyFamily, new_options->MyFamily) || !opt_streq(old_options->AccountingStart, new_options->AccountingStart) || - old_options->AccountingMax != new_options->AccountingMax) + old_options->AccountingMax != new_options->AccountingMax || + public_server_mode(old_options) != public_server_mode(new_options)) return 1; return 0;

1 0

[tor/master] Merge branch 'bug3263'
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit 6b670d6032b33f255f88e31a727b8f042b1dfcef Merge: 586d0ab bc3c54a Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 7 11:08:03 2011 -0400 Merge branch 'bug3263' changes/bug3263 | 4 ++++ src/or/config.c | 3 ++- src/or/router.c | 5 +++++ 3 files changed, 11 insertions(+), 1 deletions(-)

1 0

[tor/master] oops; wrong bug number.
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit 586d0abe591e241124790b6ee0d273158e96bd0c Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 7 11:01:41 2011 -0400 oops; wrong bug number. --- changes/bug3403 | 3 +++ changes/bug3404 | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/changes/bug3403 b/changes/bug3403 new file mode 100644 index 0000000..7d051d9 --- /dev/null +++ b/changes/bug3403 @@ -0,0 +1,3 @@ + o Minor bugfixes: + - Fix a class of double-mark-for-close bugs when bufferevents + are enabled. Fixes bug 3403; bugfix on 0.2.3.1-alpha. diff --git a/changes/bug3404 b/changes/bug3404 deleted file mode 100644 index 4e2e21b..0000000 --- a/changes/bug3404 +++ /dev/null @@ -1,3 +0,0 @@ - o Minor bugfixes: - - Fix a class of double-mark-for-close bugs when bufferevents - are enabled. Fixes bug 3404; bugfix on 0.2.3.1-alpha.

1 0

[tor/master] Avoid double-free in bufferevent read/write cbs
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit 57822cbbbe85410785716fa62667b674b35602df Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 7 11:00:21 2011 -0400 Avoid double-free in bufferevent read/write cbs Fixes bug 3404; bugfix on 0.2.3.1-alpha. --- changes/bug3404 | 3 +++ src/or/connection.c | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/changes/bug3404 b/changes/bug3404 new file mode 100644 index 0000000..4e2e21b --- /dev/null +++ b/changes/bug3404 @@ -0,0 +1,3 @@ + o Minor bugfixes: + - Fix a class of double-mark-for-close bugs when bufferevents + are enabled. Fixes bug 3404; bugfix on 0.2.3.1-alpha. diff --git a/src/or/connection.c b/src/or/connection.c index e8969e0..c84ee04 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -2957,9 +2957,11 @@ connection_handle_read_cb(struct bufferevent *bufev, void *arg) { connection_t *conn = arg; (void) bufev; - if (!conn->marked_for_close) + if (!conn->marked_for_close) { if (connection_process_inbuf(conn, 1)<0) /* XXXX Always 1? */ - connection_mark_for_close(conn); + if (!conn->marked_for_close) + connection_mark_for_close(conn); + } } /** Callback: invoked whenever a bufferevent has written data. */ @@ -2969,7 +2971,8 @@ connection_handle_write_cb(struct bufferevent *bufev, void *arg) connection_t *conn = arg; struct evbuffer *output; if (connection_flushed_some(conn)<0) { - connection_mark_for_close(conn); + if (!conn->marked_for_close) + connection_mark_for_close(conn); return; }

1 0

[tor/master] Remove unused var in write_to_evbuffer_zlib
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit 31120ff692cba0c663a144ae9ece4b80ae8877a1 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 7 11:00:51 2011 -0400 Remove unused var in write_to_evbuffer_zlib --- src/or/buffers.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 8f5fb1e..7312749 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -2240,7 +2240,6 @@ write_to_evbuffer_zlib(struct evbuffer *buf, tor_zlib_state_t *state, int over = 0, n; struct evbuffer_iovec vec[1]; do { - int need_new_chunk = 0; { size_t cap = data_len / 4; if (cap < 128) @@ -2269,7 +2268,6 @@ write_to_evbuffer_zlib(struct evbuffer *buf, tor_zlib_state_t *state, if (avail) { /* Zlib says we need more room (ZLIB_BUF_FULL). Start a new chunk * automatically, whether were going to or not. */ - need_new_chunk = 1; } break; }

1 0

[tor/master] Merge remote-tracking branch 'origin/maint-0.2.2'
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit f883ec09b5b2f48cd4186658e0b92f1511358f21 Merge: eee942c 3ab0976 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 7 09:56:01 2011 -0400 Merge remote-tracking branch 'origin/maint-0.2.2' changes/bug3536 | 5 +++++ src/or/connection_edge.c | 3 ++- 2 files changed, 7 insertions(+), 1 deletions(-)

1 0

[tor/master] Correctly send a SUCCEEDED event for rdns requests
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit 3ab09763ced6d892cd72956e53f2dfb804ec0a7c Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Thu Jul 7 05:27:20 2011 +0200 Correctly send a SUCCEEDED event for rdns requests The issue was that we overlooked the possibility of reverse DNS success at the end of connection_ap_handshake_socks_resolved(). Issue discovered by katmagic, thanks! --- changes/bug3536 | 5 +++++ src/or/connection_edge.c | 3 ++- 2 files changed, 7 insertions(+), 1 deletions(-) diff --git a/changes/bug3536 b/changes/bug3536 new file mode 100644 index 0000000..d3cec13 --- /dev/null +++ b/changes/bug3536 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Send a SUCCEEDED stream event to the controller when a reverse + resolve succeeded. Fixes bug 3536; bugfix on 0.0.8pre1. Issue + discovered by katmagic. + diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 78b1b92..d4d7e1c 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2611,7 +2611,8 @@ connection_ap_handshake_socks_resolved(edge_connection_t *conn, } connection_ap_handshake_socks_reply(conn, buf, replylen, (answer_type == RESOLVED_TYPE_IPV4 || - answer_type == RESOLVED_TYPE_IPV6) ? + answer_type == RESOLVED_TYPE_IPV6 || + answer_type == RESOLVED_TYPE_HOSTNAME) ? 0 : END_STREAM_REASON_RESOLVEFAILED); }

1 0

[tor/maint-0.2.2] Correctly send a SUCCEEDED event for rdns requests
by nickm＠torproject.org 07 Jul '11

07 Jul '11

commit 3ab09763ced6d892cd72956e53f2dfb804ec0a7c Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Thu Jul 7 05:27:20 2011 +0200 Correctly send a SUCCEEDED event for rdns requests The issue was that we overlooked the possibility of reverse DNS success at the end of connection_ap_handshake_socks_resolved(). Issue discovered by katmagic, thanks! --- changes/bug3536 | 5 +++++ src/or/connection_edge.c | 3 ++- 2 files changed, 7 insertions(+), 1 deletions(-) diff --git a/changes/bug3536 b/changes/bug3536 new file mode 100644 index 0000000..d3cec13 --- /dev/null +++ b/changes/bug3536 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Send a SUCCEEDED stream event to the controller when a reverse + resolve succeeded. Fixes bug 3536; bugfix on 0.0.8pre1. Issue + discovered by katmagic. + diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 78b1b92..d4d7e1c 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2611,7 +2611,8 @@ connection_ap_handshake_socks_resolved(edge_connection_t *conn, } connection_ap_handshake_socks_reply(conn, buf, replylen, (answer_type == RESOLVED_TYPE_IPV4 || - answer_type == RESOLVED_TYPE_IPV6) ? + answer_type == RESOLVED_TYPE_IPV6 || + answer_type == RESOLVED_TYPE_HOSTNAME) ? 0 : END_STREAM_REASON_RESOLVEFAILED); }

1 0