April 2011 - tor-commits - lists.torproject.org

[tor/master] Look at the right errno when sending reason for connect() failure
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit 6a5b94de6c51478ca638ec03c40a6d3c27f2d674 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 25 16:14:42 2011 -0400 Look at the right errno when sending reason for connect() failure In afe414 (tor-0.1.0.1-rc~173), when we moved to connection_edge_end_errno(), we used it in handling errors from connection_connect(). That's not so good, since by the time connection_connect() returns, the socket is no longer set, and we're supposed to be looking at the socket_errno return value from connection_connect() instead. So do what we should've done, and look at the socket_errno value that we get from connection_connect(). --- changes/connect_err_reporting | 6 ++++++ src/or/connection_edge.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/changes/connect_err_reporting b/changes/connect_err_reporting new file mode 100644 index 0000000..61a46b6 --- /dev/null +++ b/changes/connect_err_reporting @@ -0,0 +1,6 @@ + o Minor bugfixes: + - Be more careful about reporting the correct error from a failed + connect() operation. Under some circumstances, it was possible to + look at an incorrect value for errno when sending the end reason. + Bugfix on Tor-0.1.0.1-rc. + diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 82b7139..c75c06b 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2823,13 +2823,13 @@ connection_exit_connect(edge_connection_t *edge_conn) log_debug(LD_EXIT,"about to try connecting"); switch (connection_connect(conn, conn->address, addr, port, &socket_error)) { - case -1: - /* XXX022 use socket_error below rather than trying to piece things - * together from the current errno, which may have been clobbered. */ - connection_edge_end_errno(edge_conn); + case -1: { + int reason = errno_to_stream_end_reason(socket_error); + connection_edge_end(edge_conn, reason); circuit_detach_stream(circuit_get_by_edge_conn(edge_conn), edge_conn); connection_free(conn); return; + } case 0: conn->state = EXIT_CONN_STATE_CONNECTING;

1 0

[tor/master] Comment out ancient asserts for bug 930; resolve an xxx021
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit c4bd067359b07f6a57adf9d444d2a46e514043ff Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 25 15:44:02 2011 -0400 Comment out ancient asserts for bug 930; resolve an xxx021 --- src/common/memarea.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/common/memarea.c b/src/common/memarea.c index 6893639..a6b8c4e 100644 --- a/src/common/memarea.c +++ b/src/common/memarea.c @@ -59,7 +59,9 @@ realign_pointer(void *ptr) { uintptr_t x = (uintptr_t)ptr; x = (x+MEMAREA_ALIGN_MASK) & ~MEMAREA_ALIGN_MASK; - tor_assert(((void*)x) >= ptr); // XXXX021 remove this once bug 930 is solved + /* Reinstate this if bug 930 ever reappears + tor_assert(((void*)x) >= ptr); + */ return (void*)x; } @@ -241,9 +243,10 @@ memarea_alloc(memarea_t *area, size_t sz) } result = chunk->next_mem; chunk->next_mem = chunk->next_mem + sz; - // XXXX021 remove these once bug 930 is solved. + /* Reinstate these if bug 930 ever comes back tor_assert(chunk->next_mem >= chunk->u.mem); tor_assert(chunk->next_mem <= chunk->u.mem+chunk->mem_size); + */ chunk->next_mem = realign_pointer(chunk->next_mem); return result; }

1 0

[tor/maint-0.2.2] Improve a few comments
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit 9facf8918f959cc9d9c2674afbd0630718460dc3 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Mon Mar 28 19:28:04 2011 +0200 Improve a few comments --- src/common/address.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/address.c b/src/common/address.c index a089260..adc0ef0 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -51,11 +51,11 @@ /** Convert the tor_addr_t in a, with port in port, into a * sockaddr object in *sa_out of object size len. If not enough - * room is available in sa_out, or on error, return 0 On success, return + * room is available in sa_out, or on error, return 0. On success, return * the length of the sockaddr. * - * Interface note: ordinarly, we return -1 for error. We can't do that here, - * since socklen is unsigned on some platforms. + * Interface note: ordinarily, we return -1 for error. We can't do that here, + * since socklen_t is unsigned on some platforms. **/ socklen_t tor_addr_to_sockaddr(const tor_addr_t *a,

1 0

[tor/maint-0.2.2] Merge remote-tracking branch 'public/xxx_fixups' into maint-0.2.2
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit ba0cd8094f8e6ae0113ad69958d9d0973bb1f2c3 Merge: 118d8ff fc64783 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Apr 7 12:03:04 2011 -0400 Merge remote-tracking branch 'public/xxx_fixups' into maint-0.2.2 Conflicts: src/or/or.h changes/bug539_removal | 6 +++ changes/connect_err_reporting | 6 +++ changes/count_overflow | 5 +++ changes/full_ap_circuits | 6 +++ changes/kill_ftime | 7 ++++ changes/noroute | 5 +++ src/common/address.c | 20 ++++++----- src/common/compat_libevent.c | 17 +++------ src/common/memarea.c | 7 +++- src/common/util.c | 77 ----------------------------------------- src/common/util.h | 10 ----- src/or/buffers.c | 6 ++-- src/or/circuitbuild.c | 9 +++-- src/or/circuitlist.c | 3 ++ src/or/circuituse.c | 5 ++- src/or/config.c | 6 ++-- src/or/connection.c | 36 ++++++++++++------- src/or/connection_edge.c | 30 +++++++++++----- src/or/connection_or.c | 6 +++- src/or/directory.c | 37 ++++++++------------ src/or/dirserv.c | 11 ++++-- src/or/dns.c | 2 +- src/or/dnsserv.c | 2 +- src/or/eventdns.c | 2 +- src/or/geoip.c | 5 ++- src/or/networkstatus.c | 11 ++++-- src/or/or.h | 12 +++++-- src/or/reasons.c | 8 +---- src/or/relay.c | 2 + src/or/rendclient.c | 4 +- src/or/rendcommon.c | 2 +- src/or/rephist.c | 2 +- src/or/routerlist.c | 18 +++++----- src/or/routerparse.c | 2 +- 34 files changed, 184 insertions(+), 203 deletions(-) diff --cc src/or/or.h index 3cadd31,e44c626..1688a08 --- a/src/or/or.h +++ b/src/or/or.h @@@ -2126,9 -2126,16 +2126,15 @@@ typedef struct circuit_t * length ONIONSKIN_CHALLENGE_LEN. */ char *n_conn_onionskin; - time_t timestamp_created; /**< When was this circuit created? */ + struct timeval timestamp_created; /**< When was the circuit created? */ - time_t timestamp_dirty; /**< When the circuit was first used, or 0 if the - * circuit is clean. */ + /** When the circuit was first used, or 0 if the circuit is clean. + * + * XXXX023 Note that some code will artifically adjust this value backward + * in time in order to indicate that a circuit shouldn't be used for new + * streams, but that it can stay alive as long as it has streams on it. + * That's a kludge we should fix. + */ + time_t timestamp_dirty; - struct timeval highres_created; /**< When exactly was the circuit created? */ uint16_t marked_for_close; /**< Should we close this circuit at the end of * the main loop? (If true, holds the line number

1 0

[tor/maint-0.2.2] Add a missing cast to silence the compiler
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit 8d81831d715adf9cb374100e23cc2b118efe3101 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Mon Mar 28 18:20:50 2011 +0200 Add a missing cast to silence the compiler --- src/or/connection.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 084237d..6e7bbd5 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -2790,7 +2790,7 @@ connection_handle_write_impl(connection_t *conn, int force) edge_connection_t *edge_conn = TO_EDGE_CONN(conn); /* Check for overflow: */ if (PREDICT_LIKELY(UINT32_MAX - edge_conn->n_written > n_written)) - edge_conn->n_written += n_written; + edge_conn->n_written += (int)n_written; else edge_conn->n_written = UINT32_MAX; }

1 0

[tor/maint-0.2.2] Send END_STREAM_REASON_NOROUTE: clients that didn't grok it are now obsolete
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit fc647832783cab352bebba63fe0210d7be395058 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 28 13:42:59 2011 -0400 Send END_STREAM_REASON_NOROUTE: clients that didn't grok it are now obsolete --- changes/noroute | 5 +++++ src/or/reasons.c | 8 +------- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/changes/noroute b/changes/noroute new file mode 100644 index 0000000..644deec --- /dev/null +++ b/changes/noroute @@ -0,0 +1,5 @@ + - Minor features + - Send END_STREAM_REASON_NOROUTE in response to EHOSTUNREACH errors. + Clients before 0.2.1.27 didn't handle NOROUTE correctly, but + such clients are already deprecated because of security bugs. + diff --git a/src/or/reasons.c b/src/or/reasons.c index 27c947e..319e6c0 100644 --- a/src/or/reasons.c +++ b/src/or/reasons.c @@ -174,13 +174,7 @@ errno_to_stream_end_reason(int e) S_CASE(ENETUNREACH): return END_STREAM_REASON_INTERNAL; S_CASE(EHOSTUNREACH): - /* XXXX023 - * The correct behavior is END_STREAM_REASON_NOROUTE, but older - * clients don't recognize it. So we're going to continue sending - * "MISC" until 0.2.1.27 or later is "well established". - */ - /* return END_STREAM_REASON_NOROUTE; */ - return END_STREAM_REASON_MISC; + return END_STREAM_REASON_NOROUTE; S_CASE(ECONNREFUSED): return END_STREAM_REASON_CONNECTREFUSED; S_CASE(ECONNRESET):

1 0

[tor/maint-0.2.2] Clean up a comment-conversation about bad libevent version/method combos
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit 88bb40d8f87077e2f954b6de779ac52c637c69ec Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 25 15:32:45 2011 -0400 Clean up a comment-conversation about bad libevent version/method combos --- src/common/compat_libevent.c | 17 ++++++----------- 1 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index 3efc56d..3ad9be1 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -330,17 +330,12 @@ tor_check_libevent_version(const char *m, int server, version = tor_get_libevent_version(&v); - /* XXX Would it be worthwhile disabling the methods that we know - * are buggy, rather than just warning about them and then proceeding - * to use them? If so, we should probably not wrap this whole thing - * in HAVE_EVENT_GET_VERSION and HAVE_EVENT_GET_METHOD. -RD */ - /* XXXX The problem is that it's not trivial to get libevent to change it's - * method once it's initialized, and it's not trivial to tell what method it - * will use without initializing it. I guess we could preemptively disable - * buggy libevent modes based on the version _before_ initializing it, - * though, but then there's no good way (afaict) to warn "I would have used - * kqueue, but instead I'm using select." -NM */ - /* XXXX022 revist the above; it is fixable now. */ + /* It would be better to disable known-buggy methods rather than warning + * about them. But the problem is that with older versions of Libevent, + * it's not trivial to get them to change their methods once they're + * initialized... and with newer versions of Libevent, they aren't actually + * broken. But we should revisit this if we ever find a post-1.4 version + * of Libevent where we need to disable a given method. */ if (!strcmp(m, "kqueue")) { if (version < V_OLD(1,1,'b')) buggy = 1;

1 0

[tor/maint-0.2.2] Add XXX023s for our timestamp_dirty abuse.
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit f3b89c114112987b74bb0632cfe47a3a17859adb Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 25 17:49:44 2011 -0400 Add XXX023s for our timestamp_dirty abuse. --- src/or/circuitlist.c | 3 +++ src/or/connection_edge.c | 2 ++ src/or/or.h | 10 ++++++++-- src/or/relay.c | 2 ++ 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index b4f5f45..521c21e 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1023,6 +1023,7 @@ circuit_mark_all_unused_circs(void) * This is useful for letting the user change pseudonyms, so new * streams will not be linkable to old streams. */ +/* XXX023 this is a bad name for what this function does */ void circuit_expire_all_dirty_circs(void) { @@ -1033,6 +1034,8 @@ circuit_expire_all_dirty_circs(void) if (CIRCUIT_IS_ORIGIN(circ) && !circ->marked_for_close && circ->timestamp_dirty) + /* XXXX023 This is a screwed-up way to say "This is too dirty + * for new circuits. */ circ->timestamp_dirty -= options->MaxCircuitDirtiness; } } diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index c75c06b..af0cfbe 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -517,6 +517,7 @@ connection_ap_expire_beginning(void) /* kludge to make us not try this circuit again, yet to allow * current streams on it to survive if they can: make it * unattractive to use for new streams */ + /* XXXX023 this is a kludgy way to do this. */ tor_assert(circ->timestamp_dirty); circ->timestamp_dirty -= options->MaxCircuitDirtiness; /* give our stream another 'cutoff' seconds to try */ @@ -2164,6 +2165,7 @@ connection_ap_handshake_send_begin(edge_connection_t *ap_conn) ap_conn->stream_id = get_unique_stream_id_by_circ(circ); if (ap_conn->stream_id==0) { connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL); + /*XXXX022 _close_ the circuit because it's full? That sounds dumb. */ circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_RESOURCELIMIT); return -1; } diff --git a/src/or/or.h b/src/or/or.h index 124892c..e44c626 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2127,8 +2127,14 @@ typedef struct circuit_t { char *n_conn_onionskin; time_t timestamp_created; /**< When was this circuit created? */ - time_t timestamp_dirty; /**< When the circuit was first used, or 0 if the - * circuit is clean. */ + /** When the circuit was first used, or 0 if the circuit is clean. + * + * XXXX023 Note that some code will artifically adjust this value backward + * in time in order to indicate that a circuit shouldn't be used for new + * streams, but that it can stay alive as long as it has streams on it. + * That's a kludge we should fix. + */ + time_t timestamp_dirty; struct timeval highres_created; /**< When exactly was the circuit created? */ uint16_t marked_for_close; /**< Should we close this circuit at the end of diff --git a/src/or/relay.c b/src/or/relay.c index 076b46b..807cb3d 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -791,6 +791,8 @@ connection_ap_process_end_not_open( < MAX_RESOLVE_FAILURES) { /* We haven't retried too many times; reattach the connection. */ circuit_log_path(LOG_INFO,LD_APP,circ); + /* Mark this circuit "unusable for new streams". */ + /* XXXX023 this is a kludgy way to do this. */ tor_assert(circ->_base.timestamp_dirty); circ->_base.timestamp_dirty -= get_options()->MaxCircuitDirtiness;

1 0

[tor/maint-0.2.2] Clarify some documentation and comments wrt resetting OR token buckets
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit d38030381b04263f76ce7f8ed2d6ceefc4ce363f Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 25 17:21:16 2011 -0400 Clarify some documentation and comments wrt resetting OR token buckets --- src/or/connection_or.c | 6 +++++- src/or/networkstatus.c | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/or/connection_or.c b/src/or/connection_or.c index ff863fd..4b932ec 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -353,6 +353,9 @@ connection_or_digest_is_known_relay(const char *id_digest) * per-conn limits that are big enough they'll never matter. But if it's * not a known relay, first check if we set PerConnBwRate/Burst, then * check if the consensus sets them, else default to 'big enough'. + * + * If reset is true, set the bucket to be full. Otherwise, just + * clip the bucket if it happens to be too full. */ static void connection_or_update_token_buckets_helper(or_connection_t *conn, int reset, @@ -392,7 +395,8 @@ connection_or_update_token_buckets_helper(or_connection_t *conn, int reset, } /** Either our set of relays or our per-conn rate limits have changed. - * Go through all the OR connections and update their token buckets. */ + * Go through all the OR connections and update their token buckets to make + * sure they don't exceed their maximum values. */ void connection_or_update_token_buckets(smartlist_t *conns, or_options_t *options) { diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 18cb477..4f6fe15 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1747,7 +1747,8 @@ networkstatus_set_current_consensus(const char *consensus, routerstatus_list_update_named_server_map(); cell_ewma_set_scale_factor(options, current_consensus); - /* XXX022 where is the right place to put this call? */ + /* XXXX023 this call might be unnecessary here: can changing the + * current consensus really alter our view of any OR's rate limits? */ connection_or_update_token_buckets(get_connection_array(), options); circuit_build_times_new_consensus_params(&circ_times, current_consensus);

1 0

[tor/maint-0.2.2] Fix handling of StreamID exhaustion.
by nickm＠torproject.org 07 Apr '11

07 Apr '11

commit 432734279d3688fafb466a23f43585ff509ff693 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 25 17:57:15 2011 -0400 Fix handling of StreamID exhaustion. Since svn r1475/git 5b6099e8 in tor-0.0.6, we have responded to an exhaustion of all 65535 stream IDs on a circuit by marking that circuit for close. That's not the right response. Instead, we should mark the circuit as "too dirty for new circuits". Of course in reality this isn't really right either. If somebody has managed to cram 65535 streams onto a circuit, the circuit is probably not going to work well for any of those streams, so maybe we should be limiting the number of streams on an origin circuit concurrently. Also, closing the stream in this case is probably the wrong thing to do as well, but fixing that can also wait. --- changes/full_ap_circuits | 6 ++++++ src/or/connection_edge.c | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/changes/full_ap_circuits b/changes/full_ap_circuits new file mode 100644 index 0000000..379a1a1 --- /dev/null +++ b/changes/full_ap_circuits @@ -0,0 +1,6 @@ + o Minor bugfixes + - When a client finds that an origin circuit has run out of 16-bit + stream IDs, we now mark it as unusable for new streams. + Previously, we would try to close the entire circuit. Bugfix on + Tor version 0.0.6. + diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index af0cfbe..72e2c8a 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2164,9 +2164,14 @@ connection_ap_handshake_send_begin(edge_connection_t *ap_conn) ap_conn->stream_id = get_unique_stream_id_by_circ(circ); if (ap_conn->stream_id==0) { + /* XXXX023 Instead of closing this stream, we should make it get + * retried on another circuit. */ connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL); - /*XXXX022 _close_ the circuit because it's full? That sounds dumb. */ - circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_RESOURCELIMIT); + + /* Mark this circuit "unusable for new streams". */ + /* XXXX023 this is a kludgy way to do this. */ + tor_assert(circ->_base.timestamp_dirty); + circ->_base.timestamp_dirty -= get_options()->MaxCircuitDirtiness; return -1; } @@ -2224,9 +2229,14 @@ connection_ap_handshake_send_resolve(edge_connection_t *ap_conn) ap_conn->stream_id = get_unique_stream_id_by_circ(circ); if (ap_conn->stream_id==0) { + /* XXXX023 Instead of closing this stream, we should make it get + * retried on another circuit. */ connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL); - /*XXXX022 _close_ the circuit because it's full? That sounds dumb. */ - circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_RESOURCELIMIT); + + /* Mark this circuit "unusable for new streams". */ + /* XXXX023 this is a kludgy way to do this. */ + tor_assert(circ->_base.timestamp_dirty); + circ->_base.timestamp_dirty -= get_options()->MaxCircuitDirtiness; return -1; }

1 0