Fix issues from dgoulet's code review.

https://gitlab.com/dgoulet/tor/merge_requests/24
This commit is contained in:
Mike Perry 2017-04-20 15:00:46 -04:00 committed by Nick Mathewson
parent 687a85950a
commit 02a5835c27
6 changed files with 73 additions and 58 deletions

View File

@ -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;
}

View File

@ -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);

View File

@ -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)),

View File

@ -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,

View File

@ -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);

View File

@ -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;