[tor-commits] [tor/master] Changes from Nick's code review 'part 1'
nickm at torproject.org
nickm at torproject.org
Wed Dec 26 04:34:55 UTC 2012
commit b0fc18c37e0d30d9941109382df4b2b44f0f0ff8
Author: Mike Perry <mikeperry-git at fscked.org>
Date: Tue Dec 18 12:39:03 2012 -0800
Changes from Nick's code review 'part 1'
I think this is actually his third code review of this branch so far.
---
src/or/circuitbuild.c | 90 +++++++++++++++++++++++++---------------------
src/or/config.c | 4 +-
src/or/connection_edge.c | 2 +-
src/or/entrynodes.c | 2 +-
src/or/relay.c | 12 +++---
5 files changed, 59 insertions(+), 51 deletions(-)
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index f93b04f..58020f2 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -1015,7 +1015,7 @@ pathbias_get_notice_rate(const or_options_t *options)
/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
/** The circuit success rate below which we issue a warn */
-double
+static double
pathbias_get_warn_rate(const or_options_t *options)
{
#define DFLT_PATH_BIAS_WARN_PCT 50
@@ -1055,7 +1055,7 @@ pathbias_get_dropguards(const or_options_t *options)
return options->PathBiasDropGuards;
else
return networkstatus_get_param(NULL, "pb_dropguards",
- DFLT_PATH_BIAS_DROP_GUARDS, 0, 100)/100.0;
+ DFLT_PATH_BIAS_DROP_GUARDS, 0, 1);
}
/**
@@ -1078,9 +1078,10 @@ pathbias_get_scale_threshold(const or_options_t *options)
/**
* The scale factor is the denominator for our scaling
- * of circuit counts for our path bias window. Note that
- * we must be careful of the values we use here, as the
- * code only scales in the event of no integer truncation.
+ * of circuit counts for our path bias window.
+ *
+ * Note that our use of doubles for the path bias state
+ * file means that powers of 2 work best here.
*/
static int
pathbias_get_scale_factor(const or_options_t *options)
@@ -1301,7 +1302,7 @@ pathbias_count_circ_attempt(origin_circuit_t *circ)
} else {
if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit,
approx_time()))) {
- log_info(LD_BUG,
+ log_info(LD_CIRC,
"Unopened circuit has no known guard. "
"Circuit is a %s currently %s.%s",
circuit_purpose_to_string(circ->base_.purpose),
@@ -1378,7 +1379,7 @@ pathbias_count_build_success(origin_circuit_t *circ)
} else if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
if ((rate_msg = rate_limit_log(&success_notice_limit,
approx_time()))) {
- log_info(LD_BUG,
+ log_info(LD_CIRC,
"Completed circuit has no known guard. "
"Circuit is a %s currently %s.%s",
circuit_purpose_to_string(circ->base_.purpose),
@@ -1490,7 +1491,7 @@ pathbias_count_successful_close(origin_circuit_t *circ)
/* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
* CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here.
* No need to log that case. */
- log_info(LD_BUG,
+ log_info(LD_CIRC,
"Successfully closed circuit has no known guard. "
"Circuit is a %s currently %s",
circuit_purpose_to_string(circ->base_.purpose),
@@ -1526,7 +1527,7 @@ pathbias_count_collapse(origin_circuit_t *circ)
/* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
* CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here.
* No need to log that case. */
- log_info(LD_BUG,
+ log_info(LD_CIRC,
"Destroyed circuit has no known guard. "
"Circuit is a %s currently %s",
circuit_purpose_to_string(circ->base_.purpose),
@@ -1554,7 +1555,7 @@ pathbias_count_unusable(origin_circuit_t *circ)
/* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
* CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here.
* No need to log that case. */
- log_info(LD_BUG,
+ log_info(LD_CIRC,
"Stream-failing circuit has no known guard. "
"Circuit is a %s currently %s",
circuit_purpose_to_string(circ->base_.purpose),
@@ -1620,10 +1621,9 @@ pathbias_get_closed_count(entry_guard_t *guard)
continue;
if (ocirc->path_state >= PATH_STATE_BUILD_SUCCEEDED &&
- (memcmp(guard->identity,
+ fast_memeq(guard->identity,
ocirc->cpath->extend_info->identity_digest,
- DIGEST_LEN)
- == 0)) {
+ DIGEST_LEN)) {
open_circuits++;
}
}
@@ -1670,15 +1670,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard)
"Your Guard %s=%s is failing an extremely large amount of "
"circuits. To avoid potential route manipluation attacks, "
"Tor has disabled use of this guard. "
- "Success counts are %d/%d. %d circuits completed, %d "
- "were unusable, %d collapsed, and %d timed out. For "
+ "Success counts are %ld/%ld. %ld circuits completed, %ld "
+ "were unusable, %ld collapsed, and %ld timed out. For "
"reference, your timeout cutoff is %ld seconds.",
guard->nickname, hex_str(guard->identity, DIGEST_LEN),
- (int)pathbias_get_closed_count(guard),
- (int)guard->circ_attempts, (int)guard->circ_successes,
- (int)guard->unusable_circuits,
- (int)guard->collapsed_circuits, (int)guard->timeouts,
- (long)circ_times.close_ms/1000);
+ tor_lround(pathbias_get_closed_count(guard)),
+ tor_lround(guard->circ_attempts),
+ tor_lround(guard->circ_successes),
+ tor_lround(guard->unusable_circuits),
+ tor_lround(guard->collapsed_circuits),
+ tor_lround(guard->timeouts),
+ tor_lround(circ_times.close_ms/1000));
guard->path_bias_disabled = 1;
guard->bad_since = approx_time();
return -1;
@@ -1689,15 +1691,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard)
"Your Guard %s=%s is failing an extremely large amount of "
"circuits. This could indicate a route manipulation attack, "
"extreme network overload, or a bug. "
- "Success counts are %d/%d. %d circuits completed, %d "
- "were unusable, %d collapsed, and %d timed out. For "
+ "Success counts are %ld/%ld. %ld circuits completed, %ld "
+ "were unusable, %ld collapsed, and %ld timed out. For "
"reference, your timeout cutoff is %ld seconds.",
guard->nickname, hex_str(guard->identity, DIGEST_LEN),
- (int)pathbias_get_closed_count(guard),
- (int)guard->circ_attempts, (int)guard->circ_successes,
- (int)guard->unusable_circuits,
- (int)guard->collapsed_circuits, (int)guard->timeouts,
- (long)circ_times.close_ms/1000);
+ tor_lround(pathbias_get_closed_count(guard)),
+ tor_lround(guard->circ_attempts),
+ tor_lround(guard->circ_successes),
+ tor_lround(guard->unusable_circuits),
+ tor_lround(guard->collapsed_circuits),
+ tor_lround(guard->timeouts),
+ tor_lround(circ_times.close_ms/1000));
}
} else if (pathbias_get_success_count(guard)/((double)guard->circ_attempts)
< pathbias_get_warn_rate(options)) {
@@ -1708,15 +1712,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard)
"circuits. Most likely this means the Tor network is "
"overloaded, but it could also mean an attack against "
"you or the potentially the guard itself. "
- "Success counts are %d/%d. %d circuits completed, %d "
- "were unusable, %d collapsed, and %d timed out. For "
+ "Success counts are %ld/%ld. %ld circuits completed, %ld "
+ "were unusable, %ld collapsed, and %ld timed out. For "
"reference, your timeout cutoff is %ld seconds.",
guard->nickname, hex_str(guard->identity, DIGEST_LEN),
- (int)pathbias_get_closed_count(guard),
- (int)guard->circ_attempts, (int)guard->circ_successes,
- (int)guard->unusable_circuits,
- (int)guard->collapsed_circuits, (int)guard->timeouts,
- (long)circ_times.close_ms/1000);
+ tor_lround(pathbias_get_closed_count(guard)),
+ tor_lround(guard->circ_attempts),
+ tor_lround(guard->circ_successes),
+ tor_lround(guard->unusable_circuits),
+ tor_lround(guard->collapsed_circuits),
+ tor_lround(guard->timeouts),
+ tor_lround(circ_times.close_ms/1000));
}
} else if (pathbias_get_success_count(guard)/((double)guard->circ_attempts)
< pathbias_get_notice_rate(options)) {
@@ -1725,15 +1731,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard)
log_notice(LD_CIRC,
"Your Guard %s=%s is failing more circuits than usual. "
"Most likely this means the Tor network is overloaded. "
- "Success counts are %d/%d. %d circuits completed, %d "
- "were unusable, %d collapsed, and %d timed out. For "
+ "Success counts are %ld/%ld. %ld circuits completed, %ld "
+ "were unusable, %ld collapsed, and %ld timed out. For "
"reference, your timeout cutoff is %ld seconds.",
guard->nickname, hex_str(guard->identity, DIGEST_LEN),
- (int)pathbias_get_closed_count(guard),
- (int)guard->circ_attempts, (int)guard->circ_successes,
- (int)guard->unusable_circuits,
- (int)guard->collapsed_circuits, (int)guard->timeouts,
- (long)circ_times.close_ms/1000);
+ tor_lround(pathbias_get_closed_count(guard)),
+ tor_lround(guard->circ_attempts),
+ tor_lround(guard->circ_successes),
+ tor_lround(guard->unusable_circuits),
+ tor_lround(guard->collapsed_circuits),
+ tor_lround(guard->timeouts),
+ tor_lround(circ_times.close_ms/1000));
}
}
}
diff --git a/src/or/config.c b/src/or/config.c
index 79725cb..1d81c54 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -322,8 +322,8 @@ static config_var_t option_vars_[] = {
V(PathBiasScaleThreshold, INT, "-1"),
V(PathBiasScaleFactor, INT, "-1"),
V(PathBiasMultFactor, INT, "-1"),
- V(PathBiasDropGuards, BOOL, "0"),
- V(PathBiasUseCloseCounts, BOOL, "1"),
+ V(PathBiasDropGuards, AUTOBOOL, "0"),
+ V(PathBiasUseCloseCounts, AUTOBOOL, "1"),
OBSOLETE("PathlenCoinWeight"),
V(PerConnBWBurst, MEMUNIT, "0"),
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 570ffe4..fa91c3a 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -2189,7 +2189,7 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply,
// DNS remaps can trigger this. So can failed hidden service
// lookups.
log_info(LD_BUG,
- "No origin circuit for successful SOCKS stream %ld. Reason: "
+ "No origin circuit for successful SOCKS stream %lu. Reason: "
"%d", ENTRY_TO_CONN(conn)->global_identifier, endreason);
} else {
TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->path_state
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index 066dbec..fc7f6ed 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -1194,7 +1194,7 @@ entry_guards_update_state(or_state_t *state)
d, e->chosen_by_version, t);
next = &(line->next);
}
- if (e->circ_attempts) {
+ if (e->circ_attempts > 0) {
*next = line = tor_malloc_zero(sizeof(config_line_t));
line->key = tor_strdup("EntryGuardPathBias");
/* In the long run: circuit_success ~= successful_circuit_close +
diff --git a/src/or/relay.c b/src/or/relay.c
index b4b7700..8b3b27f 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -698,17 +698,17 @@ connection_ap_process_end_not_open(
reason == END_STREAM_REASON_INTERNAL ||
reason == END_STREAM_REASON_DESTROY) {
/* All three of these reasons could mean a failed tag
- * hit the exit and it shat itself. Do not probe.
+ * hit the exit and it complained. Do not probe.
* Fail the circuit. */
circ->path_state = PATH_STATE_USE_FAILED;
return -END_CIRC_REASON_TORPROTOCOL;
} else {
/* Path bias: If we get a valid reason code from the exit,
- * it wasn't due to tagging */
- // XXX: This relies on recognized+digest being strong enough not
- // to be spoofable.. Is that a valid assumption?
- // Or more accurately: is it better than nothing? Can the attack
- // be done offline?
+ * it wasn't due to tagging.
+ *
+ * We rely on recognized+digest being strong enough to make
+ * tags unlikely to allow us to get tagged, yet 'recognized'
+ * reason codes here. */
circ->path_state = PATH_STATE_USE_SUCCEEDED;
}
}
More information about the tor-commits
mailing list