[tor-commits] [tor/master] Improvements to #11553 fix based on review

[nickm at torproject.org]

commit a770b74501a3faf6483c40735b70adae6fb95187
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Apr 23 12:39:01 2014 -0400

    Improvements to #11553 fix based on review
    
    Use a per-channel ratelim_t to control the rate at which we report
    failures for each channel.
    
    Explain why I picked N=32.
    
    Never return a zero circID.
    
    Thanks to Andrea and to cypherpunks.
---
 src/or/channel.c      |    3 +++
 src/or/channel.h      |    6 ++++--
 src/or/circuitbuild.c |   19 +++++++++++++------
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/or/channel.c b/src/or/channel.c
index dfef703..691da17 100644
--- a/src/or/channel.c
+++ b/src/or/channel.c
@@ -731,6 +731,9 @@ channel_init(channel_t *chan)
   /* Init timestamp */
   chan->timestamp_last_added_nonpadding = time(NULL);
 
+  /* Warn about exhausted circuit IDs no more than hourly. */
+  chan->last_warned_circ_ids_exhausted.rate = 3600;
+
   /* Initialize queues. */
   TOR_SIMPLEQ_INIT(&chan->incoming_queue);
   TOR_SIMPLEQ_INIT(&chan->outgoing_queue);
diff --git a/src/or/channel.h b/src/or/channel.h
index e63c949..63da0c1 100644
--- a/src/or/channel.h
+++ b/src/or/channel.h
@@ -148,8 +148,6 @@ struct channel_s {
   ENUM_BF(circ_id_type_t) circ_id_type:2;
   /** DOCDOC*/
   unsigned wide_circ_ids:1;
-  /** Have we logged a warning about circID exhaustion on this channel? */
-  unsigned warned_circ_ids_exhausted:1;
 
   /* For how many circuits are we n_chan?  What about p_chan? */
   unsigned int num_n_circuits, num_p_circuits;
@@ -178,6 +176,10 @@ struct channel_s {
    */
   unsigned int is_local:1;
 
+  /** Have we logged a warning about circID exhaustion on this channel?
+   * If so, when? */
+  ratelim_t last_warned_circ_ids_exhausted;
+
   /** Channel timestamps for cell channels */
   time_t timestamp_client; /* Client used this, according to relay.c */
   time_t timestamp_drained; /* Output queue empty */
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 7b852ff..8d6aad6 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -102,6 +102,12 @@ channel_connect_for_circuit(const tor_addr_t *addr, uint16_t port,
 static circid_t
 get_unique_circ_id_by_chan(channel_t *chan)
 {
+/* This number is chosen somewhat arbitrarily; see comment below for more
+ * info.  When the space is 80% full, it gives a one-in-a-million failure
+ * chance; when the space is 90% full, it gives a one-in-850 chance; and when
+ * the space is 95% full, it gives a one-in-26 failure chance.  That seems
+ * okay, though you could make a case IMO for anything between N=32 and
+ * N=256. */
 #define MAX_CIRCID_ATTEMPTS 64
 
   circid_t test_circ_id;
@@ -137,19 +143,20 @@ get_unique_circ_id_by_chan(channel_t *chan)
        * whole circuit ID space every time we extend a circuit, which is
        * not so great either.
        */
-      if (! chan->warned_circ_ids_exhausted) {
-        chan->warned_circ_ids_exhausted = 1;
-        log_warn(LD_CIRC,"No unused circIDs found on channel %s wide "
+      log_fn_ratelim(&chan->last_warned_circ_ids_exhausted, LOG_WARN,
+                 LD_CIRC,"No unused circIDs found on channel %s wide "
                  "circID support, with %u inbound and %u outbound circuits. "
                  "Failing a circuit.",
                  chan->wide_circ_ids ? "with" : "without",
                  chan->num_p_circuits, chan->num_n_circuits);
-      }
       return 0;
     }
 
-    crypto_rand((char*) &test_circ_id, sizeof(test_circ_id));
-    test_circ_id &= mask;
+    do {
+      crypto_rand((char*) &test_circ_id, sizeof(test_circ_id));
+      test_circ_id &= mask;
+    } while (test_circ_id == 0);
+
     test_circ_id |= high_bit;
   } while (circuit_id_in_use_on_channel(test_circ_id, chan));
   return test_circ_id;