From 2b05a8c6716f1c7e07e658719c093723809e19a6 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 25 Mar 2013 16:04:30 -0700 Subject: [PATCH 1/4] Clip invalid path bias counts at startup. There was a bug in Tor prior to 0.2.4.10-alpha that allowed counts to become invalid. Clipping the counts at startup allows us to rule out log messages due to corruption from these prior Tor versions. --- src/or/entrynodes.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 6b21d1092b..3234f4f3c7 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1211,6 +1211,21 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) continue; } + if (use_cnt < success_cnt) { + int severity = LOG_INFO; + /* If this state file was written by a Tor that would have + * already fixed it, then the overcounting bug is still there.. */ + if (tor_version_as_new_as(state_version, "0.2.4.12-alpha")) { + severity = LOG_NOTICE; + } + log_fn(severity, LD_BUG, + "State file contains unexpectedly high usage success " + "counts %lf/%lf for Guard %s ($%s)", + success_cnt, use_cnt, + node->nickname, hex_str(node->identity, DIGEST_LEN)); + success_cnt = use_cnt; + } + node->use_attempts = use_cnt; node->use_successes = success_cnt; @@ -1261,6 +1276,21 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) unusable = 0; } + if (hop_cnt < success_cnt) { + int severity = LOG_INFO; + /* If this state file was written by a Tor that would have + * already fixed it, then the overcounting bug is still there.. */ + if (tor_version_as_new_as(state_version, "0.2.4.12-alpha")) { + severity = LOG_NOTICE; + } + log_fn(severity, LD_BUG, + "State file contains unexpectedly high success counts " + "%lf/%lf for Guard %s ($%s)", + success_cnt, hop_cnt, + node->nickname, hex_str(node->identity, DIGEST_LEN)); + success_cnt = hop_cnt; + } + node->circ_attempts = hop_cnt; node->circ_successes = success_cnt; From 56e7dff7bd4769c2a255aac7c9d266914bbf9f57 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 25 Mar 2013 16:05:42 -0700 Subject: [PATCH 2/4] Add additional checks for Path Bias scaling. Just in case more issues remain with scaling, it would be nice to pin-point them as such. --- src/or/circuitbuild.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 7db2b70bf7..df56877646 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1734,6 +1734,13 @@ pathbias_count_use_success(origin_circuit_t *circ) guard->use_successes++; entry_guards_changed(); + if (guard->use_attempts < guard->use_successes) { + log_notice(LD_BUG, "Unexpectedly high use successes counts (%f/%f) " + "for guard %s=%s", + guard->use_successes, guard->use_attempts, + guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + } + log_debug(LD_CIRC, "Marked circuit %d (%f/%f) as used successfully for guard " "%s ($%s).", @@ -2481,6 +2488,9 @@ pathbias_scale_close_rates(entry_guard_t *guard) int opened_built = pathbias_count_circs_in_states(guard, PATH_STATE_BUILD_SUCCEEDED, PATH_STATE_USE_FAILED); + /* Verify that the counts are sane before and after scaling */ + int counts_are_sane = (guard->circ_attempts >= guard->circ_successes); + guard->circ_attempts -= opened_attempts; guard->circ_successes -= opened_built; @@ -2502,6 +2512,16 @@ pathbias_scale_close_rates(entry_guard_t *guard) guard->circ_successes, guard->successful_circuits_closed, guard->circ_attempts, opened_built, opened_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + + /* Have the counts just become invalid by this scaling attempt? */ + if (counts_are_sane && guard->circ_attempts < guard->circ_successes) { + log_notice(LD_BUG, + "Scaling has mangled pathbias counts to %f/%f (%d/%d open) " + "for guard %s ($%s)", + guard->circ_successes, guard->circ_attempts, opened_built, + opened_attempts, guard->nickname, + hex_str(guard->identity, DIGEST_LEN)); + } } } @@ -2524,6 +2544,9 @@ pathbias_scale_use_rates(entry_guard_t *guard) double scale_ratio = pathbias_get_scale_ratio(options); int opened_attempts = pathbias_count_circs_in_states(guard, PATH_STATE_USE_ATTEMPTED, PATH_STATE_USE_SUCCEEDED); + /* Verify that the counts are sane before and after scaling */ + int counts_are_sane = (guard->use_attempts >= guard->use_successes); + guard->use_attempts -= opened_attempts; guard->use_attempts *= scale_ratio; @@ -2535,6 +2558,17 @@ pathbias_scale_use_rates(entry_guard_t *guard) "Scaled pathbias use counts to %f/%f (%d open) for guard %s ($%s)", guard->use_successes, guard->use_attempts, opened_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + + /* Have the counts just become invalid by this scaling attempt? */ + if (counts_are_sane && guard->use_attempts < guard->use_successes) { + log_notice(LD_BUG, + "Scaling has mangled pathbias usage counts to %f/%f " + "(%d open) for guard %s ($%s)", + guard->circ_successes, guard->circ_attempts, + opened_attempts, guard->nickname, + hex_str(guard->identity, DIGEST_LEN)); + } + entry_guards_changed(); } } From 3207ace6050ea0463cfc26d6e95d737785baa5fa Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 25 Mar 2013 16:06:26 -0700 Subject: [PATCH 3/4] Changes file. --- changes/bug8235-diagnosing | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug8235-diagnosing diff --git a/changes/bug8235-diagnosing b/changes/bug8235-diagnosing new file mode 100644 index 0000000000..b760035cfc --- /dev/null +++ b/changes/bug8235-diagnosing @@ -0,0 +1,5 @@ + o Minor features (diagnostic) + - If the state file's path bias counts are invalid (presumably from a + buggy tor prior to 0.2.4.10-alpha), make them correct. + - Add additional checks and log messages to the scaling of Path Bias + counts, in case there still are remaining issues with scaling. From 33b7083f26e30304f6c9296e3cb7205f6bda0c95 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 3 Apr 2013 09:36:37 -0400 Subject: [PATCH 4/4] Fix a wide line --- src/or/circuitbuild.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index df56877646..31242f6c14 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2555,9 +2555,9 @@ pathbias_scale_use_rates(entry_guard_t *guard) guard->use_attempts += opened_attempts; log_info(LD_CIRC, - "Scaled pathbias use counts to %f/%f (%d open) for guard %s ($%s)", - guard->use_successes, guard->use_attempts, opened_attempts, - guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + "Scaled pathbias use counts to %f/%f (%d open) for guard %s ($%s)", + guard->use_successes, guard->use_attempts, opened_attempts, + guard->nickname, hex_str(guard->identity, DIGEST_LEN)); /* Have the counts just become invalid by this scaling attempt? */ if (counts_are_sane && guard->use_attempts < guard->use_successes) {