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, 6 insertions, 12 deletions

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index fcbcb95919..76c5f6a9f4 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;
   }