Don't obsolete a very-new connection for having no circuits yet.

This fixes the last known case of bug 891, which could happen if two
hosts, A and B, disagree about how long a circuit has been open,
because of clock drift of some kind.  Host A would then mark the
connection as is_bad_for_new_circs when it got too old and open a new
connection.  In between when B receives a NETINFO cell on the new
conn, and when B receives a conn cell on the new circuit, the new
circuit will seem worse to B than the old one, and so B will mark it
as is_bad_for_new_circs in the second or third loop of
connection_or_group_set_badness().

Bugfix on 0.1.1.13-alpha.  Bug found by rovv.

Not a backport candidate: the bug is too obscure and the fix too tricky.

svn:r18303

2 files changed, 42 insertions, 14 deletions

diff --git a/ChangeLog b/ChangeLog
index 0cbde4e68d..c8512c9eab 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -15,6 +15,11 @@ Changes in version 0.2.1.12-alpha - 2009-02-??
       headers.  Bugfix on 0.2.0.10-alpha.
     - Don't consider consider expiring already-closed client connections.
       Fixes bug 893.  Bugfix on 0.0.2pre20.
+    - Fix another interesting corner-case of bug 891 spotted by rovv:
+      Previously, if two hosts had different amounts of clock drift, and one
+      of them created a new connection with just the wrong timing, the other
+      might decide to deprecate the new connection erroneously.  Bugfix on
+      0.1.1.13-alpha.
 
   o Minor features:
     - Support platforms where time_t is 64 bits long. (Congratulations,
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index 25dfc9b831..8ccc9b2634 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -441,12 +441,21 @@ connection_or_init_conn_from_address(or_connection_t *conn,
  *
  * Requires that both input connections are open; not is_bad_for_new_circs,
  * and not impossibly non-canonical.
+ *
+ * If </b>forgive_new_connections</b> is true, then we do not call
+ * <b>a</b>better than <b>b</b> simply because b has no circuits,
+ * unless b is also relatively old.
  */
 static int
-connection_or_is_better(const or_connection_t *a,
-                        const or_connection_t *b)
+connection_or_is_better(time_t now,
+                        const or_connection_t *a,
+                        const or_connection_t *b,
+                        int forgive_new_connections)
 {
   int newer;
+/** Do not definitively deprecate a new connection with no circuits on it
+ * until this much time has passed. */
+#define NEW_CONN_GRACE_PERIOD (15*60)
 
   if (b->is_canonical && !a->is_canonical)
     return 0; /* A canonical connection is better than a non-canonical
@@ -454,15 +463,26 @@ connection_or_is_better(const or_connection_t *a,
 
   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);
+  if (
+      /* 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))
+    return 1;
+
+  /* If one has no circuits and the other does... */
+  if (!b->n_circuits && a->n_circuits) {
+    /* Then it's bad, unless it's in its grace period and we're forgiving. */
+    if (forgive_new_connections &&
+        now < b->_base.timestamp_created + NEW_CONN_GRACE_PERIOD)
+      return 0;
+    else
+      return 1;
+  }
+
+  return 0;
 }
 
 /** Return the OR connection we should use to extend a circuit to the router
@@ -480,6 +500,7 @@ connection_or_get_for_extend(const char *digest,
 {
   or_connection_t *conn, *best=NULL;
   int n_inprogress_goodaddr = 0, n_old = 0, n_noncanonical = 0, n_possible = 0;
+  time_t now = approx_time();
 
   tor_assert(msg_out);
   tor_assert(launch_out);
@@ -531,7 +552,7 @@ connection_or_get_for_extend(const char *digest,
       continue;
     }
 
-    if (connection_or_is_better(conn, best))
+    if (connection_or_is_better(now, conn, best, 0))
       best = conn;
   }
 
@@ -619,7 +640,7 @@ connection_or_group_set_badness(or_connection_t *head)
       continue;
     }
 
-    if (!best || connection_or_is_better(or_conn, best))
+    if (!best || connection_or_is_better(now, or_conn, best, 0))
       best = or_conn;
   }
 
@@ -645,7 +666,9 @@ connection_or_group_set_badness(or_connection_t *head)
         or_conn->is_bad_for_new_circs ||
         or_conn->_base.state != OR_CONN_STATE_OPEN)
       continue;
-    if (or_conn != best) {
+    if (or_conn != best && connection_or_is_better(now, best, or_conn, 1)) {
+      /* This isn't the best conn, _and_ the best conn is better than it,
+         even when we're being forgiving. */
       if (best->is_canonical) {
         log_info(LD_OR,
                  "Marking OR conn to %s:%d as too old for new circuits: "