From 803e769fb255f39dba07b5dbc2ca62a43b43004c Mon Sep 17 00:00:00 2001
From: David Goulet <dgoulet@torproject.org>
Date: Tue, 21 Jul 2020 11:34:13 -0400
Subject: [PATCH] relay: Remove possible ORPorts duplicate from parsed list

Now that tor automatically binds to IPv4 _and_ IPv6, in order to avoid
breaking configurations, we sanitize the parsed lists for duplicate ORPorts.
It is possible to happen because we still allow this configuration;

  ORPort 9888
  ORPort [4242::1]:9888

Meaning that the first ORPort value will bind to 0.0.0.0:9888 _and_ [::]:9888
which would lead to an error when attempting to bind on [4242::1]:9888.
However, that configuration is accepted today and thus we must not break it.

To remedy, we now sanitize the parsed list and prioritize an ORPort that has
an explicit address over the global one.

A warning is emitted if such configuration pattern is found. This is only for
the ORPort.

Related to #33246

Signed-off-by: David Goulet <dgoulet@torproject.org>
---
 src/feature/relay/relay_config.c | 89 +++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/src/feature/relay/relay_config.c b/src/feature/relay/relay_config.c
index cddb031f8f..c8a30d63d1 100644
--- a/src/feature/relay/relay_config.c
+++ b/src/feature/relay/relay_config.c
@@ -133,12 +133,96 @@ port_warn_nonlocal_ext_orports(const smartlist_t *ports, const char *portname)
   } SMARTLIST_FOREACH_END(port);
 }
 
+/** Return a static buffer containing the human readable logging string that
+ * describes the given port object. */
+static const char *
+describe_relay_port(const port_cfg_t *port)
+{
+  IF_BUG_ONCE(!port) {
+    return "<null port>";
+  }
+
+  static char buf[256];
+  const char *type, *addr;
+
+  switch (port->type) {
+  case CONN_TYPE_OR_LISTENER:
+    type = "OR";
+    break;
+  case CONN_TYPE_DIR_LISTENER:
+    type = "Dir";
+    break;
+  case CONN_TYPE_EXT_OR_LISTENER:
+    type = "ExtOR";
+    break;
+  default:
+    type = "";
+    break;
+  }
+
+  if (port->explicit_addr) {
+    addr = fmt_and_decorate_addr(&port->addr);
+  } else {
+    addr = "";
+  }
+
+  tor_snprintf(buf, sizeof(buf), "%sPort %s%s%d",
+               type, addr, (strlen(addr) > 0) ? ":" : "", port->port);
+  return buf;
+}
+
+/** 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:
+ *
+ *    ORPort 9050
+ *    ORPort [4242::1]:9050
+ *
+ * First one binds to both v4 and v6 address but second one is specific to an
+ * address superseding the global bind one.
+ *
+ * The following is O(n^2) but it is done at bootstrap or config reload and
+ * the list is not very long usually. */
+static void
+remove_duplicate_orports(smartlist_t *ports)
+{
+  for (int i = 0; i < smartlist_len(ports); ++i) {
+    port_cfg_t *current = smartlist_get(ports, i);
+
+    /* Skip non ORPorts. */
+    if (current->type != CONN_TYPE_OR_LISTENER) {
+      continue;
+    }
+
+    for (int j = 0; j < smartlist_len(ports); ++j) {
+      port_cfg_t *next = smartlist_get(ports, j);
+
+      /* Avoid comparing the same object. */
+      if (current == next) {
+        continue;
+      }
+      /* Same address family and same port number, we have a match. */
+      if (!current->explicit_addr && next->explicit_addr &&
+          tor_addr_family(&current->addr) == tor_addr_family(&next->addr) &&
+          current->port == next->port) {
+        /* Remove current because next is explicitly set. */
+        smartlist_del_keeporder(ports, i--);
+        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);
+        port_cfg_free(current);
+      }
+    }
+  }
+}
+
 /** Given a list of <b>port_cfg_t</b> in <b>ports</b>, check them for internal
  * consistency and warn as appropriate.  On Unix-based OSes, set
  * *<b>n_low_ports_out</b> to the number of sub-1024 ports we will be
  * binding, and warn if we may be unable to re-bind after hibernation. */
 static int
-check_server_ports(const smartlist_t *ports,
+check_server_ports(smartlist_t *ports,
                    const or_options_t *options,
                    int *n_low_ports_out)
 {
@@ -159,6 +243,9 @@ check_server_ports(const smartlist_t *ports,
   int n_low_port = 0;
   int r = 0;
 
+  /* Remove possible duplicate ORPorts before inspecting the list. */
+  remove_duplicate_orports(ports);
+
   SMARTLIST_FOREACH_BEGIN(ports, const port_cfg_t *, port) {
     if (port->type == CONN_TYPE_DIR_LISTENER) {
       if (! port->server_cfg.no_advertise)