diff options
34 files changed, 188 insertions, 134 deletions
diff --git a/changes/bug539_removal b/changes/bug539_removal new file mode 100644 index 000000000..dbff43de1 --- /dev/null +++ b/changes/bug539_removal @@ -0,0 +1,6 @@ + o Removed code + - Removed workaround code to handle directory responses from + servers that had bug 539 (they would send HTTP status 503 + responses _and_ send a body too). Since only server versions + before 0.2.0.16-alpha/0.1.2.19 were affected, there is no longer + reason to keep the workaround in place. diff --git a/changes/connect_err_reporting b/changes/connect_err_reporting new file mode 100644 index 000000000..61a46b658 --- /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/changes/count_overflow b/changes/count_overflow new file mode 100644 index 000000000..f302ff2d7 --- /dev/null +++ b/changes/count_overflow @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Correctly handle an "impossible" overflow cases in connection + byte counting, where we write or read more than 4GB on an edge + connection in single second. Bugfix on 0.1.2.8-beta. + diff --git a/changes/full_ap_circuits b/changes/full_ap_circuits new file mode 100644 index 000000000..379a1a1b7 --- /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/changes/kill_ftime b/changes/kill_ftime new file mode 100644 index 000000000..47f476973 --- /dev/null +++ b/changes/kill_ftime @@ -0,0 +1,7 @@ + o Code simplification and refactoring + - Remove the old 'fuzzy time' logic. It was supposed to be used + for handling calculations where we have a known amount of clock + skew and an allowed amount of unknown skew. But we only used it + in three places, and we never adjusted the known/unknown skew + values. This is still something we might want to do someday, + but if we do, we'll want to do it differently. diff --git a/changes/noroute b/changes/noroute new file mode 100644 index 000000000..644deec45 --- /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/common/address.c b/src/common/address.c index 20dc15275..5b143b065 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -50,10 +50,14 @@ #include <assert.h> /** Convert the tor_addr_t in <b>a</b>, with port in <b>port</b>, into a - * socklen object in *<b>sa_out</b> of object size <b>len</b>. If not enough - * room is free, or on error, return -1. Else return the length of the - * sockaddr. */ -int + * sockaddr object in *<b>sa_out</b> of object size <b>len</b>. If not enough + * room is available in sa_out, or on error, return 0. On success, return + * the length of the sockaddr. + * + * 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, uint16_t port, struct sockaddr *sa_out, @@ -63,7 +67,7 @@ tor_addr_to_sockaddr(const tor_addr_t *a, if (family == AF_INET) { struct sockaddr_in *sin; if (len < (int)sizeof(struct sockaddr_in)) - return -1; + return 0; sin = (struct sockaddr_in *)sa_out; memset(sin, 0, sizeof(struct sockaddr_in)); #ifdef HAVE_STRUCT_SOCKADDR_IN_SIN_LEN @@ -76,7 +80,7 @@ tor_addr_to_sockaddr(const tor_addr_t *a, } else if (family == AF_INET6) { struct sockaddr_in6 *sin6; if (len < (int)sizeof(struct sockaddr_in6)) - return -1; + return 0; sin6 = (struct sockaddr_in6 *)sa_out; memset(sin6, 0, sizeof(struct sockaddr_in6)); #ifdef HAVE_STRUCT_SOCKADDR_IN6_SIN6_LEN @@ -87,7 +91,7 @@ tor_addr_to_sockaddr(const tor_addr_t *a, memcpy(&sin6->sin6_addr, tor_addr_to_in6(a), sizeof(struct in6_addr)); return sizeof(struct sockaddr_in6); } else { - return -1; + return 0; } } @@ -1096,7 +1100,7 @@ get_interface_address6(int severity, sa_family_t family, tor_addr_t *addr) /* ====== * IPv4 helpers - * XXXX022 IPv6 deprecate some of these. + * XXXX023 IPv6 deprecate some of these. */ /** Return true iff <b>ip</b> (in host order) is an IP reserved to localhost, diff --git a/src/common/address.h b/src/common/address.h index f8b9c0212..308790634 100644 --- a/src/common/address.h +++ b/src/common/address.h @@ -39,7 +39,7 @@ static INLINE sa_family_t tor_addr_family(const tor_addr_t *a); static INLINE const struct in_addr *tor_addr_to_in(const tor_addr_t *a); static INLINE int tor_addr_eq_ipv4h(const tor_addr_t *a, uint32_t u); -int tor_addr_to_sockaddr(const tor_addr_t *a, uint16_t port, +socklen_t tor_addr_to_sockaddr(const tor_addr_t *a, uint16_t port, struct sockaddr *sa_out, socklen_t len); int tor_addr_from_sockaddr(tor_addr_t *a, const struct sockaddr *sa, uint16_t *port_out); diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index 48a5b326c..7924036e6 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -354,21 +354,12 @@ tor_check_libevent_version(const char *m, int server, version = tor_get_libevent_version(&v); - /* It would be better to disable known-buggy methods than to simply - warn about them. However, it's not trivial to get libevent to change its - method once it's initialized, and it's not trivial to tell what method it - will use without initializing it. - - If we saw that the version was definitely bad, we could disable all the - methods that were bad for that version. But the issue with that is that - if you've found a libevent before 1.1, you are not at all guaranteed to - have _any_ good method to use. - - As of Libevent 2, we can do better, and have more control over what - methods get used. But the problem here is that there are no versions of - Libevent 2 that have buggy event cores, so there's no point in writing - disable code yet. - */ + /* 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; diff --git a/src/common/memarea.c b/src/common/memarea.c index 94f642488..a6b8c4ee9 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; + /* Reinstate this if bug 930 ever reappears tor_assert(((void*)x) >= ptr); + */ return (void*)x; } @@ -241,10 +243,10 @@ memarea_alloc(memarea_t *area, size_t sz) } result = chunk->next_mem; chunk->next_mem = chunk->next_mem + sz; - + /* 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; } diff --git a/src/common/util.h b/src/common/util.h index 97fd4f7aa..759f068b0 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -247,24 +247,6 @@ time_t approx_time(void); void update_approx_time(time_t now); #endif -/* Fuzzy time. */ - -/** Return true iff <a>a</b> is definitely after <b>b</b>, even if there - * could be up to <b>allow_seconds</b> of skew in one of them. */ -static INLINE int -time_definitely_after(time_t a, time_t b, int allow_skew) -{ - return a-allow_skew > b; -} - -/** Return true iff <a>a</b> is definitely before <b>b</b>, even if there - * could be up to <b>allow_seconds</b> of skew in one of them. */ -static INLINE int -time_definitely_before(time_t a, time_t b, int allow_skew) -{ - return a+allow_skew < b; -} - /* Rate-limiter */ /** A ratelim_t remembers how often an event is occurring, and how often diff --git a/src/or/buffers.c b/src/or/buffers.c index 2a5b4e15c..70f8b4a52 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -643,10 +643,14 @@ read_to_chunk_tls(buf_t *buf, chunk_t *chunk, tor_tls_t *tls, * (because of EOF), set *<b>reached_eof</b> to 1 and return 0. Return -1 on * error; else return the number of bytes read. */ +/* XXXX023 indicate "read blocked" somehow? */ int read_to_buf(int s, size_t at_most, buf_t *buf, int *reached_eof, int *socket_error) { + /* XXXX023 It's stupid to overload the return values for these functions: + * "error status" and "number of bytes read" are not mutually exclusive. + */ int r = 0; size_t total_read = 0; @@ -814,6 +818,9 @@ flush_chunk_tls(tor_tls_t *tls, buf_t *buf, chunk_t *chunk, int flush_buf(int s, buf_t *buf, size_t sz, size_t *buf_flushlen) { + /* XXXX023 It's stupid to overload the return values for these functions: + * "error status" and "number of bytes flushed" are not mutually exclusive. + */ int r; size_t flushed = 0; tor_assert(buf_flushlen); diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 28b1e795f..ff656fdad 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -43,11 +43,12 @@ /********* START VARIABLES **********/ /** Global list of circuit build times */ -// FIXME: Add this as a member for entry_guard_t instead of global? +// XXXX023: Add this as a member for entry_guard_t instead of global? // Then we could do per-guard statistics, as guards are likely to // vary in their own latency. The downside of this is that guards // can change frequently, so we'd be building a lot more circuits // most likely. +/* XXXX023 Make this static; add accessor functions. */ circuit_build_times_t circ_times; /** A global list of all circuits at this hop. */ @@ -3831,7 +3832,7 @@ entry_guards_compute_status(or_options_t *options, time_t now) * If <b>mark_relay_status</b>, also call router_set_status() on this * relay. * - * XXX022 change succeeded and mark_relay_status into 'int flags'. + * XXX023 change succeeded and mark_relay_status into 'int flags'. */ int entry_guard_register_connect_status(const char *digest, int succeeded, @@ -4321,7 +4322,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) } entry_guards = new_entry_guards; entry_guards_dirty = 0; - /* XXX022 hand new_entry_guards to this func, and move it up a + /* XXX023 hand new_entry_guards to this func, and move it up a * few lines, so we don't have to re-dirty it */ if (remove_obsolete_entry_guards(now)) entry_guards_dirty = 1; diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index a0359dbdc..8949e9773 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1021,6 +1021,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) { @@ -1031,6 +1032,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/circuituse.c b/src/or/circuituse.c index 5488eb7be..e58d5e05b 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1277,7 +1277,8 @@ circuit_get_open_circ_or_launch(edge_connection_t *conn, return -1; } } else { - /* XXXX022 Duplicates checks in connection_ap_handshake_attach_circuit */ + /* XXXX023 Duplicates checks in connection_ap_handshake_attach_circuit: + * refactor into a single function? */ const node_t *node = node_get_by_nickname(conn->chosen_exit_name, 1); int opt = conn->chosen_exit_optional; if (node && !connection_ap_can_use_exit(conn, node, 0)) { @@ -1616,7 +1617,7 @@ connection_ap_handshake_attach_circuit(edge_connection_t *conn) /* find the circuit that we should use, if there is one. */ retval = circuit_get_open_circ_or_launch( conn, CIRCUIT_PURPOSE_C_GENERAL, &circ); - if (retval < 1) // XXX021 if we totally fail, this still returns 0 -RD + if (retval < 1) // XXX022 if we totally fail, this still returns 0 -RD return retval; log_debug(LD_APP|LD_CIRC, diff --git a/src/or/config.c b/src/or/config.c index e6322cb0b..1bf0e6f88 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1367,7 +1367,7 @@ options_act(or_options_t *old_options) || !geoip_is_loaded())) { /* XXXX Don't use this "<default>" junk; make our filename options * understand prefixes somehow. -NM */ - /* XXXX021 Reload GeoIPFile on SIGHUP. -NM */ + /* XXXX023 Reload GeoIPFile on SIGHUP. -NM */ char *actual_fname = tor_strdup(options->GeoIPFile); #ifdef WIN32 if (!strcmp(actual_fname, "<default>")) { @@ -2537,7 +2537,7 @@ is_local_addr(const tor_addr_t *addr) if (get_options()->EnforceDistinctSubnets == 0) return 0; if (tor_addr_family(addr) == AF_INET) { - /*XXXX022 IP6 what corresponds to an /24? */ + /*XXXX023 IP6 what corresponds to an /24? */ uint32_t ip = tor_addr_to_ipv4h(addr); /* It's possible that this next check will hit before the first time @@ -3728,7 +3728,7 @@ options_validate(or_options_t *old_options, or_options_t *options, "ignore you."); } - /*XXXX022 checking for defaults manually like this is a bit fragile.*/ + /*XXXX023 checking for defaults manually like this is a bit fragile.*/ /* Keep changes to hard-coded values synchronous to man page and default * values table. */ diff --git a/src/or/connection.c b/src/or/connection.c index 8b48b96dc..02ae7ee7a 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -57,7 +57,7 @@ static int connection_finished_flushing(connection_t *conn); static int connection_flushed_some(connection_t *conn); static int connection_finished_connecting(connection_t *conn); static int connection_reached_eof(connection_t *conn); -static int connection_read_to_buf(connection_t *conn, int *max_to_read, +static int connection_read_to_buf(connection_t *conn, ssize_t *max_to_read, int *socket_error); static int connection_process_inbuf(connection_t *conn, int package_partial); static void client_check_address_changed(int sock); @@ -2510,7 +2510,7 @@ connection_consider_empty_read_buckets(connection_t *conn) static int connection_handle_read_impl(connection_t *conn) { - int max_to_read=-1, try_to_read; + ssize_t max_to_read=-1, try_to_read; size_t before, n_read = 0; int socket_error = 0; @@ -2628,7 +2628,8 @@ connection_handle_read(connection_t *conn) * Return -1 if we want to break conn, else return 0. */ static int -connection_read_to_buf(connection_t *conn, int *max_to_read, int *socket_error) +connection_read_to_buf(connection_t *conn, ssize_t *max_to_read, + int *socket_error) { int result; ssize_t at_most = *max_to_read; @@ -2746,15 +2747,19 @@ connection_read_to_buf(connection_t *conn, int *max_to_read, int *socket_error) n_read = (size_t) result; } - if (n_read > 0) { /* change *max_to_read */ - /*XXXX022 check for overflow*/ - *max_to_read = (int)(at_most - n_read); - } + if (n_read > 0) { + /* change *max_to_read */ + *max_to_read = at_most - n_read; - if (conn->type == CONN_TYPE_AP) { - edge_connection_t *edge_conn = TO_EDGE_CONN(conn); - /*XXXX022 check for overflow*/ - edge_conn->n_read += (int)n_read; + /* Update edge_conn->n_read */ + if (conn->type == CONN_TYPE_AP) { + edge_connection_t *edge_conn = TO_EDGE_CONN(conn); + /* Check for overflow: */ + if (PREDICT_LIKELY(UINT32_MAX - edge_conn->n_read > n_read)) + edge_conn->n_read += (int)n_read; + else + edge_conn->n_read = UINT32_MAX; + } } connection_buckets_decrement(conn, approx_time(), n_read, n_written); @@ -3145,10 +3150,14 @@ connection_handle_write_impl(connection_t *conn, int force) n_written = (size_t) result; } - if (conn->type == CONN_TYPE_AP) { + if (n_written && conn->type == CONN_TYPE_AP) { edge_connection_t *edge_conn = TO_EDGE_CONN(conn); - /*XXXX022 check for overflow.*/ - edge_conn->n_written += (int)n_written; + + /* Check for overflow: */ + if (PREDICT_LIKELY(UINT32_MAX - edge_conn->n_written > n_written)) + edge_conn->n_written += (int)n_written; + else + edge_conn->n_written = UINT32_MAX; } connection_buckets_decrement(conn, approx_time(), n_read, n_written); diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index dc2598945..508b06b26 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 */ @@ -557,7 +558,7 @@ connection_ap_attach_pending(void) /** Tell any AP streams that are waiting for a one-hop tunnel to * <b>failed_digest</b> that they are going to fail. */ -/* XXX022 We should get rid of this function, and instead attach +/* XXX023 We should get rid of this function, and instead attach * one-hop streams to circ->p_streams so they get marked in * circuit_mark_for_close like normal p_streams. */ void @@ -2169,8 +2170,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); - 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; } @@ -2230,9 +2237,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; } @@ -2392,7 +2404,7 @@ tell_controller_about_resolved_result(edge_connection_t *conn, * certain errors or for values that didn't come via DNS. <b>expires</b> is * a time when the answer expires, or -1 or TIME_MAX if there's a good TTL. **/ -/* XXXX022 the use of the ttl and expires fields is nutty. Let's make this +/* XXXX023 the use of the ttl and expires fields is nutty. Let's make this * interface and those that use it less ugly. */ void connection_ap_handshake_socks_resolved(edge_connection_t *conn, @@ -2833,13 +2845,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: - /* XXX021 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; diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 266a73c7f..6d68b0e70 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -374,6 +374,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 <b>reset</b> is true, set the bucket to be full. Otherwise, just + * clip the bucket if it happens to be <em>too</em> full. */ static void connection_or_update_token_buckets_helper(or_connection_t *conn, int reset, @@ -430,7 +433,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/directory.c b/src/or/directory.c index 3b1fb02a5..6bef58149 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -399,7 +399,7 @@ directory_get_from_dirserver(uint8_t dir_purpose, uint8_t router_purpose, if (!get_via_tor) { if (options->UseBridges && type != BRIDGE_AUTHORITY) { /* want to ask a running bridge for which we have a descriptor. */ - /* XXX022 we assume that all of our bridges can answer any + /* XXX023 we assume that all of our bridges can answer any * possible directory question. This won't be true forever. -RD */ /* It certainly is not true with conditional consensus downloading, * so, for now, never assume the server supports that. */ @@ -1602,27 +1602,19 @@ connection_dir_client_reached_eof(dir_connection_t *conn) (void) skewed; /* skewed isn't used yet. */ if (status_code == 503) { - if (body_len < 16) { - routerstatus_t *rs; - trusted_dir_server_t *ds; - log_info(LD_DIR,"Received http status code %d (%s) from server " - "'%s:%d'. I'll try again soon.", - status_code, escaped(reason), conn->_base.address, - conn->_base.port); - rs = router_get_mutable_consensus_status_by_id(conn->identity_digest); - if (rs) - rs->last_dir_503_at = now; - if ((ds = router_get_trusteddirserver_by_digest(conn->identity_digest))) - ds->fake_status.last_dir_503_at = now; + routerstatus_t *rs; + trusted_dir_server_t *ds; + log_info(LD_DIR,"Received http status code %d (%s) from server " + "'%s:%d'. I'll try again soon.", + status_code, escaped(reason), conn->_base.address, + conn->_base.port); + if ((rs = router_get_mutable_consensus_status_by_id(conn->identity_digest))) + rs->last_dir_503_at = now; + if ((ds = router_get_trusteddirserver_by_digest(conn->identity_digest))) + ds->fake_status.last_dir_503_at = now; - tor_free(body); tor_free(headers); tor_free(reason); - return -1; - } - /* XXXX022 Remove this once every server with bug 539 is obsolete. */ - log_info(LD_DIR, "Server at '%s:%d' sent us a 503 response, but included " - "a body anyway. We'll pretend it gave us a 200.", - conn->_base.address, conn->_base.port); - status_code = 200; + tor_free(body); tor_free(headers); tor_free(reason); + return -1; } plausible = body_is_plausible(body, body_len, conn->_base.purpose); @@ -1980,7 +1972,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn) ds->nickname); /* XXXX use this information; be sure to upload next one * sooner. -NM */ - /* XXXX021 On further thought, the task above implies that we're + /* XXXX023 On further thought, the task above implies that we're * basing our regenerate-descriptor time on when we uploaded the * last descriptor, not on the published time of the last * descriptor. If those are different, that's a bad thing to @@ -2810,7 +2802,7 @@ directory_handle_command_get(dir_connection_t *conn, const char *headers, ssize_t estimated_len = 0; smartlist_t *items = smartlist_create(); smartlist_t *dir_items = smartlist_create(); - int lifetime = 60; /* XXXX022 should actually use vote intervals. */ + int lifetime = 60; /* XXXX023 should actually use vote intervals. */ url += strlen("/tor/status-vote/"); current = !strcmpstart(url, "current/"); url = strchr(url, '/'); diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 1796c2841..759b1cfa6 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -967,7 +967,7 @@ running_long_enough_to_decide_unreachable(void) void dirserv_set_router_is_running(routerinfo_t *router, time_t now) { - /*XXXX022 This function is a mess. Separate out the part that calculates + /*XXXX023 This function is a mess. Separate out the part that calculates whether it's reachable and the part that tells rephist that the router was unreachable. */ @@ -1781,9 +1781,12 @@ dirserv_thinks_router_is_unreliable(time_t now, { if (need_uptime) { if (!enough_mtbf_info) { - /* XXX022 Once most authorities are on v3, we should change the rule from + /* XXX023 Once most authorities are on v3, we should change the rule from * "use uptime if we don't have mtbf data" to "don't advertise Stable on - * v3 if we don't have enough mtbf data." */ + * v3 if we don't have enough mtbf data." Or maybe not, since if we ever + * hit a point where we need to reset a lot of authorities at once, + * none of them would be in a position to declare Stable. + */ long uptime = real_uptime(router, now); if ((unsigned)uptime < stable_uptime && (unsigned)uptime < UPTIME_TO_GUARANTEE_STABLE) @@ -3288,7 +3291,7 @@ lookup_cached_dir_by_fp(const char *fp) d = strmap_get(cached_consensuses, "ns"); else if (memchr(fp, '\0', DIGEST_LEN) && cached_consensuses && (d = strmap_get(cached_consensuses, fp))) { - /* this here interface is a nasty hack XXXX022 */; + /* this here interface is a nasty hack XXXX023 */; } else if (router_digest_is_me(fp) && the_v2_networkstatus) d = the_v2_networkstatus; else if (cached_v2_networkstatus) diff --git a/src/or/dns.c b/src/or/dns.c index a3ddc7a48..c8e101252 100644 --- a/src/or/dns.c +++ b/src/or/dns.c @@ -1206,7 +1206,7 @@ configure_nameservers(int force) struct sockaddr_storage ss; socklen = tor_addr_to_sockaddr(&addr, 0, (struct sockaddr *)&ss, sizeof(ss)); - if (socklen < 0) { + if (socklen <= 0) { log_warn(LD_BUG, "Couldn't convert outbound bind address to sockaddr." " Ignoring."); } else { diff --git a/src/or/dnsserv.c b/src/or/dnsserv.c index 8222c8b45..f7a8d35f7 100644 --- a/src/or/dnsserv.c +++ b/src/or/dnsserv.c @@ -19,7 +19,7 @@ #ifdef HAVE_EVENT2_DNS_H #include <event2/dns.h> #include <event2/dns_compat.h> -/* XXXX022 this implies we want an improved evdns */ +/* XXXX023 this implies we want an improved evdns */ #include <event2/dns_struct.h> #else #include "eventdns.h" diff --git a/src/or/eventdns.c b/src/or/eventdns.c index 05831197d..9030dfbbb 100644 --- a/src/or/eventdns.c +++ b/src/or/eventdns.c @@ -1999,7 +1999,7 @@ evdns_request_timeout_callback(int fd, short events, void *arg) { /* retransmit it */ /* Stop waiting for the timeout. No need to do this in * request_finished; that one already deletes the timeout event. - * XXXX021 port this change to libevent. */ + * XXXX023 port this change to libevent. */ del_timeout_event(req); evdns_request_transmit(req); } diff --git a/src/or/geoip.c b/src/or/geoip.c index 03e5b523b..71ed3bedf 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -621,8 +621,9 @@ _dirreq_map_put(dirreq_map_entry_t *entry, dirreq_type_t type, tor_assert(entry->type == type); tor_assert(entry->dirreq_id == dirreq_id); - /* XXXX022 once we're sure the bug case never happens, we can switch - * to HT_INSERT */ + /* XXXX we could switch this to HT_INSERT some time, since it seems that + * this bug doesn't happen. But since this function doesn't seem to be + * critical-path, it's sane to leave it alone. */ old_ent = HT_REPLACE(dirreqmap, &dirreq_map, entry); if (old_ent && old_ent != entry) { log_warn(LD_BUG, "Error when putting directory request into local " diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 638712639..50982d715 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1630,7 +1630,7 @@ networkstatus_set_current_consensus(const char *consensus, if (from_cache && !accept_obsolete && c->valid_until < now-OLD_ROUTER_DESC_MAX_AGE) { - /* XXX022 when we try to make fallbackconsensus work again, we should + /* XXXX If we try to make fallbackconsensus work again, we should * consider taking this out. Until then, believing obsolete consensuses * is causing more harm than good. See also bug 887. */ log_info(LD_DIR, "Loaded an expired consensus. Discarding."); @@ -1790,7 +1790,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); @@ -1807,7 +1808,11 @@ networkstatus_set_current_consensus(const char *consensus, write_str_to_file(consensus_fname, consensus, 0); } - if (time_definitely_before(now, c->valid_after, 60)) { +/** If a consensus appears more than this many seconds before its declared + * valid-after time, declare that our clock is skewed. */ +#define EARLY_CONSENSUS_NOTICE_SKEW 60 + + if (now < current_consensus->valid_after - EARLY_CONSENSUS_NOTICE_SKEW) { char tbuf[ISO_TIME_LEN+1]; char dbuf[64]; long delta = now - c->valid_after; diff --git a/src/or/or.h b/src/or/or.h index c134d7c7a..56d701bee 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1024,7 +1024,7 @@ typedef struct connection_t { /** Unique identifier for this connection on this Tor instance. */ uint64_t global_identifier; - /* XXXX022 move this field, and all the listener-only fields (just + /* XXXX023 move this field, and all the listener-only fields (just socket_family, I think), into a new listener_connection_t subtype. */ /** If the connection is a CONN_TYPE_AP_DNS_LISTENER, this field points * to the evdns_server_port is uses to listen to and answer connections. */ @@ -2283,8 +2283,14 @@ typedef struct circuit_t { * resolution than most so that the circuit-build-time tracking code can * get millisecond resolution. */ struct timeval timestamp_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; uint16_t marked_for_close; /**< Should we close this circuit at the end of * the main loop? (If true, holds the line number diff --git a/src/or/reasons.c b/src/or/reasons.c index 304ea9fcf..319e6c055 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): - /* XXXX022 - * 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): diff --git a/src/or/relay.c b/src/or/relay.c index 657ee32fc..e5bc310fe 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -793,6 +793,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; diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 3aa8aa3fc..47161ebfa 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -602,7 +602,7 @@ rend_client_rendezvous_acked(origin_circuit_t *circ, const uint8_t *request, log_info(LD_REND,"Got rendezvous ack. This circuit is now ready for " "rendezvous."); circ->_base.purpose = CIRCUIT_PURPOSE_C_REND_READY; - /* XXXX022 This is a pretty brute-force approach. It'd be better to + /* XXXX023 This is a pretty brute-force approach. It'd be better to * attach only the connections that are waiting on this circuit, rather * than trying to attach them all. See comments bug 743. */ /* If we already have the introduction circuit built, make sure we send @@ -672,7 +672,7 @@ rend_client_receive_rendezvous(origin_circuit_t *circ, const uint8_t *request, onion_append_to_cpath(&circ->cpath, hop); circ->build_state->pending_final_cpath = NULL; /* prevent double-free */ - /* XXXX022 This is a pretty brute-force approach. It'd be better to + /* XXXX023 This is a pretty brute-force approach. It'd be better to * attach only the connections that are waiting on this circuit, rather * than trying to attach them all. See comments bug 743. */ connection_ap_attach_pending(); diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c index aac660ab6..9d2b1b15b 100644 --- a/src/or/rendcommon.c +++ b/src/or/rendcommon.c @@ -931,7 +931,7 @@ rend_cache_lookup_entry(const char *query, int version, rend_cache_entry_t **e) if (!*e) return 0; tor_assert((*e)->parsed && (*e)->parsed->intro_nodes); - /* XXX022 hack for now, to return "not found" if there are no intro + /* XXX023 hack for now, to return "not found" if there are no intro * points remaining. See bug 997. */ if (smartlist_len((*e)->parsed->intro_nodes) == 0) return 0; diff --git a/src/or/rephist.c b/src/or/rephist.c index 688ff82ee..56a3d1de2 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -588,7 +588,7 @@ rep_hist_get_weighted_time_known(const char *id, time_t when) int rep_hist_have_measured_enough_stability(void) { - /* XXXX021 This doesn't do so well when we change our opinion + /* XXXX022 This doesn't do so well when we change our opinion * as to whether we're tracking router stability. */ return started_tracking_stability < time(NULL) - 4*60*60; } diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 1205fd123..580763de9 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -332,7 +332,7 @@ trusted_dirs_remove_old_certs(void) time_t cert_published; if (newest == cert) continue; - expired = time_definitely_after(now, cert->expires, CERT_EXPIRY_SKEW); + expired = now > cert->expires; cert_published = cert->cache_info.published_on; /* Store expired certs for 48 hours after a newer arrives; */ @@ -524,7 +524,7 @@ authority_certs_fetch_missing(networkstatus_t *status, time_t now) continue; cl = get_cert_list(ds->v3_identity_digest); SMARTLIST_FOREACH(cl->certs, authority_cert_t *, cert, { - if (! time_definitely_after(now, cert->expires, CERT_EXPIRY_SKEW)) { + if (now < cert->expires) { /* It's not expired, and we weren't looking for something to * verify a consensus with. Call it done. */ download_status_reset(&cl->dl_status); @@ -1766,7 +1766,7 @@ smartlist_choose_node_by_bandwidth_weights(smartlist_t *sl, sl_last_weighted_bw_of_me = weight*this_bw; } SMARTLIST_FOREACH_END(node); - /* XXXX022 this is a kludge to expose these values. */ + /* XXXX023 this is a kludge to expose these values. */ sl_last_total_weighted_bw = weighted_bw; log_debug(LD_CIRC, "Choosing node for rule %s based on weights " @@ -1885,7 +1885,7 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl, if (node->rs->has_bandwidth) { this_bw = kb_to_bytes(node->rs->bandwidth); } else { /* guess */ - /* XXX022 once consensuses always list bandwidths, we can take + /* XXX023 once consensuses always list bandwidths, we can take * this guessing business out. -RD */ is_known = 0; flags = node->rs->is_fast ? 1 : 0; @@ -2004,7 +2004,7 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl, } } - /* XXXX022 this is a kludge to expose these values. */ + /* XXXX023 this is a kludge to expose these values. */ sl_last_total_weighted_bw = total_bw; log_debug(LD_CIRC, "Total weighted bw = "U64_FORMAT @@ -3420,7 +3420,7 @@ router_add_extrainfo_to_routerlist(extrainfo_t *ei, const char **msg, int inserted; (void)from_fetch; if (msg) *msg = NULL; - /*XXXX022 Do something with msg */ + /*XXXX023 Do something with msg */ inserted = extrainfo_insert(router_get_routerlist(), ei); @@ -4671,7 +4671,7 @@ update_consensus_router_descriptor_downloads(time_t now, int is_vote, /** How often should we launch a server/authority request to be sure of getting * a guess for our IP? */ -/*XXXX021 this info should come from netinfo cells or something, or we should +/*XXXX023 this info should come from netinfo cells or something, or we should * do this only when we aren't seeing incoming data. see bug 652. */ #define DUMMY_DOWNLOAD_INTERVAL (20*60) @@ -4690,7 +4690,7 @@ update_router_descriptor_downloads(time_t now) update_consensus_router_descriptor_downloads(now, 0, networkstatus_get_reasonably_live_consensus(now, FLAV_NS)); - /* XXXX021 we could be smarter here; see notes on bug 652. */ + /* XXXX023 we could be smarter here; see notes on bug 652. */ /* XXXX NM Microdescs: if we're not fetching microdescriptors, we need * to make something else invoke this. */ /* If we're a server that doesn't have a configured address, we rely on diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 4557f5c81..cc98cc0e7 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1806,7 +1806,7 @@ authority_cert_parse_from_string(const char *s, const char **end_of_string) struct in_addr in; char *address = NULL; tor_assert(tok->n_args); - /* XXX021 use tor_addr_port_parse() below instead. -RD */ + /* XXX023 use tor_addr_port_parse() below instead. -RD */ if (parse_addr_port(LOG_WARN, tok->args[0], &address, NULL, &cert->dir_port)<0 || tor_inet_aton(address, &in) == 0) { |