Stop frobbing timestamp_dirty as our sole means to mark circuits unusable

In a number of places, we decrement timestamp_dirty by
MaxCircuitDirtiness in order to mark a stream as "unusable for any
new connections.

This pattern sucks for a few reasons:
  * It is nonobvious.
  * It is error-prone: decrementing 0 can be a bad choice indeed.
  * It really wants to have a function.

It can also introduce bugs if the system time jumps backwards, or if
MaxCircuitDirtiness is increased.

So in this patch, I add an unusable_for_new_conns flag to
origin_circuit_t, make it get checked everywhere it should (I looked
for things that tested timestamp_dirty), and add a new function to
frob it.

For now, the new function does still frob timestamp_dirty (after
checking for underflow and whatnot), in case I missed any cases that
should be checking unusable_for_new_conns.

Fixes bug 6174. We first used this pattern in 516ef41ac1fd26f338c,
which I think was in 0.0.2pre26 (but it could have been 0.0.2pre27).

7 files changed, 56 insertions, 23 deletions

diff --git a/changes/bug6174 b/changes/bug6174
new file mode 100644
index 000000000..79d2930ec
--- /dev/null
+++ b/changes/bug6174
@@ -0,0 +1,6 @@
+  o Major bugfixes:
+    - When we mark a circuit as unusable for new circuits, have it
+      continue to be unusable for new circuits even if MaxCircuitDirtiness
+      is increased too much at the wrong time, or the system clock jumped
+      backwards. Fix for bug 6174; bugfix on 0.0.2pre26.
+     
diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index ef3268073..ae0955d12 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -1204,6 +1204,7 @@ circuit_find_to_cannibalize(uint8_t purpose, extend_info_t *info,
       if ((!need_uptime || circ->build_state->need_uptime) &&
           (!need_capacity || circ->build_state->need_capacity) &&
           (internal == circ->build_state->is_internal) &&
+          !circ->unusable_for_new_conns &&
           circ->remaining_relay_early_cells &&
           circ->build_state->desired_path_len == DEFAULT_ROUTE_LEN &&
           !circ->build_state->onehop_tunnel &&
@@ -1304,15 +1305,13 @@ void
 circuit_expire_all_dirty_circs(void)
 {
   circuit_t *circ;
-  const or_options_t *options = get_options();
 
   for (circ=global_circuitlist; circ; circ = circ->next) {
     if (CIRCUIT_IS_ORIGIN(circ) &&
         !circ->marked_for_close &&
-        circ->timestamp_dirty)
-      /* XXXX024 This is a screwed-up way to say "This is too dirty
-       * for new circuits. */
-      circ->timestamp_dirty -= options->MaxCircuitDirtiness;
+        circ->timestamp_dirty) {
+      mark_circuit_unusable_for_new_conns(TO_ORIGIN_CIRCUIT(circ));
+    }
   }
 }
 
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index c0612039b..7ee58fc5f 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -85,10 +85,14 @@ circuit_is_acceptable(const origin_circuit_t *origin_circ,
   }
 
   if (purpose == CIRCUIT_PURPOSE_C_GENERAL ||
-      purpose == CIRCUIT_PURPOSE_C_REND_JOINED)
+      purpose == CIRCUIT_PURPOSE_C_REND_JOINED) {
     if (circ->timestamp_dirty &&
        circ->timestamp_dirty+get_options()->MaxCircuitDirtiness <= now)
       return 0;
+  }
+
+  if (origin_circ->unusable_for_new_conns)
+    return 0;
 
   /* decide if this circ is suitable for this conn */
 
@@ -798,9 +802,12 @@ circuit_stream_is_being_handled(entry_connection_t *conn,
         circ->purpose == CIRCUIT_PURPOSE_C_GENERAL &&
         (!circ->timestamp_dirty ||
          circ->timestamp_dirty + get_options()->MaxCircuitDirtiness > now)) {
-      cpath_build_state_t *build_state = TO_ORIGIN_CIRCUIT(circ)->build_state;
+      origin_circuit_t *origin_circ = TO_ORIGIN_CIRCUIT(circ);
+      cpath_build_state_t *build_state = origin_circ->build_state;
       if (build_state->is_internal || build_state->onehop_tunnel)
         continue;
+      if (!origin_circ->unusable_for_new_conns)
+        continue;
 
       exitnode = build_state_get_exit_node(build_state);
       if (exitnode && (!need_uptime || build_state->need_uptime)) {
@@ -842,6 +849,7 @@ circuit_predict_and_launch_new(void)
   /* First, count how many of each type of circuit we have already. */
   for (circ=global_circuitlist;circ;circ = circ->next) {
     cpath_build_state_t *build_state;
+    origin_circuit_t *origin_circ;
     if (!CIRCUIT_IS_ORIGIN(circ))
       continue;
     if (circ->marked_for_close)
@@ -850,7 +858,10 @@ circuit_predict_and_launch_new(void)
       continue; /* only count clean circs */
     if (circ->purpose != CIRCUIT_PURPOSE_C_GENERAL)
       continue; /* only pay attention to general-purpose circs */
-    build_state = TO_ORIGIN_CIRCUIT(circ)->build_state;
+    origin_circ = TO_ORIGIN_CIRCUIT(circ);
+    if (origin_circ->unusable_for_new_conns)
+      continue;
+    build_state = origin_circ->build_state;
     if (build_state->onehop_tunnel)
       continue;
     num++;
@@ -2272,3 +2283,22 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose)
   }
 }
 
+/** Mark <b>circ</b> so that no more connections can be attached to it. */
+void
+mark_circuit_unusable_for_new_conns(origin_circuit_t *circ)
+{
+  const or_options_t *options = get_options();
+  tor_assert(circ);
+
+  /* XXXX025 This is a kludge; we're only keeping it around in case there's
+   * something that doesn't check unusable_for_new_conns, and to avoid
+   * deeper refactoring of our expiration logic. */
+  if (! circ->base_.timestamp_dirty)
+    circ->base_.timestamp_dirty = approx_time();
+  if (options->MaxCircuitDirtiness >= circ->base_.timestamp_dirty)
+    circ->base_.timestamp_dirty = 1; /* prevent underflow */
+  else
+    circ->base_.timestamp_dirty -= options->MaxCircuitDirtiness;
+
+  circ->unusable_for_new_conns = 1;
+}
diff --git a/src/or/circuituse.h b/src/or/circuituse.h
index d4d68aad9..11e5a6416 100644
--- a/src/or/circuituse.h
+++ b/src/or/circuituse.h
@@ -55,6 +55,7 @@ void circuit_change_purpose(circuit_t *circ, uint8_t new_purpose);
 
 int hostname_in_track_host_exits(const or_options_t *options,
                                  const char *address);
+void mark_circuit_unusable_for_new_conns(origin_circuit_t *circ);
 
 #endif
 
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index fcbcb9591..76c5f6a9f 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -674,12 +674,10 @@ connection_ap_expire_beginning(void)
     /* un-mark it as ending, since we're going to reuse it */
     conn->edge_has_sent_end = 0;
     conn->end_reason = 0;
-    /* 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 */
-    /* XXXX024 this is a kludgy way to do this. */
-    tor_assert(circ->timestamp_dirty);
-    circ->timestamp_dirty -= options->MaxCircuitDirtiness;
+    /* make us not try this circuit again, but allow
+     * current streams on it to survive if they can */
+    mark_circuit_unusable_for_new_conns(TO_ORIGIN_CIRCUIT(circ));
+
     /* give our stream another 'cutoff' seconds to try */
     conn->base_.timestamp_lastread += cutoff;
     if (entry_conn->num_socks_retries < 250) /* avoid overflow */
@@ -1806,9 +1804,7 @@ connection_ap_handshake_send_begin(entry_connection_t *ap_conn)
     connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL);
 
     /* Mark this circuit "unusable for new streams". */
-    /* XXXX024 this is a kludgy way to do this. */
-    tor_assert(circ->base_.timestamp_dirty);
-    circ->base_.timestamp_dirty -= get_options()->MaxCircuitDirtiness;
+    mark_circuit_unusable_for_new_conns(circ);
     return -1;
   }
 
@@ -1899,9 +1895,7 @@ connection_ap_handshake_send_resolve(entry_connection_t *ap_conn)
     connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL);
 
     /* Mark this circuit "unusable for new streams". */
-    /* XXXX024 this is a kludgy way to do this. */
-    tor_assert(circ->base_.timestamp_dirty);
-    circ->base_.timestamp_dirty -= get_options()->MaxCircuitDirtiness;
+    mark_circuit_unusable_for_new_conns(circ);
     return -1;
   }
 
diff --git a/src/or/or.h b/src/or/or.h
index 45eb4673c..e5c60452f 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -2944,6 +2944,10 @@ typedef struct origin_circuit_t {
    */
   ENUM_BF(path_state_t) path_state : 3;
 
+  /* If this flag is set, we should not consider attaching any more
+   * connections to this circuit. */
+  unsigned int unusable_for_new_conns : 1;
+
   /**
    * Tristate variable to guard against pathbias miscounting
    * due to circuit purpose transitions changing the decision
diff --git a/src/or/relay.c b/src/or/relay.c
index 9ff9e1e1f..fb15141ef 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -17,6 +17,7 @@
 #include "channel.h"
 #include "circuitbuild.h"
 #include "circuitlist.h"
+#include "circuituse.h"
 #include "config.h"
 #include "connection.h"
 #include "connection_edge.h"
@@ -851,9 +852,7 @@ connection_ap_process_end_not_open(
           /* 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". */
-          /* XXXX024 this is a kludgy way to do this. */
-          tor_assert(circ->base_.timestamp_dirty);
-          circ->base_.timestamp_dirty -= get_options()->MaxCircuitDirtiness;
+          mark_circuit_unusable_for_new_conns(circ);
 
           if (conn->chosen_exit_optional) {
             /* stop wanting a specific exit */