commit a770b74501a3faf6483c40735b70adae6fb95187 Author: Nick Mathewson nickm@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;