Bug 1245: Ignore negative and large timeouts.

This should prevent some asserts and storage of incorrect build times
for the cases where Tor is suspended during a circuit construction, or
just after completing a circuit. The idea is that if the circuit
build time is much greater than we would have cut it off at, we probably
had a suspend event along this codepath, and we should discard the
value.

2 files changed, 28 insertions, 9 deletions

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index d597b752ad..0840e304f1 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -308,9 +308,9 @@ circuit_build_times_rewind_history(circuit_build_times_t *cbt, int n)
 int
 circuit_build_times_add_time(circuit_build_times_t *cbt, build_time_t time)
 {
-  tor_assert(time <= CBT_BUILD_TIME_MAX);
-  if (time <= 0) {
+  if (time <= 0 || time > CBT_BUILD_TIME_MAX) {
     log_warn(LD_CIRC, "Circuit build time is %u!", time);
+    tor_fragile_assert();
     return -1;
   }
 
@@ -1760,11 +1760,19 @@ circuit_send_next_onion_skin(origin_circuit_t *circ)
         long timediff;
         tor_gettimeofday(&end);
         timediff = tv_mdiff(&circ->_base.highres_created, &end);
-        if (timediff > INT32_MAX)
-          timediff = INT32_MAX;
-        circuit_build_times_add_time(&circ_times, (build_time_t)timediff);
-        circuit_build_times_network_circ_success(&circ_times);
-        circuit_build_times_set_timeout(&circ_times);
+        /*
+         * If the circuit build time is much greater than we would have cut
+         * it off at, we probably had a suspend event along this codepath,
+         * and we should discard the value.
+         */
+        if (timediff < 0 || timediff > 2*circ_times.timeout_ms+1000) {
+          log_notice(LD_CIRC, "Strange value for circuit build time: %ld. "
+                              "Assuming clock jump.", timediff);
+        } else {
+          circuit_build_times_add_time(&circ_times, (build_time_t)timediff);
+          circuit_build_times_network_circ_success(&circ_times);
+          circuit_build_times_set_timeout(&circ_times);
+        }
       }
       log_info(LD_CIRC,"circuit built!");
       circuit_reset_failure_count(0);
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index 7e47e60559..e7d10a22f2 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -350,9 +350,20 @@ circuit_expire_building(time_t now)
     } else { /* circuit not open, consider recording failure as timeout */
       int first_hop_succeeded = TO_ORIGIN_CIRCUIT(victim)->cpath &&
             TO_ORIGIN_CIRCUIT(victim)->cpath->state == CPATH_STATE_OPEN;
-      if (circuit_build_times_add_timeout(&circ_times, first_hop_succeeded,
-                                          victim->timestamp_created))
+      /*
+       * If the circuit build time is much greater than we would have cut
+       * it off at, we probably had a suspend event along this codepath,
+       * and we should discard the value.
+       */
+      if (now - victim->timestamp_created > (2*circ_times.timeout_ms)/1000+1) {
+        log_notice(LD_CIRC,
+                   "Extremely large value for circuit build timeout: %ld. "
+                   "Assuming clock jump.", now - victim->timestamp_created);
+      } else if (circuit_build_times_add_timeout(&circ_times,
+                                          first_hop_succeeded,
+                                          victim->timestamp_created)) {
         circuit_build_times_set_timeout(&circ_times);
+      }
     }
 
     if (victim->n_conn)