Checkpoint my big bug-891 patch.

svn:r17757

src/or/circuitbuild.c

@@ -334,13 +334,14 @@ circuit_establish_circuit(uint8_t purpose, extend_info_t *exit, int flags)
   return circ;
 }
 
+#if 0
 /** Return true iff <b>n_conn</b> (a connection with a desired identity), is
  * an acceptable choice for extending or launching a circuit to the address
  * <b>target_addr</b>.  If it is not, set <b>state_out</b> to a message
  * describing the connection's state and our next action, and set
  * <b>launch_out</b> to a boolean for whether we should launch a new
  * connection or not. */
-static int
+int
 connection_good_enough_for_extend(const or_connection_t *n_conn,
                                   const tor_addr_t *target_addr,
                                   const char **state_out,
@@ -373,6 +374,7 @@ connection_good_enough_for_extend(const or_connection_t *n_conn,
     return 1;
   }
 }
+#endif
 
 /** Start establishing the first hop of our circuit. Figure out what
  * OR we should connect to, and if necessary start the connection to
@@ -396,18 +398,16 @@ circuit_handle_first_hop(origin_circuit_t *circ)
             fmt_addr(&firsthop->extend_info->addr),
             firsthop->extend_info->port);
 
-  n_conn = connection_or_get_by_identity_digest(
+  n_conn = connection_or_get_for_extend(firsthop->extend_info->identity_digest,
-         firsthop->extend_info->identity_digest);
+                                        &firsthop->extend_info->addr,
+                                        &msg,
+                                        &should_launch);
 
-  /* If we don't have an open conn, or the conn we have is obsolete
+  if (!n_conn) {
-   * (i.e. old or broken) and the other side will let us make a second
-   * connection without dropping it immediately... */
-  if (!connection_good_enough_for_extend(n_conn, &firsthop->extend_info->addr,
-                                         &msg, &should_launch)) {
     /* not currently connected in a useful way. */
     const char *name = firsthop->extend_info->nickname ?
       firsthop->extend_info->nickname : fmt_addr(&firsthop->extend_info->addr);
-    log_info(LD_CIRC, "Next router %s on circuit is %s", safe_str(name), msg);
+    log_info(LD_CIRC, "Next router is %s: %s ", safe_str(name), msg?msg:"???");
     circ->_base.n_hop = extend_info_dup(firsthop->extend_info);
 
     if (should_launch) {
@@ -824,14 +824,13 @@ circuit_extend(cell_t *cell, circuit_t *circ)
     return -1;
   }
 
-  n_conn = connection_or_get_by_identity_digest(id_digest);
+  n_conn = connection_or_get_for_extend(id_digest,
+                                        &n_addr,
+                                        &msg,
+                                        &should_launch);
 
-  /* If we don't have an open conn, or the conn we have is obsolete
+  if (!n_conn) {
-   * (i.e. old or broken) and the other side will let us make a second
+    log_debug(LD_CIRC|LD_OR,"Next router (%s:%d): %s",
-   * connection without dropping it immediately... */
-  if (!connection_good_enough_for_extend(n_conn, &n_addr, &msg,
-                                         &should_launch)) {
-    log_debug(LD_CIRC|LD_OR,"Next router (%s:%d) %s",
               fmt_addr(&n_addr), (int)n_port, msg?msg:"????");
 
     circ->n_hop = extend_info_alloc(NULL /*nickname*/,

src/or/circuituse.c

@@ -811,9 +811,14 @@ circuit_build_failed(origin_circuit_t *circ)
       if (n_conn) n_conn_id = n_conn->identity_digest;
     } else if (circ->_base.state == CIRCUIT_STATE_OR_WAIT &&
                circ->_base.n_hop) {
-      /* we have to hunt for it */
       n_conn_id = circ->_base.n_hop->identity_digest;
+#if 0
+      /* XXXX021 I believe this logic was wrong.  If we're in state_or_wait,
+       * it's wrong to blame a particular connection for our failure to extend
+       * and set its is_bad_for_new_circs field: no connection ever got
+       * a chance to hear our CREATE cell. -NM*/
       n_conn = connection_or_get_by_identity_digest(n_conn_id);
+#endif
     }
     if (n_conn) {
       log_info(LD_OR,
@@ -821,14 +826,14 @@ circuit_build_failed(origin_circuit_t *circ)
                "(%s:%d). I'm going to try to rotate to a better connection.",
                n_conn->_base.address, n_conn->_base.port);
       n_conn->is_bad_for_new_circs = 1;
-      entry_guard_register_connect_status(n_conn->identity_digest, 0,
-                                          time(NULL));
     }
+    if (n_conn_id) {
+      entry_guard_register_connect_status(n_conn_id, 0, time(NULL));
       /* if there are any one-hop streams waiting on this circuit, fail
        * them now so they can retry elsewhere. */
-    if (n_conn_id)
       connection_ap_fail_onehop(n_conn_id, circ->build_state);
     }
+  }
 
   switch (circ->_base.purpose) {
     case CIRCUIT_PURPOSE_C_GENERAL:

src/or/connection_or.c

@@ -436,23 +436,62 @@ connection_or_init_conn_from_address(or_connection_t *conn,
   }
 }
 
-/** Return the best connection of type OR with the
+/** Return true iff <b>a</b> is "better" than <b>b</b> for new circuits.
- * digest <b>digest</b> that we have, or NULL if we have none.
  *
- * 1) Don't return it if it's marked for close.
+ * A more canonical connection is always better than a less canonical
- * 2) If there are any open conns, ignore non-open conns.
+ * connection.  That aside, a connection is better if it has circuits and the
- * 3) If there are any non-obsolete conns, ignore obsolete conns.
+ * other does not, or if it was created more recently.
- * 4) Then if there are any non-empty conns, ignore empty conns.
+ *
- * 5) Of the remaining conns, prefer newer conns.
+ * Requires that both input connections are open; not is_bad_for_new_circs,
+ * and not impossibly non-canonical.
  */
-or_connection_t *
+static int
-connection_or_get_by_identity_digest(const char *digest)
+connection_or_is_better(const or_connection_t *a,
+                        const or_connection_t *b)
 {
   int newer;
-  or_connection_t *conn, *best=NULL;
 
-  if (!orconn_identity_map)
+  if (b->is_canonical && !a->is_canonical)
+    return 0; /* A canonical connection is better than a non-canonical
+               * one, no matter how new it is or which has circuits. */
+
+  newer = b->_base.timestamp_created < a->_base.timestamp_created;
+
+  return
+    /* We prefer canonical connections regardless of newness. */
+    (!b->is_canonical && a->is_canonical) ||
+    /* If both have circuits we prefer the newer: */
+    (b->n_circuits && a->n_circuits && newer) ||
+    /* If neither has circuits we prefer the newer: */
+    (!b->n_circuits && !a->n_circuits && newer) ||
+    /* If only one has circuits, use that. */
+    (!b->n_circuits && a->n_circuits);
+}
+
+/** Return the OR connection we should use to extend a circuit to the router
+ * whose identity is <b>digest</b>, and whose address we believe (or have been
+ * told in an extend cell) is <b>target_addr</b>.  If there is no good
+ * connection, set *<b>msg_out</b> to a message describing the connection's
+ * state and our next action, and set <b>launch_out</b> to a boolean for
+ * whether we should launch a new connection or not.
+ */
+or_connection_t *
+connection_or_get_for_extend(const char *digest,
+                             const tor_addr_t *target_addr,
+                             const char **msg_out,
+                             int *launch_out)
+{
+  or_connection_t *conn, *best=NULL;
+  int n_inprogress_goodaddr = 0, n_old = 0, n_noncanonical = 0, n_possible = 0;
+
+  tor_assert(msg_out);
+  tor_assert(launch_out);
+
+  if (!orconn_identity_map) {
+    *msg_out = "Router not connected (nothing is).  Connecting.";
+    *launch_out = 1;
     return NULL;
+  }
 
   conn = digestmap_get(orconn_identity_map, digest);
 
@@ -462,36 +501,192 @@ connection_or_get_by_identity_digest(const char *digest)
     tor_assert(!memcmp(conn->identity_digest, digest, DIGEST_LEN));
     if (conn->_base.marked_for_close)
       continue;
-    if (!best) {
+    /* Never return a non-open connection. */
-      best = conn; /* whatever it is, it's better than nothing. */
+    if (conn->_base.state != OR_CONN_STATE_OPEN) {
+      /* If the address matches, don't launch a new connection for this
+       * circuit. */
+      if (!tor_addr_compare(&conn->real_addr, target_addr, CMP_EXACT))
+        ++n_inprogress_goodaddr;
       continue;
     }
-    if (best->_base.state == OR_CONN_STATE_OPEN &&
+    /* Never return a connection that shouldn't be used for circs. */
-        conn->_base.state != OR_CONN_STATE_OPEN)
+    if (conn->is_bad_for_new_circs) {
-      continue; /* avoid non-open conns if we can */
+      ++n_old;
-    newer = best->_base.timestamp_created < conn->_base.timestamp_created;
+      continue;
-
-    if (best->is_canonical && !conn->is_canonical)
-      continue; /* A canonical connection is best. */
-
-    if (!best->is_bad_for_new_circs && conn->is_bad_for_new_circs)
-      continue; /* We never prefer obsolete over non-obsolete connections. */
-
-    if (
-      /* We prefer canonical connections: */
-        (!best->is_canonical && conn->is_canonical) ||
-      /* We prefer non-obsolete connections: */
-        (best->is_bad_for_new_circs && !conn->is_bad_for_new_circs) ||
-      /* If both have circuits we prefer the newer: */
-        (best->n_circuits && conn->n_circuits && newer) ||
-      /* If neither has circuits we prefer the newer: */
-        (!best->n_circuits && !conn->n_circuits && newer) ||
-      /* We prefer connections with circuits: */
-        (!best->n_circuits && conn->n_circuits)) {
-      best = conn;
-    };
     }
+    /* Never return a non-canonical connection using a recent link protocol
+     * if the address is not what we wanted.
+     *
+     * (For old link protocols, we can't rely on is_canonical getting
+     * set properly if we're talking to the right address, since we might
+     * have an out-of-date descriptor, and we will get no NETINFO cell to
+     * tell us about the right address.) */
+    if (!conn->is_canonical && conn->link_proto >= 2 &&
+        tor_addr_compare(&conn->real_addr, target_addr, CMP_EXACT)) {
+      ++n_noncanonical;
+      continue;
+    }
+
+    ++n_possible;
+
+    if (!best) {
+      best = conn; /* If we have no 'best' so far, this one is good enough. */
+      continue;
+    }
+
+    if (connection_or_is_better(conn, best))
+      best = conn;
+  }
+
+  if (best) {
+    *msg_out = "Connection is fine; using it.";
+    *launch_out = 0;
     return best;
+  } else if (n_inprogress_goodaddr) {
+    *msg_out = "Connection in progress; waiting.";
+    *launch_out = 0;
+    return NULL;
+  } else if (n_old || n_noncanonical) {
+    *msg_out = "Connections all too old, or too non-canonical. "
+      " Launching a new one.";
+    *launch_out = 1;
+    return NULL;
+  } else {
+    *msg_out = "Not connected. Connecting.";
+    *launch_out = 1;
+    return NULL;
+  }
+}
+
+/** How old do we let a connection to an OR get before deciding it's
+ * too old for new circuits? */
+#define TIME_BEFORE_OR_CONN_IS_TOO_OLD (60*60*24*7)
+
+/** Given the head of the linked list for all the or_connections with a given
+ * identity, set elements of that list as is_bad_for_new_circs() as
+ * appropriate.  Helper for connection_or_set_bad_connections().
+ */
+static void
+connection_or_group_set_badness(or_connection_t *head)
+{
+  or_connection_t *or_conn = NULL, *best = NULL;
+  int n_old = 0, n_inprogress = 0, n_canonical = 0, n_other = 0;
+  time_t now = time(NULL);
+
+  /* Pass 1: expire everything that's old, and see what the status of
+   * everything else is. */
+  for (or_conn = head; or_conn; or_conn = or_conn->next_with_same_id) {
+    if (or_conn->_base.marked_for_close ||
+        or_conn->is_bad_for_new_circs)
+      continue;
+    if (or_conn->_base.timestamp_created + TIME_BEFORE_OR_CONN_IS_TOO_OLD
+        < now) {
+      log_info(LD_OR,
+               "Marking OR conn to %s:%d as too old for new circuits "
+               "(fd %d, %d secs old).",
+               or_conn->_base.address, or_conn->_base.port, or_conn->_base.s,
+               (int)(now - or_conn->_base.timestamp_created));
+      or_conn->is_bad_for_new_circs = 1;
+    }
+
+    if (or_conn->is_bad_for_new_circs) {
+      ++n_old;
+    } else if (or_conn->_base.state != OR_CONN_STATE_OPEN) {
+      ++n_inprogress;
+    } else if (or_conn->is_canonical) {
+      ++n_canonical;
+    } else {
+      ++n_other;
+    }
+  }
+
+  /* Pass 2: We know how about how good the best connection is.
+   * expire everything that's worse, and find the very best if we can. */
+  for (or_conn = head; or_conn; or_conn = or_conn->next_with_same_id) {
+    if (or_conn->_base.marked_for_close ||
+        or_conn->is_bad_for_new_circs)
+      continue; /* This one doesn't need to be marked bad. */
+    if (or_conn->_base.state != OR_CONN_STATE_OPEN)
+      continue; /* Don't mark anything bad until we have seen what happens
+                 * when the connection finishes. */
+    if (n_canonical && !or_conn->is_canonical) {
+      /* We have at least one open canonical connection to this router,
+       * and this one is open but not canonical.  Mark it bad. */
+      log_info(LD_OR,
+               "Marking OR conn to %s:%d as too old for new circuits: "
+               "(fd %d, %d secs old).  It is not canonical, and we have "
+               "another connection to that OR that is.",
+               or_conn->_base.address, or_conn->_base.port, or_conn->_base.s,
+               (int)(now - or_conn->_base.timestamp_created));
+      or_conn->is_bad_for_new_circs = 1;
+      continue;
+    }
+
+    if (!best || connection_or_is_better(or_conn, best))
+      best = or_conn;
+  }
+
+  if (!best)
+    return;
+
+  /* Pass 3: One connection to OR is best.  If it's canonical, mark as bad
+   * every other open connection.  If it's non-canonical, mark as bad
+   * every other open connection to the same address.
+   *
+   * XXXX021.
+   */
+  for (or_conn = head; or_conn; or_conn = or_conn->next_with_same_id) {
+    if (or_conn->_base.marked_for_close ||
+        or_conn->is_bad_for_new_circs ||
+        or_conn->_base.state != OR_CONN_STATE_OPEN)
+      continue;
+    if (or_conn != best) {
+      if (best->is_canonical) {
+        log_info(LD_OR,
+                 "Marking OR conn to %s:%d as too old for new circuits: "
+                 "(fd %d, %d secs old).  We have a better canonical one "
+                 "(fd %d; %d secs old).",
+                 or_conn->_base.address, or_conn->_base.port, or_conn->_base.s,
+                 (int)(now - or_conn->_base.timestamp_created),
+                 best->_base.s, (int)(now - best->_base.timestamp_created));
+        or_conn->is_bad_for_new_circs = 1;
+      } else if (!tor_addr_compare(&or_conn->real_addr,
+                                   &best->real_addr, CMP_EXACT)) {
+        log_info(LD_OR,
+                 "Marking OR conn to %s:%d as too old for new circuits: "
+                 "(fd %d, %d secs old).  We have a better one "
+                 "(fd %d; %d secs old).",
+                 or_conn->_base.address, or_conn->_base.port, or_conn->_base.s,
+                 (int)(now - or_conn->_base.timestamp_created),
+                 best->_base.s, (int)(now - best->_base.timestamp_created));
+        or_conn->is_bad_for_new_circs = 1;
+      }
+    }
+  }
+}
+
+/** Go through all the OR connections, and set the is_bad_for_new_circs
+ * flag on:
+ *    - all connections that are too old.
+ *    - all open non-canonical connections for which a canonical connection
+ *      exists to the same router.
+ *    - all open canonical connections for which a 'better' canonical
+ *      connection exists to the same router.
+ *    - all open non-canonical connections for which a 'better' non-canonical
+ *      connection exists to the same router at the same address.
+ *
+ * See connection_or_is_better() for our idea of what makes one OR connection
+ * better than another.
+ */
+void
+connection_or_set_bad_connections(void)
+{
+  if (!orconn_identity_map)
+    return;
+
+  DIGESTMAP_FOREACH(orconn_identity_map, identity, or_connection_t *, conn) {
+    connection_or_group_set_badness(conn);
+  } DIGESTMAP_FOREACH_END;
 }
 
 /** <b>conn</b> is in the 'connecting' state, and it failed to complete

src/or/main.c

@@ -99,9 +99,6 @@ int has_completed_circuit=0;
 /** How long do we let a directory connection stall before expiring it? */
 #define DIR_CONN_MAX_STALL (5*60)
 
-/** How old do we let a connection to an OR get before deciding it's
- * too old for new circuits? */
-#define TIME_BEFORE_OR_CONN_IS_TOO_OLD (60*60*24*7)
 /** How long do we let OR connections handshake before we decide that
  * they are obsolete? */
 #define TLS_HANDSHAKE_TIMEOUT (60)
@@ -715,39 +712,9 @@ run_connection_housekeeping(int i, time_t now)
 
   or_conn = TO_OR_CONN(conn);
 
-  if (!or_conn->is_bad_for_new_circs) {
-    if (conn->timestamp_created + TIME_BEFORE_OR_CONN_IS_TOO_OLD < now) {
-      log_info(LD_OR,
-               "Marking OR conn to %s:%d as too old for new circuits "
-               "(fd %d, %d secs old).",
-               conn->address, conn->port, conn->s,
-               (int)(now - conn->timestamp_created));
-      or_conn->is_bad_for_new_circs = 1;
-    } else {
-      or_connection_t *best =
-        connection_or_get_by_identity_digest(or_conn->identity_digest);
-      if (best && best != or_conn &&
-          (conn->state == OR_CONN_STATE_OPEN ||
-           now > conn->timestamp_created + TLS_HANDSHAKE_TIMEOUT)) {
-          /* We only mark as obsolete connections that already are in
-           * OR_CONN_STATE_OPEN, i.e. that have finished their TLS handshaking.
-           * This is necessary because authorities judge whether a router is
-           * reachable based on whether they were able to TLS handshake with it
-           * recently.  Without this check we would expire connections too
-           * early for router->last_reachable to be updated.
-           */
-        log_info(LD_OR,
-                 "Marking duplicate conn to %s:%d as too old for new circuits "
-                 "(fd %d, %d secs old).",
-                 conn->address, conn->port, conn->s,
-                 (int)(now - conn->timestamp_created));
-        or_conn->is_bad_for_new_circs = 1;
-      }
-    }
-  }
-
   if (or_conn->is_bad_for_new_circs && !or_conn->n_circuits) {
-    /* no unmarked circs -- mark it now */
+    /* It's bad for new circuits, and has no unmarked circuits on it:
+     * mark it now. */
     log_info(LD_OR,
              "Expiring non-used OR connection to fd %d (%s:%d) [Too old].",
              conn->s, conn->address, conn->port);
@@ -1095,6 +1062,7 @@ run_scheduled_events(time_t now)
     circuit_build_needed_circs(now);
 
   /** 5. We do housekeeping for each connection... */
+  connection_or_set_bad_connections();
   for (i=0;i<smartlist_len(connection_array);i++) {
     run_connection_housekeeping(i, now);
   }

src/or/or.h

@@ -3097,7 +3097,11 @@ int get_pf_socket(void);
 
 void connection_or_remove_from_identity_map(or_connection_t *conn);
 void connection_or_clear_identity_map(void);
-or_connection_t *connection_or_get_by_identity_digest(const char *digest);
+or_connection_t *connection_or_get_for_extend(const char *digest,
+                                              const tor_addr_t *target_addr,
+                                              const char **msg_out,
+                                              int *launch_out);
+void connection_or_set_bad_connections(void);
 
 int connection_or_reached_eof(or_connection_t *conn);
 int connection_or_process_inbuf(or_connection_t *conn);