commit b0fc18c37e0d30d9941109382df4b2b44f0f0ff8 Author: Mike Perry <mikeperry-git@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; } }