[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor/master] Fix issues from dgoulet's code review.



commit 02a5835c27780e45f705fc1c044b9c471b929dbe
Author: Mike Perry <mikeperry-git@xxxxxxxxxxxxxx>
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;



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits