Merge branch 'maint-0.4.5'

4 files changed, 73 insertions, 7 deletions

diff --git a/changes/ticket40289 b/changes/ticket40289
new file mode 100644
index 0000000000..cdb36825b0
--- /dev/null
+++ b/changes/ticket40289
@@ -0,0 +1,6 @@
+  o Minor bugfixes (relay, config):
+    - Fix a problem in the removal of duplicate ORPort from the internal port
+      list when loading config file. We were removing wrong ports breaking valid
+      torrc uses cases for multiple ORPorts of the same address family. Fixes
+      bug 40289; bugfix on 0.4.5.1-alpha.
+
diff --git a/src/app/config/config.c b/src/app/config/config.c
index a4dbac4b49..1e4d18116c 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -6046,6 +6046,7 @@ port_parse_config(smartlist_t *out,
       port = (int) tor_parse_long(addrport, 10, 0, 65535, &ok, NULL);
       if (ok) {
         tor_addr_copy(&addr, &default_addr);
+        addr_is_explicit = false;
       } else if (tor_addr_port_lookup(addrport, &addr, &ptmp) == 0) {
         if (ptmp == 0) {
           log_warn(LD_CONFIG, "%sPort line has address but no port", portname);
diff --git a/src/feature/relay/relay_config.c b/src/feature/relay/relay_config.c
index e007a3bd0d..c4a5d7f572 100644
--- a/src/feature/relay/relay_config.c
+++ b/src/feature/relay/relay_config.c
@@ -188,6 +188,41 @@ describe_relay_port(const port_cfg_t *port)
   return buf;
 }
 
+/** Return true iff port p1 is equal to p2.
+ *
+ * This does a field by field comparaison. */
+static bool
+port_cfg_eq(const port_cfg_t *p1, const port_cfg_t *p2)
+{
+  bool ret = true;
+
+  tor_assert(p1);
+  tor_assert(p2);
+
+  /* Address, port and type. */
+  ret &= tor_addr_eq(&p1->addr, &p2->addr);
+  ret &= (p1->port == p2->port);
+  ret &= (p1->type == p2->type);
+
+  /* Mode. */
+  ret &= (p1->is_unix_addr == p2->is_unix_addr);
+  ret &= (p1->is_group_writable == p2->is_group_writable);
+  ret &= (p1->is_world_writable == p2->is_world_writable);
+  ret &= (p1->relax_dirmode_check == p2->relax_dirmode_check);
+  ret &= (p1->explicit_addr == p2->explicit_addr);
+
+  /* Entry config flags. */
+  ret &= tor_memeq(&p1->entry_cfg, &p2->entry_cfg,
+                    sizeof(entry_port_cfg_t));
+  /* Server config flags. */
+  ret &= tor_memeq(&p1->server_cfg, &p2->server_cfg,
+                    sizeof(server_port_cfg_t));
+  /* Unix address path if any. */
+  ret &= !strcmp(p1->unix_addr, p2->unix_addr);
+
+  return ret;
+}
+
 /** Attempt to find duplicate ORPort that would be superseded by another and
  * remove them from the given ports list. This is possible if we have for
  * instance:
@@ -241,20 +276,42 @@ remove_duplicate_orports(smartlist_t *ports)
       if (next->type != CONN_TYPE_OR_LISTENER) {
         continue;
       }
+      /* Remove duplicates. */
+      if (port_cfg_eq(current, next)) {
+        removing[j] = true;
+        continue;
+      }
       /* Don't compare addresses of different family. */
       if (tor_addr_family(&current->addr) != tor_addr_family(&next->addr)) {
         continue;
       }
+      /* At this point, we have a port of the same type and same address
+       * family. Now, we want to avoid comparing addresses that are different
+       * but are both explicit. As an example, these are not duplicates:
+       *
+       *    ORPort 127.0.0.:9001 NoAdvertise
+       *    ORPort 1.2.3.4:9001 NoListen
+       *
+       * Any implicit address must be considered for removal since an explicit
+       * one will always supersedes it. */
+      if (!tor_addr_eq(&current->addr, &next->addr) &&
+          current->explicit_addr && next->explicit_addr) {
+        continue;
+      }
 
-      /* Same port, we keep the explicit one. */
+      /* Port value is the same so we either have a duplicate or a port that
+       * supersedes another. */
       if (current->port == next->port) {
-        removing[j] = true;
+        /* Do not remove the explicit address. As stated before above, we keep
+         * explicit addresses which supersedes implicit ones. */
         if (!current->explicit_addr && next->explicit_addr) {
-          char *next_str = tor_strdup(describe_relay_port(next));
-          log_warn(LD_CONFIG, "Configuration port %s superseded by %s",
-                   describe_relay_port(current), next_str);
-          tor_free(next_str);
+          continue;
         }
+        removing[j] = true;
+        char *next_str = tor_strdup(describe_relay_port(next));
+        log_warn(LD_CONFIG, "Configuration port %s superseded by %s",
+                 next_str, describe_relay_port(current));
+        tor_free(next_str);
       }
     }
   }
diff --git a/src/test/test_config.c b/src/test/test_config.c
index eacf12a25f..73c8ca0549 100644
--- a/src/test/test_config.c
+++ b/src/test/test_config.c
@@ -6924,12 +6924,14 @@ test_config_duplicate_orports(void *arg)
 
   /* We have four address here, 1 IPv4 on 9050, IPv6 on 9050, IPv6 on 9051 and
    * a different IPv6 on 9051. */
-  tt_int_op(smartlist_len(ports), OP_EQ, 3);
+  tt_int_op(smartlist_len(ports), OP_EQ, 4);
   tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ,
             "ORPort 9050");
   tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ,
             "ORPort [4242::1]:9051");
   tt_str_op(describe_relay_port(smartlist_get(ports, 2)), OP_EQ,
+            "ORPort [4242::2]:9051");
+  tt_str_op(describe_relay_port(smartlist_get(ports, 3)), OP_EQ,
             "ORPort 9050");
 
   /* Reset. Test different ORPort value. */