Merge branch 'bug24119_squashed'

changes/bug24119

@@ -0,0 +1,4 @@
+  o Code simplification and refactoring:
+    - Rewrite channel_rsa_id_group_set_badness to reduce temporary memory
+      allocations with large numbers of OR connections (e.g. relays). Closes
+      ticket 24119.

src/or/channel.c

@@ -4726,6 +4726,16 @@ channel_set_circid_type,(channel_t *chan,
   }
 }
 
+static int
+channel_sort_by_ed25519_identity(const void **a_, const void **b_)
+{
+  const channel_t *a = *a_,
+                  *b = *b_;
+  return fast_memcmp(&a->ed25519_identity.pubkey,
+                     &b->ed25519_identity.pubkey,
+                     sizeof(a->ed25519_identity.pubkey));
+}
+
 /** Helper for channel_update_bad_for_new_circs(): Perform the
  * channel_update_bad_for_new_circs operation on all channels in <b>lst</b>,
  * all of which MUST have the same RSA ID.  (They MAY have different
@@ -4734,44 +4744,52 @@ static void
 channel_rsa_id_group_set_badness(struct channel_list_s *lst, int force)
 {
   /*XXXX This function should really be about channels. 15056 */
-  channel_t *chan;
+  channel_t *chan = TOR_LIST_FIRST(lst);
+
+  if (!chan)
+    return;
+
+  /* if there is only one channel, don't bother looping */
+  if (PREDICT_LIKELY(!TOR_LIST_NEXT(chan, next_with_same_id))) {
+    connection_or_single_set_badness_(
+            time(NULL), BASE_CHAN_TO_TLS(chan)->conn, force);
+    return;
+  }
+
+  smartlist_t *channels = smartlist_new();
 
-  /* First, get a minimal list of the ed25519 identites */
-  smartlist_t *ed_identities = smartlist_new();
   TOR_LIST_FOREACH(chan, lst, next_with_same_id) {
-    uint8_t *id_copy =
-      tor_memdup(&chan->ed25519_identity.pubkey, DIGEST256_LEN);
-    smartlist_add(ed_identities, id_copy);
+    if (BASE_CHAN_TO_TLS(chan)->conn) {
+      smartlist_add(channels, chan);
+    }
   }
-  smartlist_sort_digests256(ed_identities);
-  smartlist_uniq_digests256(ed_identities);
 
-  /* Now, for each Ed identity, build a smartlist and find the best entry on
-   * it.  */
+  smartlist_sort(channels, channel_sort_by_ed25519_identity);
+
+  const ed25519_public_key_t *common_ed25519_identity = NULL;
+  /* it would be more efficient to do a slice, but this case is rare */
   smartlist_t *or_conns = smartlist_new();
-  SMARTLIST_FOREACH_BEGIN(ed_identities, const uint8_t *, ed_id) {
-    TOR_LIST_FOREACH(chan, lst, next_with_same_id) {
-      channel_tls_t *chantls = BASE_CHAN_TO_TLS(chan);
-      if (tor_memneq(ed_id, &chan->ed25519_identity.pubkey, DIGEST256_LEN))
-        continue;
-      or_connection_t *orconn = chantls->conn;
-      if (orconn) {
-        tor_assert(orconn->chan == chantls);
-        smartlist_add(or_conns, orconn);
-      }
+  SMARTLIST_FOREACH_BEGIN(channels, channel_t *, channel) {
+    if (!common_ed25519_identity)
+      common_ed25519_identity = &channel->ed25519_identity;
+
+    if (! ed25519_pubkey_eq(&channel->ed25519_identity,
+                            common_ed25519_identity)) {
+        connection_or_group_set_badness_(or_conns, force);
+        smartlist_clear(or_conns);
+        common_ed25519_identity = &channel->ed25519_identity;
     }
 
-    connection_or_group_set_badness_(or_conns, force);
-    smartlist_clear(or_conns);
-  } SMARTLIST_FOREACH_END(ed_id);
+    smartlist_add(or_conns, BASE_CHAN_TO_TLS(channel)->conn);
+  } SMARTLIST_FOREACH_END(channel);
+
+  connection_or_group_set_badness_(or_conns, force);
 
   /* XXXX 15056 we may want to do something special with connections that have
    * no set Ed25519 identity! */
 
   smartlist_free(or_conns);
-
-  SMARTLIST_FOREACH(ed_identities, uint8_t *, ed_id, tor_free(ed_id));
-  smartlist_free(ed_identities);
+  smartlist_free(channels);
 }
 
 /** Go through all the channels (or if <b>digest</b> is non-NULL, just

src/or/connection_or.c

@@ -965,6 +965,36 @@ connection_or_mark_bad_for_new_circs(or_connection_t *or_conn)
  * too old for new circuits? */
 #define TIME_BEFORE_OR_CONN_IS_TOO_OLD (60*60*24*7)
 
+/** Expire an or_connection if it is too old. Helper for
+ * connection_or_group_set_badness_ and fast path for
+ * channel_rsa_id_group_set_badness.
+ *
+ * Returns 1 if the connection was already expired, else 0.
+ */
+int
+connection_or_single_set_badness_(time_t now,
+                                  or_connection_t *or_conn,
+                                  int force)
+{
+  /* XXXX this function should also be about channels? */
+  if (or_conn->base_.marked_for_close ||
+      connection_or_is_bad_for_new_circs(or_conn))
+    return 1;
+
+  if (force ||
+      or_conn->base_.timestamp_created + TIME_BEFORE_OR_CONN_IS_TOO_OLD
+        < now) {
+    log_info(LD_OR,
+             "Marking OR conn to %s:%d as too old for new circuits "
+             "(fd "TOR_SOCKET_T_FORMAT", %d secs old).",
+             or_conn->base_.address, or_conn->base_.port, or_conn->base_.s,
+             (int)(now - or_conn->base_.timestamp_created));
+    connection_or_mark_bad_for_new_circs(or_conn);
+  }
+
+  return 0;
+}
+
 /** Given a list of all the or_connections with a given
  * identity, set elements of that list as is_bad_for_new_circs as
  * appropriate. Helper for connection_or_set_bad_connections().
@@ -995,19 +1025,8 @@ connection_or_group_set_badness_(smartlist_t *group, int force)
   /* Pass 1: expire everything that's old, and see what the status of
    * everything else is. */
   SMARTLIST_FOREACH_BEGIN(group, or_connection_t *, or_conn) {
-    if (or_conn->base_.marked_for_close ||
-        connection_or_is_bad_for_new_circs(or_conn))
+    if (connection_or_single_set_badness_(now, or_conn, force))
       continue;
-    if (force ||
-        or_conn->base_.timestamp_created + TIME_BEFORE_OR_CONN_IS_TOO_OLD
-          < now) {
-      log_info(LD_OR,
-               "Marking OR conn to %s:%d as too old for new circuits "
-               "(fd "TOR_SOCKET_T_FORMAT", %d secs old).",
-               or_conn->base_.address, or_conn->base_.port, or_conn->base_.s,
-               (int)(now - or_conn->base_.timestamp_created));
-      connection_or_mark_bad_for_new_circs(or_conn);
-    }
 
     if (connection_or_is_bad_for_new_circs(or_conn)) {
       ++n_old;

src/or/connection_or.h

@@ -112,6 +112,9 @@ void var_cell_free(var_cell_t *cell);
 #define MIN_LINK_PROTO_FOR_CHANNEL_PADDING 5
 #define MAX_LINK_PROTO MIN_LINK_PROTO_FOR_CHANNEL_PADDING
 
+int connection_or_single_set_badness_(time_t now,
+                                      or_connection_t *or_conn,
+                                      int force);
 void connection_or_group_set_badness_(smartlist_t *group, int force);
 
 #ifdef TOR_UNIT_TESTS