Merge remote-tracking branch 'public/my-family-list-fix-4498'

changes/ticket4998

@@ -0,0 +1,5 @@
+  o Minor features:
+    - The MyFamily line may now be repeated as many times as desired, for
+      relays that want to configure large families. Closes ticket 4998;
+      patch by Daniel Pinto.
+

doc/tor.1.txt

@@ -1732,14 +1732,15 @@ is non-zero):
     If we have more onionskins queued for processing than we can process in
     this amount of time, reject new ones. (Default: 1750 msec)
 
-[[MyFamily]] **MyFamily** __node__,__node__,__...__::
+[[MyFamily]] **MyFamily** __node__::
     Declare that this Tor server is controlled or administered by a group or
     organization identical or similar to that of the other servers, defined by
-    their identity fingerprints. When two servers both declare
-    that they are in the same \'family', Tor clients will not use them in the
-    same circuit. (Each server only needs to list the other servers in its
-    family; it doesn't need to list itself, but it won't hurt.) Do not list
-    any bridge relay as it would compromise its concealment. +
+    their identity fingerprints. This option can be repeated many times, for
+    multiple families. When two servers both declare that they are in the
+    same \'family', Tor clients will not use them in the same circuit. (Each
+    server only needs to list the other servers in its family; it doesn't need to
+    list itself, but it won't hurt.) Do not list any bridge relay as it would
+    compromise its concealment. +
  +
     When listing a node, it's better to list it by fingerprint than by
     nickname: fingerprints are more reliable.

src/or/config.c

@@ -398,7 +398,7 @@ static config_var_t option_vars_[] = {
   V(MaxOnionQueueDelay,          MSEC_INTERVAL, "1750 msec"),
   V(MaxUnparseableDescSizeToLog, MEMUNIT, "10 MB"),
   V(MinMeasuredBWsForAuthToIgnoreAdvertised, INT, "500"),
-  V(MyFamily,                    STRING,   NULL),
+  VAR("MyFamily",                LINELIST, MyFamily_lines,       NULL),
   V(NewCircuitPeriod,            INTERVAL, "30 seconds"),
   OBSOLETE("NamingAuthoritativeDirectory"),
   OBSOLETE("NATDListenAddress"),
@@ -685,7 +685,9 @@ static int options_transition_affects_workers(
       const or_options_t *old_options, const or_options_t *new_options);
 static int options_transition_affects_descriptor(
       const or_options_t *old_options, const or_options_t *new_options);
-static int check_nickname_list(char **lst, const char *name, char **msg);
+static int normalize_nickname_list(config_line_t **normalized_out,
+                                   const config_line_t *lst, const char *name,
+                                   char **msg);
 static char *get_bindaddr_from_transport_listen_line(const char *line,
                                                      const char *transport);
 static int parse_ports(or_options_t *options, int validate_only,
@@ -893,6 +895,7 @@ or_options_free(or_options_t *options)
   tor_free(options->BridgePassword_AuthDigest_);
   tor_free(options->command_arg);
   tor_free(options->master_key_fname);
+  config_free_lines(options->MyFamily);
   config_free(&options_format, options);
 }
 
@@ -3817,13 +3820,14 @@ options_validate(or_options_t *old_options, or_options_t *options,
              "have it group-readable.");
   }
 
-  if (options->MyFamily && options->BridgeRelay) {
+  if (options->MyFamily_lines && options->BridgeRelay) {
     log_warn(LD_CONFIG, "Listing a family for a bridge relay is not "
              "supported: it can reveal bridge fingerprints to censors. "
              "You should also make sure you aren't listing this bridge's "
              "fingerprint in any other MyFamily.");
   }
-  if (check_nickname_list(&options->MyFamily, "MyFamily", msg))
+  if (normalize_nickname_list(&options->MyFamily,
+                              options->MyFamily_lines, "MyFamily", msg))
     return -1;
   for (cl = options->NodeFamilies; cl; cl = cl->next) {
     routerset_t *rs = routerset_new();
@@ -4524,7 +4528,7 @@ options_transition_affects_descriptor(const or_options_t *old_options,
       get_effective_bwburst(old_options) !=
         get_effective_bwburst(new_options) ||
       !opt_streq(old_options->ContactInfo, new_options->ContactInfo) ||
-      !opt_streq(old_options->MyFamily, new_options->MyFamily) ||
+      !config_lines_eq(old_options->MyFamily, new_options->MyFamily) ||
       !opt_streq(old_options->AccountingStart, new_options->AccountingStart) ||
       old_options->AccountingMax != new_options->AccountingMax ||
       old_options->AccountingRule != new_options->AccountingRule ||
@@ -4620,27 +4624,36 @@ get_default_conf_file(int defaults_file)
 #endif
 }
 
-/** Verify whether lst is a string containing valid-looking comma-separated
- * nicknames, or NULL. Will normalise <b>lst</b> to prefix '$' to any nickname
- * or fingerprint that needs it. Return 0 on success.
+/** Verify whether lst is a list of strings containing valid-looking
+ * comma-separated nicknames, or NULL. Will normalise <b>lst</b> to prefix '$'
+ * to any nickname or fingerprint that needs it. Also splits comma-separated
+ * list elements into multiple elements. Return 0 on success.
  * Warn and return -1 on failure.
  */
 static int
-check_nickname_list(char **lst, const char *name, char **msg)
+normalize_nickname_list(config_line_t **normalized_out,
+                        const config_line_t *lst, const char *name,
+                        char **msg)
 {
-  int r = 0;
-  smartlist_t *sl;
-  int changes = 0;
-
-  if (!*lst)
+  if (!lst)
     return 0;
-  sl = smartlist_new();
 
-  smartlist_split_string(sl, *lst, ",",
-    SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK|SPLIT_STRIP_SPACE, 0);
+  config_line_t *new_nicknames = NULL;
+  config_line_t **new_nicknames_next = &new_nicknames;
 
-  SMARTLIST_FOREACH_BEGIN(sl, char *, s)
+  const config_line_t *cl;
+  for (cl = lst; cl; cl = cl->next) {
+    const char *line = cl->value;
+    if (!line)
+      continue;
+
+    int valid_line = 1;
+    smartlist_t *sl = smartlist_new();
+    smartlist_split_string(sl, line, ",",
+      SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK|SPLIT_STRIP_SPACE, 0);
+    SMARTLIST_FOREACH_BEGIN(sl, char *, s)
     {
+      char *normalized = NULL;
       if (!is_legal_nickname_or_hexdigest(s)) {
         // check if first char is dollar
         if (s[0] != '$') {
@@ -4649,36 +4662,45 @@ check_nickname_list(char **lst, const char *name, char **msg)
           tor_asprintf(&prepended, "$%s", s);
 
           if (is_legal_nickname_or_hexdigest(prepended)) {
-            // The nickname is valid when it's prepended, swap the current
-            // version with a prepended one
-            tor_free(s);
-            SMARTLIST_REPLACE_CURRENT(sl, s, prepended);
-            changes = 1;
-            continue;
+            // The nickname is valid when it's prepended, set it as the
+            // normalized version
+            normalized = prepended;
+          } else {
+            // Still not valid, free and fallback to error message
+            tor_free(prepended);
           }
-
-          // Still not valid, free and fallback to error message
-          tor_free(prepended);
         }
 
-        tor_asprintf(msg, "Invalid nickname '%s' in %s line", s, name);
-        r = -1;
-        break;
+        if (!normalized) {
+          tor_asprintf(msg, "Invalid nickname '%s' in %s line", s, name);
+          valid_line = 0;
+          break;
+        }
+      } else {
+        normalized = tor_strdup(s);
       }
-    }
-  SMARTLIST_FOREACH_END(s);
 
-  // Replace the caller's nickname list with a fixed one
-  if (changes && r == 0) {
-    char *newNicknames = smartlist_join_strings(sl, ", ", 0, NULL);
-    tor_free(*lst);
-    *lst = newNicknames;
+      config_line_t *next = tor_malloc_zero(sizeof(*next));
+      next->key = tor_strdup(cl->key);
+      next->value = normalized;
+      next->next = NULL;
+
+      *new_nicknames_next = next;
+      new_nicknames_next = &next->next;
+    } SMARTLIST_FOREACH_END(s);
+
+    SMARTLIST_FOREACH(sl, char *, s, tor_free(s));
+    smartlist_free(sl);
+
+    if (!valid_line) {
+      config_free_lines(new_nicknames);
+      return -1;
+    }
   }
 
-  SMARTLIST_FOREACH(sl, char *, s, tor_free(s));
-  smartlist_free(sl);
+  *normalized_out = new_nicknames;
 
-  return r;
+  return 0;
 }
 
 /** Learn config file name from command line arguments, or use the default.

src/or/or.h

@@ -3926,7 +3926,8 @@ typedef struct {
   /** If set, use these bridge authorities and not the default one. */
   config_line_t *AlternateBridgeAuthority;
 
-  char *MyFamily; /**< Declared family for this OR. */
+  config_line_t *MyFamily_lines; /**< Declared family for this OR. */
+  config_line_t *MyFamily; /**< Declared family for this OR, normalized */
   config_line_t *NodeFamilies; /**< List of config lines for
                                 * node families */
   smartlist_t *NodeFamilySets; /**< List of parsed NodeFamilies values. */

src/or/router.c

@@ -2281,14 +2281,12 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
   }
 
   if (options->MyFamily && ! options->BridgeRelay) {
-    smartlist_t *family;
     if (!warned_nonexistent_family)
       warned_nonexistent_family = smartlist_new();
-    family = smartlist_new();
     ri->declared_family = smartlist_new();
-    smartlist_split_string(family, options->MyFamily, ",",
-      SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK|SPLIT_STRIP_SPACE, 0);
-    SMARTLIST_FOREACH_BEGIN(family, char *, name) {
+    config_line_t *family;
+    for (family = options->MyFamily; family; family = family->next) {
+       char *name = family->value;
        const node_t *member;
        if (!strcasecmp(name, options->Nickname))
          goto skip; /* Don't list ourself, that's redundant */
@@ -2327,13 +2325,11 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
        }
     skip:
        tor_free(name);
-    } SMARTLIST_FOREACH_END(name);
+    }
 
     /* remove duplicates from the list */
     smartlist_sort_strings(ri->declared_family);
     smartlist_uniq_strings(ri->declared_family);
-
-    smartlist_free(family);
   }
 
   /* Now generate the extrainfo. */

src/test/test_config.c

@@ -854,9 +854,23 @@ static void
 test_config_fix_my_family(void *arg)
 {
   char *err = NULL;
-  const char *family = "$1111111111111111111111111111111111111111, "
-                       "1111111111111111111111111111111111111112, "
-                       "$1111111111111111111111111111111111111113";
+  config_line_t *family = tor_malloc_zero(sizeof(config_line_t));
+  family->key = tor_strdup("MyFamily");
+  family->value = tor_strdup("$1111111111111111111111111111111111111111, "
+                             "1111111111111111111111111111111111111112, "
+                             "$1111111111111111111111111111111111111113");
+
+  config_line_t *family2 = tor_malloc_zero(sizeof(config_line_t));
+  family2->key = tor_strdup("MyFamily");
+  family2->value = tor_strdup("1111111111111111111111111111111111111114");
+
+  config_line_t *family3 = tor_malloc_zero(sizeof(config_line_t));
+  family3->key = tor_strdup("MyFamily");
+  family3->value = tor_strdup("$1111111111111111111111111111111111111115");
+
+  family->next = family2;
+  family2->next = family3;
+  family3->next = NULL;
 
   or_options_t* options = options_new();
   or_options_t* defaults = options_new();
@@ -864,7 +878,7 @@ test_config_fix_my_family(void *arg)
 
   options_init(options);
   options_init(defaults);
-  options->MyFamily = tor_strdup(family);
+  options->MyFamily_lines = family;
 
   options_validate(NULL, options, defaults, 0, &err) ;
 
@@ -872,18 +886,23 @@ test_config_fix_my_family(void *arg)
     TT_FAIL(("options_validate failed: %s", err));
   }
 
-  tt_str_op(options->MyFamily,OP_EQ,
-                                "$1111111111111111111111111111111111111111, "
-                                "$1111111111111111111111111111111111111112, "
-                                "$1111111111111111111111111111111111111113");
-
-  done:
-    if (err != NULL) {
-      tor_free(err);
-    }
+  const char *valid[] = { "$1111111111111111111111111111111111111111",
+                          "$1111111111111111111111111111111111111112",
+                          "$1111111111111111111111111111111111111113",
+                          "$1111111111111111111111111111111111111114",
+                          "$1111111111111111111111111111111111111115" };
+  int ret_size = 0;
+  config_line_t *ret;
+  for (ret = options->MyFamily; ret && ret_size < 5; ret = ret->next) {
+    tt_str_op(ret->value, OP_EQ, valid[ret_size]);
+    ret_size++;
+  }
+  tt_int_op(ret_size, OP_EQ, 5);
 
-    or_options_free(options);
-    or_options_free(defaults);
+ done:
+  tor_free(err);
+  or_options_free(options);
+  or_options_free(defaults);
 }
 
 static int n_hostname_01010101 = 0;