Merge remote-tracking branch 'origin/maint-0.2.2'

Conflicts:
	src/common/address.c
	src/common/compat_libevent.c
	src/common/memarea.c
	src/common/util.h
	src/or/buffers.c
	src/or/circuitbuild.c
	src/or/circuituse.c
	src/or/connection.c
	src/or/directory.c
	src/or/networkstatus.c
	src/or/or.h
	src/or/routerlist.c

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.

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

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

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

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.

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

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
+ * sockaddr 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
+ * room is available in sa_out, or on error, return 0.  On success, return
- * sockaddr. */
+ * the length of the sockaddr.
-int
+ *
+ * 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,

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);

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
+  /* It would be better to disable known-buggy methods rather than warning
-     warn about them.  However, it's not trivial to get libevent to change its
+   * about them.  But the problem is that with older versions of Libevent,
-     method once it's initialized, and it's not trivial to tell what method it
+   * it's not trivial to get them to change their methods once they're
-     will use without initializing it.
+   * 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
-     If we saw that the version was definitely bad, we could disable all the
+   * of Libevent where we need to disable a given method. */
-     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.
-  */
   if (!strcmp(m, "kqueue")) {
     if (version < V_OLD(1,1,'b'))
       buggy = 1;

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;
 }

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

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);

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;

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;
   }
 }

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,

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. */

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 */
+  if (n_read > 0) {
-    /*XXXX022 check for overflow*/
+     /* change *max_to_read */
-    *max_to_read = (int)(at_most - n_read);
+    *max_to_read = at_most - n_read;
-  }
 
+    /* Update edge_conn->n_read */
     if (conn->type == CONN_TYPE_AP) {
       edge_connection_t *edge_conn = TO_EDGE_CONN(conn);
-    /*XXXX022 check for overflow*/
+      /* 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.*/
+
+    /* 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);

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:
+    case -1: {
-      /* XXX021 use socket_error below rather than trying to piece things
+      int reason = errno_to_stream_end_reason(socket_error);
-       * together from the current errno, which may have been clobbered. */
+      connection_edge_end(edge_conn, reason);
-      connection_edge_end_errno(edge_conn);
       circuit_detach_stream(circuit_get_by_edge_conn(edge_conn), edge_conn);
       connection_free(conn);
       return;
+    }
     case 0:
       conn->state = EXIT_CONN_STATE_CONNECTING;
 

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)
 {

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,15 +1602,13 @@ 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 = 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;
@@ -1618,12 +1616,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
     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;
-  }
 
   plausible = body_is_plausible(body, body_len, conn->_base.purpose);
   if (compression != NO_METHOD || !plausible) {
@@ -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, '/');

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)

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 {

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"

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);
 	}

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
+  /* XXXX we could switch this to HT_INSERT some time, since it seems that
-   * to HT_INSERT */
+   * 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 "

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;

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
+  /** When the circuit was first used, or 0 if the circuit is clean.
-                           * 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

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
+      return END_STREAM_REASON_NOROUTE;
-       * 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;
     S_CASE(ECONNREFUSED):
       return END_STREAM_REASON_CONNECTREFUSED;
     S_CASE(ECONNRESET):

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;
 

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();

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;

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;
 }

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

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) {