Merge branch 'dgoulet_ticket23709_033_01_squashed'

changes/ticket23709

@@ -0,0 +1,11 @@
+  o Major feature (channel):
+    - Remove the incoming and outgoing channel queues. The reason to do so was
+      due to the fact that they were always empty meaning never used but still
+      looked at in our fast path. Bottom line, it was an unused code path.
+    - We've simplify a lot the channel subsystem by removing those queues but
+      also by removing a lot of unused code or dead code around it. Overall
+      this is a cleanup removing more than 1500 lines of code overall and
+      adding very little except for unit test.
+    - The majority ot the channel unit tests have been rewritten and the code
+      coverage has now been raised to 83.6% for channel.c.
+      Closes ticket 23709.

src/or/channel.h

@@ -19,10 +19,6 @@ typedef void (*channel_listener_fn_ptr)(channel_listener_t *, channel_t *);
 typedef void (*channel_cell_handler_fn_ptr)(channel_t *, cell_t *);
 typedef void (*channel_var_cell_handler_fn_ptr)(channel_t *, var_cell_t *);
 
-struct cell_queue_entry_s;
-TOR_SIMPLEQ_HEAD(chan_cell_queue, cell_queue_entry_s);
-typedef struct chan_cell_queue chan_cell_queue_t;
-
 /**
  * This enum is used by channelpadding to decide when to pad channels.
  * Don't add values to it without updating the checks in
@@ -259,21 +255,12 @@ struct channel_s {
    */
   ed25519_public_key_t ed25519_identity;
 
-  /** Nickname of the OR on the other side, or NULL if none. */
-  char *nickname;
-
   /**
    * Linked list of channels with the same RSA identity digest, for use with
    * the digest->channel map
    */
   TOR_LIST_ENTRY(channel_s) next_with_same_id;
 
-  /** List of incoming cells to handle */
-  chan_cell_queue_t incoming_queue;
-
-  /** List of queued outgoing cells */
-  chan_cell_queue_t outgoing_queue;
-
   /** Circuit mux for circuits sending on this channel */
   circuitmux_t *cmux;
 
@@ -320,7 +307,6 @@ struct channel_s {
 
   /** Channel timestamps for cell channels */
   time_t timestamp_client; /* Client used this, according to relay.c */
-  time_t timestamp_drained; /* Output queue empty */
   time_t timestamp_recv; /* Cell received from lower layer */
   time_t timestamp_xmit; /* Cell sent to lower layer */
 
@@ -337,14 +323,6 @@ struct channel_s {
   /** Channel counters for cell channels */
   uint64_t n_cells_recved, n_bytes_recved;
   uint64_t n_cells_xmitted, n_bytes_xmitted;
-
-  /** Our current contribution to the scheduler's total xmit queue */
-  uint64_t bytes_queued_for_xmit;
-
-  /** Number of bytes in this channel's cell queue; does not include
-   * lower-layer queueing.
-   */
-  uint64_t bytes_in_queue;
 };
 
 struct channel_listener_s {
@@ -412,18 +390,13 @@ channel_listener_state_to_string(channel_listener_state_t state);
 /* Abstract channel operations */
 
 void channel_mark_for_close(channel_t *chan);
-void channel_write_cell(channel_t *chan, cell_t *cell);
-void channel_write_packed_cell(channel_t *chan, packed_cell_t *cell);
-void channel_write_var_cell(channel_t *chan, var_cell_t *cell);
+int channel_write_packed_cell(channel_t *chan, packed_cell_t *cell);
 
 void channel_listener_mark_for_close(channel_listener_t *chan_l);
 
 /* Channel callback registrations */
 
 /* Listener callback */
-channel_listener_fn_ptr
-channel_listener_get_listener_fn(channel_listener_t *chan);
-
 void channel_listener_set_listener_fn(channel_listener_t *chan,
                                       channel_listener_fn_ptr listener);
 
@@ -457,36 +430,9 @@ void channel_set_cmux_policy_everywhere(circuitmux_policy_t *pol);
 #ifdef TOR_CHANNEL_INTERNAL_
 
 #ifdef CHANNEL_PRIVATE_
-/* Cell queue structure (here rather than channel.c for test suite use) */
 
-typedef struct cell_queue_entry_s cell_queue_entry_t;
-struct cell_queue_entry_s {
-  TOR_SIMPLEQ_ENTRY(cell_queue_entry_s) next;
-  enum {
-    CELL_QUEUE_FIXED,
-    CELL_QUEUE_VAR,
-    CELL_QUEUE_PACKED
-  } type;
-  union {
-    struct {
-      cell_t *cell;
-    } fixed;
-    struct {
-      var_cell_t *var_cell;
-    } var;
-    struct {
-      packed_cell_t *packed_cell;
-    } packed;
-  } u;
-};
+STATIC void channel_add_to_digest_map(channel_t *chan);
 
-/* Cell queue functions for benefit of test suite */
-STATIC int chan_cell_queue_len(const chan_cell_queue_t *queue);
-
-STATIC void cell_queue_entry_free(cell_queue_entry_t *q, int handed_off);
-
-void channel_write_cell_generic_(channel_t *chan, const char *cell_type,
-                                 void *cell, cell_queue_entry_t *q);
 #endif /* defined(CHANNEL_PRIVATE_) */
 
 /* Channel operations for subclasses and internal use only */
@@ -511,10 +457,6 @@ void channel_close_from_lower_layer(channel_t *chan);
 void channel_close_for_error(channel_t *chan);
 void channel_closed(channel_t *chan);
 
-void channel_listener_close_from_lower_layer(channel_listener_t *chan_l);
-void channel_listener_close_for_error(channel_listener_t *chan_l);
-void channel_listener_closed(channel_listener_t *chan_l);
-
 /* Free a channel */
 void channel_free(channel_t *chan);
 void channel_listener_free(channel_listener_t *chan_l);
@@ -532,9 +474,6 @@ void channel_mark_remote(channel_t *chan);
 void channel_set_identity_digest(channel_t *chan,
                                  const char *identity_digest,
                                  const ed25519_public_key_t *ed_identity);
-void channel_set_remote_end(channel_t *chan,
-                            const char *identity_digest,
-                            const char *nickname);
 
 void channel_listener_change_state(channel_listener_t *chan_l,
                                    channel_listener_state_t to_state);
@@ -542,7 +481,6 @@ void channel_listener_change_state(channel_listener_t *chan_l,
 /* Timestamp updates */
 void channel_timestamp_created(channel_t *chan);
 void channel_timestamp_active(channel_t *chan);
-void channel_timestamp_drained(channel_t *chan);
 void channel_timestamp_recv(channel_t *chan);
 void channel_timestamp_xmit(channel_t *chan);
 
@@ -556,12 +494,7 @@ void channel_listener_queue_incoming(channel_listener_t *listener,
                                      channel_t *incoming);
 
 /* Incoming cell handling */
-void channel_process_cells(channel_t *chan);
-void channel_queue_cell(channel_t *chan, cell_t *cell);
-void channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell);
-
-/* Outgoing cell handling */
-void channel_flush_cells(channel_t *chan);
+void channel_process_cell(channel_t *chan, cell_t *cell);
 
 /* Request from lower layer for more cells if available */
 MOCK_DECL(ssize_t, channel_flush_some_cells,
@@ -576,10 +509,6 @@ void channel_notify_flushed(channel_t *chan);
 /* Handle stuff we need to do on open like notifying circuits */
 void channel_do_open_actions(channel_t *chan);
 
-#ifdef TOR_UNIT_TESTS
-extern uint64_t estimated_total_queue_size;
-#endif
-
 #endif /* defined(TOR_CHANNEL_INTERNAL_) */
 
 /* Helper functions to perform operations on channels */
@@ -680,7 +609,6 @@ MOCK_DECL(void,channel_set_circid_type,(channel_t *chan,
                                         crypto_pk_t *identity_rcvd,
                                         int consider_identity));
 void channel_timestamp_client(channel_t *chan);
-void channel_update_xmit_queue_size(channel_t *chan);
 
 const char * channel_listener_describe_transport(channel_listener_t *chan_l);
 void channel_listener_dump_statistics(channel_listener_t *chan_l,
@@ -692,27 +620,14 @@ void channel_check_for_duplicates(void);
 void channel_update_bad_for_new_circs(const char *digest, int force);
 
 /* Flow control queries */
-uint64_t channel_get_global_queue_estimate(void);
 int channel_num_cells_writeable(channel_t *chan);
 
 /* Timestamp queries */
 time_t channel_when_created(channel_t *chan);
-time_t channel_when_last_active(channel_t *chan);
 time_t channel_when_last_client(channel_t *chan);
-time_t channel_when_last_drained(channel_t *chan);
-time_t channel_when_last_recv(channel_t *chan);
 time_t channel_when_last_xmit(channel_t *chan);
 
-time_t channel_listener_when_created(channel_listener_t *chan_l);
-time_t channel_listener_when_last_active(channel_listener_t *chan_l);
-time_t channel_listener_when_last_accepted(channel_listener_t *chan_l);
-
 /* Counter queries */
-uint64_t channel_count_recved(channel_t *chan);
-uint64_t channel_count_xmitted(channel_t *chan);
-
-uint64_t channel_listener_count_accepted(channel_listener_t *chan_l);
-
 int packed_cell_is_destroy(channel_t *chan,
                            const packed_cell_t *packed_cell,
                            circid_t *circid_out);

src/or/channeltls.c

@@ -832,6 +832,9 @@ channel_tls_write_cell_method(channel_t *chan, cell_t *cell)
  *
  * This implements the write_packed_cell method for channel_tls_t; given a
  * channel_tls_t and a packed_cell_t, transmit the packed_cell_t.
+ *
+ * Return 0 on success or negative value on error. The caller must free the
+ * packed cell.
  */
 
 static int
@@ -841,7 +844,6 @@ channel_tls_write_packed_cell_method(channel_t *chan,
   tor_assert(chan);
   channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan);
   size_t cell_network_size = get_cell_network_size(chan->wide_circ_ids);
-  int written = 0;
 
   tor_assert(tlschan);
   tor_assert(packed_cell);
@@ -849,18 +851,15 @@ channel_tls_write_packed_cell_method(channel_t *chan,
   if (tlschan->conn) {
     connection_buf_add(packed_cell->body, cell_network_size,
                             TO_CONN(tlschan->conn));
-
-    /* This is where the cell is finished; used to be done from relay.c */
-    packed_cell_free(packed_cell);
-    ++written;
   } else {
     log_info(LD_CHANNEL,
              "something called write_packed_cell on a tlschan "
              "(%p with ID " U64_FORMAT " but no conn",
              chan, U64_PRINTF_ARG(chan->global_identifier));
+    return -1;
   }
 
-  return written;
+  return 0;
 }
 
 /**
@@ -1149,7 +1148,7 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn)
        * These are all transport independent and we pass them up through the
        * channel_t mechanism.  They are ultimately handled in command.c.
        */
-      channel_queue_cell(TLS_CHAN_TO_BASE(chan), cell);
+      channel_process_cell(TLS_CHAN_TO_BASE(chan), cell);
       break;
     default:
       log_fn(LOG_INFO, LD_PROTOCOL,

src/or/circuitbuild.c

@@ -631,8 +631,7 @@ circuit_n_chan_done(channel_t *chan, int status, int close_origin_circuits)
 
   tor_assert(chan);
 
-  log_debug(LD_CIRC,"chan to %s/%s, status=%d",
-            chan->nickname ? chan->nickname : "NULL",
+  log_debug(LD_CIRC,"chan to %s, status=%d",
             channel_get_canonical_remote_descr(chan), status);
 
   pending_circs = smartlist_new();

src/or/circuitlist.c

@@ -505,8 +505,7 @@ circuit_count_pending_on_channel(channel_t *chan)
   circuit_get_all_pending_on_channel(sl, chan);
   cnt = smartlist_len(sl);
   smartlist_free(sl);
-  log_debug(LD_CIRC,"or_conn to %s at %s, %d pending circs",
-            chan->nickname ? chan->nickname : "NULL",
+  log_debug(LD_CIRC,"or_conn to %s, %d pending circs",
             channel_get_canonical_remote_descr(chan),
             cnt);
   return cnt;

src/or/connection_or.c

@@ -592,9 +592,6 @@ connection_or_flushed_some(or_connection_t *conn)
 {
   size_t datalen;
 
-  /* The channel will want to update its estimated queue size */
-  channel_update_xmit_queue_size(TLS_CHAN_TO_BASE(conn->chan));
-
   /* If we're under the low water mark, add cells until we're just over the
    * high water mark. */
   datalen = connection_get_outbuf_len(TO_CONN(conn));

src/or/relay.c

@@ -2746,7 +2746,13 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
       /* this code is duplicated from some of the logic below. Ugly! XXXX */
       tor_assert(destroy_queue->n > 0);
       cell = cell_queue_pop(destroy_queue);
-      channel_write_packed_cell(chan, cell);
+      /* Send the DESTROY cell. It is very unlikely that this fails but just
+       * in case, get rid of the channel. */
+      if (channel_write_packed_cell(chan, cell) < 0) {
+        /* The cell has been freed. */
+        channel_mark_for_close(chan);
+        continue;
+      }
       /* Update the cmux destroy counter */
       circuitmux_notify_xmit_destroy(cmux);
       cell = NULL;
@@ -2823,8 +2829,13 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
                                 DIRREQ_TUNNELED,
                                 DIRREQ_CIRC_QUEUE_FLUSHED);
 
-    /* Now send the cell */
-    channel_write_packed_cell(chan, cell);
+    /* Now send the cell. It is very unlikely that this fails but just in
+     * case, get rid of the channel. */
+    if (channel_write_packed_cell(chan, cell) < 0) {
+      /* The cell has been freed at this point. */
+      channel_mark_for_close(chan);
+      continue;
+    }
     cell = NULL;
 
     /*
@@ -2859,22 +2870,13 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
   return n_flushed;
 }
 
-#if 0
-/** Indicate the current preferred cap for middle circuits; zero disables
- * the cap.  Right now it's just a constant, ORCIRC_MAX_MIDDLE_CELLS, but
- * the logic in append_cell_to_circuit_queue() is written to be correct
- * if we want to base it on a consensus param or something that might change
- * in the future.
- */
-static int
-get_max_middle_cells(void)
-{
-  return ORCIRC_MAX_MIDDLE_CELLS;
-}
-#endif /* 0 */
-
 /** Add <b>cell</b> to the queue of <b>circ</b> writing to <b>chan</b>
- * transmitting in <b>direction</b>. */
+ * transmitting in <b>direction</b>.
+ *
+ * The given <b>cell</b> is copied over the circuit queue so the caller must
+ * cleanup the memory.
+ *
+ * This function is part of the fast path. */
 void
 append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
                              cell_t *cell, cell_direction_t direction,
@@ -2883,10 +2885,6 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
   or_circuit_t *orcirc = NULL;
   cell_queue_t *queue;
   int streams_blocked;
-#if 0
-  uint32_t tgt_max_middle_cells, p_len, n_len, tmp, hard_max_middle_cells;
-#endif
-
   int exitward;
   if (circ->marked_for_close)
     return;
@@ -2901,93 +2899,14 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
     streams_blocked = circ->streams_blocked_on_p_chan;
   }
 
-  /*
-   * Disabling this for now because of a possible guard discovery attack
-   */
-#if 0
-  /* Are we a middle circuit about to exceed ORCIRC_MAX_MIDDLE_CELLS? */
-  if ((circ->n_chan != NULL) && CIRCUIT_IS_ORCIRC(circ)) {
-    orcirc = TO_OR_CIRCUIT(circ);
-    if (orcirc->p_chan) {
-      /* We are a middle circuit if we have both n_chan and p_chan */
-      /* We'll need to know the current preferred maximum */
-      tgt_max_middle_cells = get_max_middle_cells();
-      if (tgt_max_middle_cells > 0) {
-        /* Do we need to initialize middle_max_cells? */
-        if (orcirc->max_middle_cells == 0) {
-          orcirc->max_middle_cells = tgt_max_middle_cells;
-        } else {
-          if (tgt_max_middle_cells > orcirc->max_middle_cells) {
-            /* If we want to increase the cap, we can do so right away */
-            orcirc->max_middle_cells = tgt_max_middle_cells;
-          } else if (tgt_max_middle_cells < orcirc->max_middle_cells) {
-            /*
-             * If we're shrinking the cap, we can't shrink past either queue;
-             * compare tgt_max_middle_cells rather than tgt_max_middle_cells *
-             * ORCIRC_MAX_MIDDLE_KILL_THRESH so the queues don't shrink enough
-             * to generate spurious warnings, either.
-             */
-            n_len = circ->n_chan_cells.n;
-            p_len = orcirc->p_chan_cells.n;
-            tmp = tgt_max_middle_cells;
-            if (tmp < n_len) tmp = n_len;
-            if (tmp < p_len) tmp = p_len;
-            orcirc->max_middle_cells = tmp;
-          }
-          /* else no change */
-        }
-      } else {
-        /* tgt_max_middle_cells == 0 indicates we should disable the cap */
-        orcirc->max_middle_cells = 0;
-      }
-
-      /* Now we know orcirc->max_middle_cells is set correctly */
-      if (orcirc->max_middle_cells > 0) {
-        hard_max_middle_cells =
-          (uint32_t)(((double)orcirc->max_middle_cells) *
-                     ORCIRC_MAX_MIDDLE_KILL_THRESH);
-
-        if ((unsigned)queue->n + 1 >= hard_max_middle_cells) {
-          /* Queueing this cell would put queue over the kill theshold */
-          log_warn(LD_CIRC,
-                   "Got a cell exceeding the hard cap of %u in the "
-                   "%s direction on middle circ ID %u on chan ID "
-                   U64_FORMAT "; killing the circuit.",
-                   hard_max_middle_cells,
-                   (direction == CELL_DIRECTION_OUT) ? "n" : "p",
-                   (direction == CELL_DIRECTION_OUT) ?
-                     circ->n_circ_id : orcirc->p_circ_id,
-                   U64_PRINTF_ARG(
-                     (direction == CELL_DIRECTION_OUT) ?
-                        circ->n_chan->global_identifier :
-                        orcirc->p_chan->global_identifier));
-          circuit_mark_for_close(circ, END_CIRC_REASON_RESOURCELIMIT);
-          return;
-        } else if ((unsigned)queue->n + 1 == orcirc->max_middle_cells) {
-          /* Only use ==, not >= for this test so we don't spam the log */
-          log_warn(LD_CIRC,
-                   "While trying to queue a cell, reached the soft cap of %u "
-                   "in the %s direction on middle circ ID %u "
-                   "on chan ID " U64_FORMAT ".",
-                   orcirc->max_middle_cells,
-                   (direction == CELL_DIRECTION_OUT) ? "n" : "p",
-                   (direction == CELL_DIRECTION_OUT) ?
-                     circ->n_circ_id : orcirc->p_circ_id,
-                   U64_PRINTF_ARG(
-                     (direction == CELL_DIRECTION_OUT) ?
-                        circ->n_chan->global_identifier :
-                        orcirc->p_chan->global_identifier));
-        }
-      }
-    }
-  }
-#endif /* 0 */
-
+  /* Very important that we copy to the circuit queue because all calls to
+   * this function use the stack for the cell memory. */
   cell_queue_append_packed_copy(circ, queue, exitward, cell,
                                 chan->wide_circ_ids, 1);
 
+  /* Check and run the OOM if needed. */
   if (PREDICT_UNLIKELY(cell_queues_check_size())) {
-    /* We ran the OOM handler */
+    /* We ran the OOM handler which might have closed this circuit. */
     if (circ->marked_for_close)
       return;
   }

src/test/fakechans.h

@@ -20,7 +20,6 @@ void scheduler_release_channel_mock(channel_t *ch);
 
 /* Query some counters used by the exposed mocks */
 int get_mock_scheduler_has_waiting_cells_count(void);
-int get_mock_scheduler_release_channel_count(void);
 
 #endif /* !defined(TOR_FAKECHANS_H) */
 

src/test/test_relay.c

@@ -67,10 +67,6 @@ test_relay_append_cell_to_circuit_queue(void *arg)
   pchan = new_fake_channel();
   tt_assert(pchan);
 
-  /* We'll need chans with working cmuxes */
-  nchan->cmux = circuitmux_alloc();
-  pchan->cmux = circuitmux_alloc();
-
   /* Make a fake orcirc */
   orcirc = new_fake_orcirc(nchan, pchan);
   tt_assert(orcirc);

src/test/test_scheduler.c

@@ -299,10 +299,6 @@ channel_more_to_flush_mock(channel_t *chan)
 
   flush_mock_channel_t *found_mock_ch = NULL;
 
-  /* Check if we have any queued */
-  if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue))
-      return 1;
-
   SMARTLIST_FOREACH_BEGIN(chans_for_flush_mock,
                           flush_mock_channel_t *,
                           flush_mock_ch) {
@@ -444,8 +440,6 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type)
 
   /* Start it off in OPENING */
   ch1->state = CHANNEL_STATE_OPENING;
-  /* We'll need a cmux */
-  ch1->cmux = circuitmux_alloc();
   /* Try to register it */
   channel_register(ch1);
   tt_assert(ch1->registered);
@@ -457,7 +451,6 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type)
   ch2 = new_fake_channel();
   tt_assert(ch2);
   ch2->state = CHANNEL_STATE_OPENING;
-  ch2->cmux = circuitmux_alloc();
   channel_register(ch2);
   tt_assert(ch2->registered);
 
@@ -656,8 +649,6 @@ test_scheduler_loop_vanilla(void *arg)
 
   /* Start it off in OPENING */
   ch1->state = CHANNEL_STATE_OPENING;
-  /* We'll need a cmux */
-  ch1->cmux = circuitmux_alloc();
   /* Try to register it */
   channel_register(ch1);
   tt_assert(ch1->registered);
@@ -672,7 +663,6 @@ test_scheduler_loop_vanilla(void *arg)
   ch2->magic = TLS_CHAN_MAGIC;
   tt_assert(ch2);
   ch2->state = CHANNEL_STATE_OPENING;
-  ch2->cmux = circuitmux_alloc();
   channel_register(ch2);
   tt_assert(ch2->registered);
   /*
@@ -824,7 +814,6 @@ test_scheduler_loop_kist(void *arg)
   tt_assert(ch1);
   ch1->magic = TLS_CHAN_MAGIC;
   ch1->state = CHANNEL_STATE_OPENING;
-  ch1->cmux = circuitmux_alloc();
   channel_register(ch1);
   tt_assert(ch1->registered);
   channel_change_state_open(ch1);
@@ -835,7 +824,6 @@ test_scheduler_loop_kist(void *arg)
   tt_assert(ch2);
   ch2->magic = TLS_CHAN_MAGIC;
   ch2->state = CHANNEL_STATE_OPENING;
-  ch2->cmux = circuitmux_alloc();
   channel_register(ch2);
   tt_assert(ch2->registered);
   channel_change_state_open(ch2);