Changes from Nick's code review 'part 1'

I think this is actually his third code review of this branch so far.

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index f93b04f579cf4a3ca9893800012d575c60588c1e..58020f25ba2e09919ae17ca910cbaec7df1319e7 100644 (file)
--- 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 79725cbe0329a348808acd05b15d746d1ff0aee7..1d81c540a828aa06618593a0d2ef32a63b103a9a 100644 (file)
--- 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 570ffe4941062357c94103c35750e507b33bbb67..fa91c3a1baad39c12cca9a4e043a074cd8629c49 100644 (file)
--- 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 066dbecc2a6f6145cb74bef7c003ad4ac7d13069..fc7f6ed352bac48a22810f7f389f9088948bcca9 100644 (file)
--- 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 b4b77007cd32b673c0dec08766d19c75cff96ca8..8b3b27f036c545bdc656c3a916a517e80b0698f3 100644 (file)
--- 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;
     }
   }