port: Don't ignore ports of a different family

Commit c3a0f757964de0e8a24911d72abff5df20bb323c added this feature for ORPort
that we ignore any port that is not the family of our default address when
parsing the port. So if port_parse_config() was called with an IPv4 default
address, all IPv6 address would be ignored.

That makes sense for ORPort since we call twice port_parse_config() for
0.0.0.0 and [::] but for the rest of the ports, it is not good since a
perfectly valid configuration can be:

  SocksPort 9050
  SocksPort [::1]:9050

Any non-ORPort only binds by default to an IPv4 except the ORPort that binds
to both IPv4 and IPv6 by default.

The fix here is to always parse all ports within port_parse_config() and then,
specifically for ORPort, remove the duplicates or superseding ones. The
warning is only emitted when a port supersedes another.

A unit tests is added to make sure SocksPort of different family always exists
together.

Fixes #40183

Signed-off-by: David Goulet <dgoulet@torproject.org>

4 files changed, 51 insertions, 20 deletions

diff --git a/changes/ticket40183 b/changes/ticket40183
new file mode 100644
index 0000000000..3c4bdf21e2
--- /dev/null
+++ b/changes/ticket40183
@@ -0,0 +1,4 @@
+  o Minor bugfixes (port configuration):
+    - Second non ORPort of a different family (ex: SocksPort [::1]:9050) was
+      ignored due to a logical configuration parsing error. Fixes bug 40183;
+      bugfix on 0.4.5.1-alpha.
diff --git a/src/app/config/config.c b/src/app/config/config.c
index 458067af4d..04a82a5c43 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -5934,13 +5934,12 @@ port_parse_config(smartlist_t *out,
   char *unix_socket_path = NULL;
   port_cfg_t *cfg = NULL;
   bool addr_is_explicit = false;
-  int family = -1;
-
-  /* Parse default address. This can fail for Unix socket for instance so
-   * family can be -1 and the default_addr will be made UNSPEC. */
   tor_addr_t default_addr = TOR_ADDR_NULL;
+
+  /* Parse default address. This can fail for Unix socket so the default_addr
+   * will simply be made UNSPEC. */
   if (defaultaddr) {
-    family = tor_addr_parse(&default_addr, defaultaddr);
+    tor_addr_parse(&default_addr, defaultaddr);
   }
 
   /* If there's no FooPort, then maybe make a default one. */
@@ -6018,7 +6017,6 @@ port_parse_config(smartlist_t *out,
         port = 1;
     } else if (!strcasecmp(addrport, "auto")) {
       port = CFG_AUTO_PORT;
-      tor_assert(family >= 0);
       tor_addr_copy(&addr, &default_addr);
     } else if (!strcasecmpend(addrport, ":auto")) {
       char *addrtmp = tor_strndup(addrport, strlen(addrport)-5);
@@ -6035,18 +6033,12 @@ port_parse_config(smartlist_t *out,
          "9050" might be a valid address. */
       port = (int) tor_parse_long(addrport, 10, 0, 65535, &ok, NULL);
       if (ok) {
-        tor_assert(family >= 0);
         tor_addr_copy(&addr, &default_addr);
       } 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);
           goto err;
         }
-        if (family != -1 && tor_addr_family(&addr) != family) {
-          /* This means we are parsing another ORPort family but we are
-           * attempting to find the default address' family ORPort. */
-          goto ignore;
-        }
         port = ptmp;
         addr_is_explicit = true;
       } else {
diff --git a/src/feature/relay/relay_config.c b/src/feature/relay/relay_config.c
index 6504c680eb..ea03f43e13 100644
--- a/src/feature/relay/relay_config.c
+++ b/src/feature/relay/relay_config.c
@@ -228,15 +228,16 @@ remove_duplicate_orports(smartlist_t *ports)
         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) &&
+      if (tor_addr_family(&current->addr) == tor_addr_family(&next->addr) &&
           current->port == next->port) {
         /* Remove current because next is explicitly set. */
         removing[i] = true;
-        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);
+        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);
+        }
       }
     }
   }
diff --git a/src/test/test_config.c b/src/test/test_config.c
index a0278f9422..d680ab31ef 100644
--- a/src/test/test_config.c
+++ b/src/test/test_config.c
@@ -6855,9 +6855,15 @@ test_config_duplicate_orports(void *arg)
   port_parse_config(ports, config_port, "OR", CONN_TYPE_OR_LISTENER, "[::]",
                     0, CL_PORT_SERVER_OPTIONS);
 
-  // There should be three ports at this point.
-  tt_int_op(smartlist_len(ports), OP_EQ, 3);
+  /* There should be 4 ports at this point that is:
+   *    - 0.0.0.0:9050
+   *    - [::]:9050
+   *    - [::1]:9050
+   *    - [::1]:9050
+   */
+  tt_int_op(smartlist_len(ports), OP_EQ, 4);
 
+  /* This will remove the [::] and the extra [::1]. */
   remove_duplicate_orports(ports);
 
   // The explicit IPv6 port should have replaced the implicit IPv6 port.
@@ -6869,6 +6875,33 @@ test_config_duplicate_orports(void *arg)
   config_free_lines(config_port);
 }
 
+static void
+test_config_multifamily_port(void *arg)
+{
+  (void) arg;
+
+  config_line_t *config_port = NULL;
+  smartlist_t *ports = smartlist_new();
+
+  config_line_append(&config_port, "SocksPort", "9050");
+  config_line_append(&config_port, "SocksPort", "[::1]:9050");
+
+  // Parse IPv4, then IPv6.
+  port_parse_config(ports, config_port, "SOCKS", CONN_TYPE_AP_LISTENER,
+                    "0.0.0.0", 9050, 0);
+
+  /* There should be 2 ports at this point that is:
+   *    - 0.0.0.0:9050
+   *    - [::1]:9050
+   */
+  tt_int_op(smartlist_len(ports), OP_EQ, 2);
+
+ done:
+  SMARTLIST_FOREACH(ports, port_cfg_t *, cfg, port_cfg_free(cfg));
+  smartlist_free(ports);
+  config_free_lines(config_port);
+}
+
 #ifndef COCCI
 #define CONFIG_TEST(name, flags)                          \
   { #name, test_config_ ## name, flags, NULL, NULL }
@@ -6937,5 +6970,6 @@ struct testcase_t config_tests[] = {
   CONFIG_TEST(kvline_parse, 0),
   CONFIG_TEST(getinfo_config_names, 0),
   CONFIG_TEST(duplicate_orports, 0),
+  CONFIG_TEST(multifamily_port, 0),
   END_OF_TESTCASES
 };