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

1 files changed, 33 insertions, 3 deletions

diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index c0612039be..7ee58fc5fe 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;
+}