Fix 1108: Handle corrupt or large build times state.

1108 was actually just a fencepost error in an assert,
but making the state file handling code resilient is a
good idea.

2 files changed, 50 insertions, 20 deletions

diff --git a/ChangeLog b/ChangeLog
index c79c865efa..05e4373b38 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,9 @@
 Changes in version 0.2.2.4-alpha - 2009-??-??
+  o Major bugfixes:
+    - Fix another assert in the circuit_build_times code that causes Tor
+	  to fail to start once we have accumulated 5000 build times in the
+	  state file. Bugfix on 0.2.2.2-alpha; fixes bug 1108.
+
   o Minor features:
     - Log SSL state transitions at debug level during handshake, and
       include SSL states in error messages.  This may help debug
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 5c3a86c123..2e3465d6fe 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -372,18 +372,29 @@ circuit_build_times_update_state(circuit_build_times_t *cbt,
  * Stolen from http://en.wikipedia.org/wiki/Fisher\u2013Yates_shuffle
  */
 static void
-circuit_build_times_shuffle_array(circuit_build_times_t *cbt)
+circuit_build_times_shuffle_and_store_array(circuit_build_times_t *cbt,
+                                            build_time_t *raw_times,
+                                            int num_times)
 {
-   int n = cbt->total_build_times;
-
-   /* This code can only be run on a compact array */
-   tor_assert(cbt->total_build_times == cbt->build_times_idx);
-   while (n-- > 1) {
-     int k = crypto_rand_int(n + 1); /* 0 <= k <= n. */
-     build_time_t tmp = cbt->circuit_build_times[k];
-     cbt->circuit_build_times[k] = cbt->circuit_build_times[n];
-     cbt->circuit_build_times[n] = tmp;
-   }
+  int n = num_times;
+  if (num_times > NCIRCUITS_TO_OBSERVE) {
+    log_notice(LD_CIRC, "Decreasing circuit_build_times size from %d to %d",
+               num_times, NCIRCUITS_TO_OBSERVE);
+  }
+
+  /* This code can only be run on a compact array */
+  while (n-- > 1) {
+    int k = crypto_rand_int(n + 1); /* 0 <= k <= n. */
+    build_time_t tmp = raw_times[k];
+    raw_times[k] = raw_times[n];
+    raw_times[n] = tmp;
+  }
+
+  /* Since the times are now shuffled, take a random NCIRCUITS_TO_OBSERVE
+   * subset (ie the first NCIRCUITS_TO_OBSERVE values) */
+  for (n = 0; n < MIN(num_times, NCIRCUITS_TO_OBSERVE); n++) {
+    circuit_build_times_add_time(cbt, raw_times[n]);
+  }
 }
 
 /**
@@ -397,14 +408,14 @@ int
 circuit_build_times_parse_state(circuit_build_times_t *cbt,
                                 or_state_t *state, char **msg)
 {
-  int tot_values = 0, N = 0;
+  int tot_values = 0;
+  uint32_t loaded_cnt = 0, N = 0;
   config_line_t *line;
   int i;
-  *msg = NULL;
+  build_time_t *loaded_times = tor_malloc(sizeof(build_time_t)
+                                          * state->TotalBuildTimes);
   circuit_build_times_init(cbt);
-
-  /* We don't support decreasing the table size yet */
-  tor_assert(state->TotalBuildTimes <= NCIRCUITS_TO_OBSERVE);
+  *msg = NULL;
 
   for (line = state->BuildtimeHistogram; line; line = line->next) {
     smartlist_t *args = smartlist_create();
@@ -441,17 +452,30 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
         break;
       }
 
+      if (loaded_cnt+count > state->TotalBuildTimes) {
+        log_warn(LD_CIRC,
+                 "Too many build times in state file. "
+                 "Stopping short before %d",
+                 loaded_cnt+count);
+        break;
+      }
+
       for (k = 0; k < count; k++) {
-        circuit_build_times_add_time(cbt, ms);
+        loaded_times[loaded_cnt++] = ms;
       }
       N++;
       SMARTLIST_FOREACH(args, char*, cp, tor_free(cp));
       smartlist_free(args);
     }
+  }
 
+  if (loaded_cnt != state->TotalBuildTimes) {
+    log_warn(LD_CIRC,
+            "Corrupt state file? Build times count mismatch. "
+            "Read %d, file says %d", loaded_cnt, state->TotalBuildTimes);
   }
 
-  circuit_build_times_shuffle_array(cbt);
+  circuit_build_times_shuffle_and_store_array(cbt, loaded_times, loaded_cnt);
 
   /* Verify that we didn't overwrite any indexes */
   for (i=0; i < NCIRCUITS_TO_OBSERVE; i++) {
@@ -462,9 +486,10 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
   log_info(LD_CIRC,
            "Loaded %d/%d values from %d lines in circuit time histogram",
            tot_values, cbt->total_build_times, N);
-  tor_assert(cbt->total_build_times == state->TotalBuildTimes);
-  tor_assert(tot_values == cbt->total_build_times);
+  tor_assert(cbt->total_build_times == tot_values);
+  tor_assert(cbt->total_build_times <= NCIRCUITS_TO_OBSERVE);
   circuit_build_times_set_timeout(cbt);
+  tor_free(loaded_times);
   return *msg ? -1 : 0;
 }