[tor-commits] [tor/master] Fix issues from dgoulet's code review.
nickm at torproject.org
nickm at torproject.org
Mon May 8 18:00:18 UTC 2017
commit 02a5835c27780e45f705fc1c044b9c471b929dbe
Author: Mike Perry <mikeperry-git at torproject.org>
Date: Thu Apr 20 15:00:46 2017 -0400
Fix issues from dgoulet's code review.
https://gitlab.com/dgoulet/tor/merge_requests/24
---
src/or/channelpadding.c | 73 +++++++++++++++++++++++++++++++------------------
src/or/channelpadding.h | 3 +-
src/or/channeltls.c | 20 ++++++++------
src/or/command.c | 29 +++++++-------------
src/or/connection_or.c | 2 +-
src/or/relay.c | 4 +--
6 files changed, 73 insertions(+), 58 deletions(-)
diff --git a/src/or/channelpadding.c b/src/or/channelpadding.c
index a84994a..2370827 100644
--- a/src/or/channelpadding.c
+++ b/src/or/channelpadding.c
@@ -47,6 +47,28 @@ static int consensus_nf_pad_before_usage;
/** Should we pad relay-to-relay connections? */
static int consensus_nf_pad_relays;
+#define TOR_MSEC_PER_SEC 1000
+#define TOR_USEC_PER_MSEC 1000
+
+/**
+ * How often do we get called by the connection housekeeping (ie: once
+ * per second) */
+#define TOR_HOUSEKEEPING_CALLBACK_MSEC 1000
+/**
+ * Additional extra time buffer on the housekeeping callback, since
+ * it can be delayed. This extra slack is used to decide if we should
+ * schedule a timer or wait for the next callback. */
+#define TOR_HOUSEKEEPING_CALLBACK_SLACK_MSEC 100
+
+/**
+ * This macro tells us if either end of the channel is connected to a client.
+ * (If we're not a server, we're definitely a client. If the channel thinks
+ * its a client, use that. Then finally verify in the consensus).
+ */
+#define CHANNEL_IS_CLIENT(chan, options) \
+ (!public_server_mode((options)) || (chan)->is_client || \
+ !connection_or_digest_is_known_relay((chan)->identity_digest))
+
/**
* This function is called to update cached consensus parameters every time
* there is a consensus update. This allows us to move the consensus param
@@ -64,7 +86,7 @@ channelpadding_new_consensus_params(networkstatus_t *ns)
DFLT_NETFLOW_INACTIVE_KEEPALIVE_LOW,
DFLT_NETFLOW_INACTIVE_KEEPALIVE_MIN,
DFLT_NETFLOW_INACTIVE_KEEPALIVE_MAX);
- consensus_nf_ito_high = networkstatus_get_param(NULL, "nf_ito_high",
+ consensus_nf_ito_high = networkstatus_get_param(ns, "nf_ito_high",
DFLT_NETFLOW_INACTIVE_KEEPALIVE_HIGH,
consensus_nf_ito_low,
DFLT_NETFLOW_INACTIVE_KEEPALIVE_MAX);
@@ -74,13 +96,13 @@ channelpadding_new_consensus_params(networkstatus_t *ns)
#define DFLT_NETFLOW_REDUCED_KEEPALIVE_MIN 0
#define DFLT_NETFLOW_REDUCED_KEEPALIVE_MAX 60000
consensus_nf_ito_low_reduced =
- networkstatus_get_param(NULL, "nf_ito_low_reduced",
+ networkstatus_get_param(ns, "nf_ito_low_reduced",
DFLT_NETFLOW_REDUCED_KEEPALIVE_LOW,
DFLT_NETFLOW_REDUCED_KEEPALIVE_MIN,
DFLT_NETFLOW_REDUCED_KEEPALIVE_MAX);
consensus_nf_ito_high_reduced =
- networkstatus_get_param(NULL, "nf_ito_high_reduced",
+ networkstatus_get_param(ns, "nf_ito_high_reduced",
DFLT_NETFLOW_REDUCED_KEEPALIVE_HIGH,
consensus_nf_ito_low_reduced,
DFLT_NETFLOW_REDUCED_KEEPALIVE_MAX);
@@ -89,7 +111,7 @@ channelpadding_new_consensus_params(networkstatus_t *ns)
#define CONNTIMEOUT_RELAYS_MIN 60
#define CONNTIMEOUT_RELAYS_MAX (7*24*60*60) // 1 week
consensus_nf_conntimeout_relays =
- networkstatus_get_param(NULL, "nf_conntimeout_relays",
+ networkstatus_get_param(ns, "nf_conntimeout_relays",
CONNTIMEOUT_RELAYS_DFLT,
CONNTIMEOUT_RELAYS_MIN,
CONNTIMEOUT_RELAYS_MAX);
@@ -98,16 +120,16 @@ channelpadding_new_consensus_params(networkstatus_t *ns)
#define CIRCTIMEOUT_CLIENTS_MIN 60
#define CIRCTIMEOUT_CLIENTS_MAX (24*60*60) // 24 hours
consensus_nf_conntimeout_clients =
- networkstatus_get_param(NULL, "nf_conntimeout_clients",
+ networkstatus_get_param(ns, "nf_conntimeout_clients",
CIRCTIMEOUT_CLIENTS_DFLT,
CIRCTIMEOUT_CLIENTS_MIN,
CIRCTIMEOUT_CLIENTS_MAX);
consensus_nf_pad_before_usage =
- networkstatus_get_param(NULL, "nf_pad_before_usage", 1, 0, 1);
+ networkstatus_get_param(ns, "nf_pad_before_usage", 1, 0, 1);
consensus_nf_pad_relays =
- networkstatus_get_param(NULL, "nf_pad_relays", 0, 0, 1);
+ networkstatus_get_param(ns, "nf_pad_relays", 0, 0, 1);
}
/**
@@ -214,10 +236,10 @@ channelpadding_update_padding_for_channel(channel_t *chan,
// We should not allow malicious relays to disable or reduce padding for
// us as clients. In fact, we should only accept this cell at all if we're
- // operating as a relay. Brides should not accept it from relays, either
+ // operating as a relay. Bridges should not accept it from relays, either
// (only from their clients).
if ((get_options()->BridgeRelay &&
- connection_or_digest_is_known_relay(chan->identity_digest)) ||
+ connection_or_digest_is_known_relay(chan->identity_digest)) ||
!get_options()->ORPort_set) {
static ratelim_t relay_limit = RATELIM_INIT(600);
@@ -238,7 +260,7 @@ channelpadding_update_padding_for_channel(channel_t *chan,
/* Max must not be lower than ito_low_ms */
chan->padding_timeout_high_ms = MAX(chan->padding_timeout_low_ms,
- pad_vars->ito_high_ms);
+ pad_vars->ito_high_ms);
log_fn(LOG_INFO,LD_OR,
"Negotiated padding=%d, lo=%d, hi=%d on "U64_FORMAT,
@@ -271,7 +293,7 @@ channelpadding_send_disable_command(channel_t *chan)
channelpadding_negotiate_set_command(&disable, CHANNELPADDING_COMMAND_STOP);
if (channelpadding_negotiate_encode(cell.payload, CELL_PAYLOAD_SIZE,
- &disable) < 0)
+ &disable) < 0)
return -1;
if (chan->write_cell(chan, &cell) == 1)
@@ -305,7 +327,7 @@ channelpadding_send_enable_command(channel_t *chan, uint16_t low_timeout,
channelpadding_negotiate_set_ito_high_ms(&enable, high_timeout);
if (channelpadding_negotiate_encode(cell.payload, CELL_PAYLOAD_SIZE,
- &enable) < 0)
+ &enable) < 0)
return -1;
if (chan->write_cell(chan, &cell) == 1)
@@ -388,9 +410,9 @@ channelpadding_send_padding_callback(tor_timer_t *timer, void *args,
if (chan && CHANNEL_CAN_HANDLE_CELLS(chan)) {
/* Hrmm.. It might be nice to have an equivalent to assert_connection_ok
* for channels. Then we could get rid of the channeltls dependency */
- tor_assert(BASE_CHAN_TO_TLS(chan)->conn->base_.magic ==
+ tor_assert(TO_CONN(BASE_CHAN_TO_TLS(chan)->conn)->magic ==
OR_CONNECTION_MAGIC);
- assert_connection_ok(&BASE_CHAN_TO_TLS(chan)->conn->base_, approx_time());
+ assert_connection_ok(TO_CONN(BASE_CHAN_TO_TLS(chan)->conn), approx_time());
channelpadding_send_padding_cell_for_callback(chan);
} else {
@@ -421,8 +443,8 @@ channelpadding_schedule_padding(channel_t *chan, int in_ms)
return CHANNELPADDING_PADDING_SENT;
}
- timeout.tv_sec = in_ms/1000;
- timeout.tv_usec = (in_ms%1000)*1000;
+ timeout.tv_sec = in_ms/TOR_MSEC_PER_SEC;
+ timeout.tv_usec = (in_ms%TOR_USEC_PER_MSEC)*TOR_USEC_PER_MSEC;
if (!chan->timer_handle) {
chan->timer_handle = channel_handle_new(chan);
@@ -467,7 +489,7 @@ channelpadding_compute_time_until_pad_for_netflow(channel_t *chan)
if (!chan->next_padding_time_ms) {
/* If the below line or crypto_rand_int() shows up on a profile,
- * we can avoid getting a timeout until we're at least nt_ito_lo
+ * we can avoid getting a timeout until we're at least nf_ito_lo
* from a timeout window. That will prevent us from setting timers
* on connections that were active up to 1.5 seconds ago.
* Idle connections should only call this once every 5.5s on average
@@ -479,7 +501,7 @@ channelpadding_compute_time_until_pad_for_netflow(channel_t *chan)
return CHANNELPADDING_TIME_DISABLED;
chan->next_padding_time_ms = padding_timeout
- + chan->timestamp_xfer_ms;
+ + chan->timestamp_xfer_ms;
}
/* If the next padding time is beyond the maximum possible consensus value,
@@ -499,11 +521,13 @@ channelpadding_compute_time_until_pad_for_netflow(channel_t *chan)
}
/* If the timeout will expire before the next time we're called (1000ms
- from now, plus some slack), then calcualte the number of milliseconds
+ from now, plus some slack), then calculate the number of milliseconds
from now which we should send padding, so we can schedule a callback
then.
*/
- if (long_now + 1100 >= chan->next_padding_time_ms) {
+ if (long_now +
+ (TOR_HOUSEKEEPING_CALLBACK_MSEC + TOR_HOUSEKEEPING_CALLBACK_SLACK_MSEC)
+ >= chan->next_padding_time_ms) {
int64_t ms_until_pad_for_netflow = chan->next_padding_time_ms -
long_now;
if (ms_until_pad_for_netflow < 0) {
@@ -540,9 +564,7 @@ channelpadding_get_channel_idle_timeout(const channel_t *chan,
unsigned int timeout;
/* Non-canonical and client channels only last for 3-4.5 min when idle */
- if (!is_canonical || !public_server_mode(options) ||
- chan->is_client ||
- !connection_or_digest_is_known_relay(chan->identity_digest)) {
+ if (!is_canonical || CHANNEL_IS_CLIENT(chan, options)) {
#define CONNTIMEOUT_CLIENTS_BASE 180 // 3 to 4.5 min
timeout = CONNTIMEOUT_CLIENTS_BASE
+ crypto_rand_int(CONNTIMEOUT_CLIENTS_BASE/2);
@@ -562,7 +584,7 @@ channelpadding_get_channel_idle_timeout(const channel_t *chan,
* set.
*/
if (options->ReducedConnectionPadding
- && !options->CircuitsAvailableTimeout) {
+ && !options->CircuitsAvailableTimeout) {
timeout /= 2;
}
@@ -688,8 +710,7 @@ channelpadding_decide_to_pad_channel(channel_t *chan)
if (!chan->has_queued_writes(chan)) {
int is_client_channel = 0;
- if (!public_server_mode(options) || chan->is_client ||
- !connection_or_digest_is_known_relay(chan->identity_digest)) {
+ if (CHANNEL_IS_CLIENT(chan, options)) {
is_client_channel = 1;
}
diff --git a/src/or/channelpadding.h b/src/or/channelpadding.h
index e08f7a2..2708ee9 100644
--- a/src/or/channelpadding.h
+++ b/src/or/channelpadding.h
@@ -24,7 +24,8 @@ typedef enum {
channelpadding_decision_t channelpadding_decide_to_pad_channel(channel_t
*chan);
int channelpadding_update_padding_for_channel(channel_t *,
- const channelpadding_negotiate_t *);
+ const channelpadding_negotiate_t
+ *chan);
void channelpadding_disable_padding_on_channel(channel_t *chan);
void channelpadding_reduce_padding_on_channel(channel_t *chan);
diff --git a/src/or/channeltls.c b/src/or/channeltls.c
index 4f2663a..7f5667b 100644
--- a/src/or/channeltls.c
+++ b/src/or/channeltls.c
@@ -1114,13 +1114,13 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn)
entry_guards_note_internet_connectivity(get_guard_selection_info());
rep_hist_padding_count_read(PADDING_TYPE_TOTAL);
- if (chan->base_.currently_padding)
+ if (TLS_CHAN_TO_BASE(chan)->currently_padding)
rep_hist_padding_count_read(PADDING_TYPE_ENABLED_TOTAL);
switch (cell->command) {
case CELL_PADDING:
rep_hist_padding_count_read(PADDING_TYPE_CELL);
- if (chan->base_.currently_padding)
+ if (TLS_CHAN_TO_BASE(chan)->currently_padding)
rep_hist_padding_count_read(PADDING_TYPE_ENABLED_CELL);
++stats_n_padding_cells_processed;
/* do nothing */
@@ -1591,11 +1591,11 @@ channel_tls_process_versions_cell(var_cell_t *cell, channel_tls_t *chan)
/* We set this after sending the verions cell. */
/*XXXXX symbolic const.*/
- chan->base_.wide_circ_ids =
+ TLS_CHAN_TO_BASE(chan)->wide_circ_ids =
chan->conn->link_proto >= MIN_LINK_PROTO_FOR_WIDE_CIRC_IDS;
- chan->conn->wide_circ_ids = chan->base_.wide_circ_ids;
+ chan->conn->wide_circ_ids = TLS_CHAN_TO_BASE(chan)->wide_circ_ids;
- chan->base_.padding_enabled =
+ TLS_CHAN_TO_BASE(chan)->padding_enabled =
chan->conn->link_proto >= MIN_LINK_PROTO_FOR_CHANNEL_PADDING;
if (send_certs) {
@@ -1758,7 +1758,7 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan)
if (!get_options()->BridgeRelay && me &&
get_uint32(my_addr_ptr) == htonl(me->addr)) {
- chan->base_.is_canonical_to_peer = 1;
+ TLS_CHAN_TO_BASE(chan)->is_canonical_to_peer = 1;
}
} else if (my_addr_type == RESOLVED_TYPE_IPV6 && my_addr_len == 16) {
@@ -1767,7 +1767,7 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan)
if (!get_options()->BridgeRelay && me &&
!tor_addr_is_null(&me->ipv6_addr) &&
tor_addr_eq(&my_apparent_addr, &me->ipv6_addr)) {
- chan->base_.is_canonical_to_peer = 1;
+ TLS_CHAN_TO_BASE(chan)->is_canonical_to_peer = 1;
}
}
@@ -1800,12 +1800,14 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan)
--n_other_addrs;
}
- if (me && !chan->base_.is_canonical_to_peer && chan->conn->is_canonical) {
+ if (me && !TLS_CHAN_TO_BASE(chan)->is_canonical_to_peer &&
+ channel_is_canonical(TLS_CHAN_TO_BASE(chan))) {
log_info(LD_OR,
"We made a connection to a relay at %s (fp=%s) but we think "
"they will not consider this connection canonical. They "
"think we are at %s, but we think its %s.",
- safe_str(chan->base_.get_remote_descr(&chan->base_, 0)),
+ safe_str(TLS_CHAN_TO_BASE(chan)->get_remote_descr(TLS_CHAN_TO_BASE(chan),
+ 0)),
safe_str(hex_str(chan->conn->identity_digest, DIGEST_LEN)),
safe_str(tor_addr_is_null(&my_apparent_addr) ?
"<none>" : fmt_and_decorate_addr(&my_apparent_addr)),
diff --git a/src/or/command.c b/src/or/command.c
index cebb5bf..5f3b667 100644
--- a/src/or/command.c
+++ b/src/or/command.c
@@ -326,17 +326,18 @@ command_process_create_cell(cell_t *cell, channel_t *chan)
return;
}
+ if (connection_or_digest_is_known_relay(chan->identity_digest)) {
+ rep_hist_note_circuit_handshake_requested(create_cell->handshake_type);
+ // Needed for chutney: Sometimes relays aren't in the consensus yet, and
+ // get marked as clients. This resets their channels once they appear.
+ // Probably useful for normal operation wrt relay flapping, too.
+ chan->is_client = 0;
+ } else {
+ channel_mark_client(chan);
+ }
+
if (create_cell->handshake_type != ONION_HANDSHAKE_TYPE_FAST) {
/* hand it off to the cpuworkers, and then return. */
- if (connection_or_digest_is_known_relay(chan->identity_digest)) {
- rep_hist_note_circuit_handshake_requested(create_cell->handshake_type);
- // Needed for chutney: Sometimes relays aren't in the consensus yet, and
- // get marked as clients. This resets their channels once they appear.
- // Probably useful for normal operation wrt relay flapping, too.
- chan->is_client = 0;
- } else {
- channel_mark_client(chan);
- }
if (assign_onionskin_to_cpuworker(circ, create_cell) < 0) {
log_debug(LD_GENERAL,"Failed to hand off onionskin. Closing.");
@@ -352,16 +353,6 @@ command_process_create_cell(cell_t *cell, channel_t *chan)
int len;
created_cell_t created_cell;
- /* If this is a create_fast, this might be a client. Let's check. */
- if (connection_or_digest_is_known_relay(chan->identity_digest)) {
- // Needed for chutney: Sometimes relays aren't in the consensus yet, and
- // get marked as clients. This resets their channels once they appear.
- // Probably useful for normal operation wrt relay flapping, too.
- chan->is_client = 0;
- } else {
- channel_mark_client(chan);
- }
-
memset(&created_cell, 0, sizeof(created_cell));
len = onion_skin_server_handshake(ONION_HANDSHAKE_TYPE_FAST,
create_cell->onionskin,
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index c77e364..0ee3fce 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -1976,7 +1976,7 @@ connection_or_write_cell_to_buf(const cell_t *cell, or_connection_t *conn)
if (conn->chan) {
channel_timestamp_active(TLS_CHAN_TO_BASE(conn->chan));
- if (conn->chan->base_.currently_padding) {
+ if (TLS_CHAN_TO_BASE(conn->chan)->currently_padding) {
rep_hist_padding_count_write(PADDING_TYPE_ENABLED_TOTAL);
if (cell->command == CELL_PADDING)
rep_hist_padding_count_write(PADDING_TYPE_ENABLED_CELL);
diff --git a/src/or/relay.c b/src/or/relay.c
index 18b68ff..886ddad 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -260,8 +260,8 @@ circuit_update_channel_usage(circuit_t *circ, cell_t *cell)
if (BUG(!or_circ->p_chan))
return;
- if (!or_circ->p_chan->is_client ||
- (or_circ->p_chan->is_client && circ->n_chan)) {
+ if (!channel_is_client(or_circ->p_chan) ||
+ (channel_is_client(or_circ->p_chan) && circ->n_chan)) {
if (cell->command == CELL_RELAY_EARLY) {
if (or_circ->p_chan->channel_usage < CHANNEL_USED_FOR_FULL_CIRCS) {
or_circ->p_chan->channel_usage = CHANNEL_USED_FOR_FULL_CIRCS;
More information about the tor-commits
mailing list