[tor-commits] [tor/master] Stop frobbing timestamp_dirty as our sole means to mark circuits unusable
nickm at torproject.org
nickm at torproject.org
Mon Mar 18 20:37:03 UTC 2013
commit 62fb209d837f3f5510075ef8bdb6e231ebdfa9bc
Author: Nick Mathewson <nickm at torproject.org>
Date: Tue Feb 19 18:29:17 2013 -0500
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).
---
changes/bug6174 | 6 ++++++
src/or/circuitlist.c | 9 ++++-----
src/or/circuituse.c | 36 +++++++++++++++++++++++++++++++++---
src/or/circuituse.h | 1 +
src/or/connection_edge.c | 18 ++++++------------
src/or/or.h | 4 ++++
src/or/relay.c | 5 ++---
7 files changed, 56 insertions(+), 23 deletions(-)
diff --git a/changes/bug6174 b/changes/bug6174
new file mode 100644
index 0000000..79d2930
--- /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 ef32680..ae0955d 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 c061203..7ee58fc 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 d4d68aa..11e5a64 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 fcbcb95..76c5f6a 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 45eb467..e5c6045 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 9ff9e1e..fb15141 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 */
More information about the tor-commits
mailing list