Merge remote-tracking branch 'tor-github/pr/1797/head'

2 files changed, 89 insertions, 95 deletions

diff --git a/changes/ticket32994 b/changes/ticket32994
new file mode 100644
index 0000000000..43a32afa78
--- /dev/null
+++ b/changes/ticket32994
@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Rewrite port_parse_config() to use the default port flags from
+      port_cfg_new(). Closes ticket 32994. Patch by MrSquanchee.
diff --git a/src/app/config/config.c b/src/app/config/config.c
index 3047d2ebb5..4becae4756 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -5888,12 +5888,18 @@ port_cfg_new(size_t namelen)
 {
   tor_assert(namelen <= SIZE_T_CEILING - sizeof(port_cfg_t) - 1);
   port_cfg_t *cfg = tor_malloc_zero(sizeof(port_cfg_t) + namelen + 1);
+
+  /* entry_cfg flags */
   cfg->entry_cfg.ipv4_traffic = 1;
   cfg->entry_cfg.ipv6_traffic = 1;
   cfg->entry_cfg.prefer_ipv6 = 1;
   cfg->entry_cfg.dns_request = 1;
   cfg->entry_cfg.onion_traffic = 1;
   cfg->entry_cfg.prefer_ipv6_virtaddr = 1;
+  cfg->entry_cfg.session_group = SESSION_GROUP_UNSET;
+  cfg->entry_cfg.isolation_flags = ISO_DEFAULT;
+
+  /* Other flags default to 0 due to tor_malloc_zero */
   return cfg;
 }
 
@@ -6097,11 +6103,12 @@ port_parse_config(smartlist_t *out,
   const unsigned is_unix_socket = flags & CL_PORT_IS_UNIXSOCKET;
   int got_zero_port=0, got_nonzero_port=0;
   char *unix_socket_path = NULL;
+  port_cfg_t *cfg = NULL;
 
   /* If there's no FooPort, then maybe make a default one. */
   if (! ports) {
     if (defaultport && defaultaddr && out) {
-      port_cfg_t *cfg = port_cfg_new(is_unix_socket ? strlen(defaultaddr) : 0);
+       cfg = port_cfg_new(is_unix_socket ? strlen(defaultaddr) : 0);
        cfg->type = listener_type;
        if (is_unix_socket) {
          tor_addr_make_unspec(&cfg->addr);
@@ -6111,8 +6118,6 @@ port_parse_config(smartlist_t *out,
          cfg->port = defaultport;
          tor_addr_parse(&cfg->addr, defaultaddr);
        }
-       cfg->entry_cfg.session_group = SESSION_GROUP_UNSET;
-       cfg->entry_cfg.isolation_flags = ISO_DEFAULT;
        smartlist_add(out, cfg);
     }
     return 0;
@@ -6126,28 +6131,12 @@ port_parse_config(smartlist_t *out,
   for (; ports; ports = ports->next) {
     tor_addr_t addr;
     tor_addr_make_unspec(&addr);
-
-    int port;
-    int sessiongroup = SESSION_GROUP_UNSET;
-    unsigned isolation = ISO_DEFAULT;
-    int prefer_no_auth = 0;
-    int socks_iso_keep_alive = 0;
-
+    int port, ok,
+        has_used_unix_socket_only_option = 0,
+        is_unix_tagged_addr = 0;
     uint16_t ptmp=0;
-    int ok;
-    /* This must be kept in sync with port_cfg_new's defaults */
-    int no_listen = 0, no_advertise = 0, all_addrs = 0,
-      bind_ipv4_only = 0, bind_ipv6_only = 0,
-      ipv4_traffic = 1, ipv6_traffic = 1, prefer_ipv6 = 1, dns_request = 1,
-      onion_traffic = 1,
-      cache_ipv4 = 0, use_cached_ipv4 = 0,
-      cache_ipv6 = 0, use_cached_ipv6 = 0,
-      prefer_ipv6_automap = 1, world_writable = 0, group_writable = 0,
-      relax_dirmode_check = 0,
-      has_used_unix_socket_only_option = 0, extended_errors = 0;
-
-    int is_unix_tagged_addr = 0;
     const char *rest_of_line = NULL;
+
     if (port_cfg_line_extract_addrport(ports->value,
                           &addrport, &is_unix_tagged_addr, &rest_of_line)<0) {
       log_warn(LD_CONFIG, "Invalid %sPort line with unparsable address",
@@ -6223,17 +6212,20 @@ port_parse_config(smartlist_t *out,
       }
     }
 
+    /* Default port_cfg_t object initialization */
+    cfg = port_cfg_new(unix_socket_path ? strlen(unix_socket_path) : 0);
+
     if (unix_socket_path && default_to_group_writable)
-      group_writable = 1;
+      cfg->is_group_writable = 1;
 
     /* Now parse the rest of the options, if any. */
     if (use_server_options) {
       /* This is a server port; parse advertising options */
       SMARTLIST_FOREACH_BEGIN(elts, char *, elt) {
         if (!strcasecmp(elt, "NoAdvertise")) {
-          no_advertise = 1;
+          cfg->server_cfg.no_advertise = 1;
         } else if (!strcasecmp(elt, "NoListen")) {
-          no_listen = 1;
+          cfg->server_cfg.no_listen = 1;
 #if 0
         /* not implemented yet. */
         } else if (!strcasecmp(elt, "AllAddrs")) {
@@ -6241,33 +6233,36 @@ port_parse_config(smartlist_t *out,
           all_addrs = 1;
 #endif /* 0 */
         } else if (!strcasecmp(elt, "IPv4Only")) {
-          bind_ipv4_only = 1;
+          cfg->server_cfg.bind_ipv4_only = 1;
         } else if (!strcasecmp(elt, "IPv6Only")) {
-          bind_ipv6_only = 1;
+          cfg->server_cfg.bind_ipv6_only = 1;
         } else {
           log_warn(LD_CONFIG, "Unrecognized %sPort option '%s'",
                    portname, escaped(elt));
         }
       } SMARTLIST_FOREACH_END(elt);
 
-      if (no_advertise && no_listen) {
+      if (cfg->server_cfg.no_advertise && cfg->server_cfg.no_listen) {
         log_warn(LD_CONFIG, "Tried to set both NoListen and NoAdvertise "
                  "on %sPort line '%s'",
                  portname, escaped(ports->value));
         goto err;
       }
-      if (bind_ipv4_only && bind_ipv6_only) {
+      if (cfg->server_cfg.bind_ipv4_only &&
+          cfg->server_cfg.bind_ipv6_only) {
         log_warn(LD_CONFIG, "Tried to set both IPv4Only and IPv6Only "
                  "on %sPort line '%s'",
                  portname, escaped(ports->value));
         goto err;
       }
-      if (bind_ipv4_only && tor_addr_family(&addr) != AF_INET) {
+      if (cfg->server_cfg.bind_ipv4_only &&
+          tor_addr_family(&addr) != AF_INET) {
         log_warn(LD_CONFIG, "Could not interpret %sPort address as IPv4",
                  portname);
         goto err;
       }
-      if (bind_ipv6_only && tor_addr_family(&addr) != AF_INET6) {
+      if (cfg->server_cfg.bind_ipv6_only &&
+          tor_addr_family(&addr) != AF_INET6) {
         log_warn(LD_CONFIG, "Could not interpret %sPort address as IPv6",
                  portname);
         goto err;
@@ -6286,12 +6281,12 @@ port_parse_config(smartlist_t *out,
                      portname, escaped(elt));
             goto err;
           }
-          if (sessiongroup >= 0) {
+          if (cfg->entry_cfg.session_group >= 0) {
             log_warn(LD_CONFIG, "Multiple SessionGroup options on %sPort",
                      portname);
             goto err;
           }
-          sessiongroup = group;
+          cfg->entry_cfg.session_group = group;
           continue;
         }
 
@@ -6301,15 +6296,15 @@ port_parse_config(smartlist_t *out,
         }
 
         if (!strcasecmp(elt, "GroupWritable")) {
-          group_writable = !no;
+          cfg->is_group_writable = !no;
           has_used_unix_socket_only_option = 1;
           continue;
         } else if (!strcasecmp(elt, "WorldWritable")) {
-          world_writable = !no;
+          cfg->is_world_writable = !no;
           has_used_unix_socket_only_option = 1;
           continue;
         } else if (!strcasecmp(elt, "RelaxDirModeCheck")) {
-          relax_dirmode_check = !no;
+          cfg->relax_dirmode_check = !no;
           has_used_unix_socket_only_option = 1;
           continue;
         }
@@ -6322,19 +6317,19 @@ port_parse_config(smartlist_t *out,
 
         if (takes_hostnames) {
           if (!strcasecmp(elt, "IPv4Traffic")) {
-            ipv4_traffic = ! no;
+            cfg->entry_cfg.ipv4_traffic = ! no;
             continue;
           } else if (!strcasecmp(elt, "IPv6Traffic")) {
-            ipv6_traffic = ! no;
+            cfg->entry_cfg.ipv6_traffic = ! no;
             continue;
           } else if (!strcasecmp(elt, "PreferIPv6")) {
-            prefer_ipv6 = ! no;
+            cfg->entry_cfg.prefer_ipv6 = ! no;
             continue;
           } else if (!strcasecmp(elt, "DNSRequest")) {
-            dns_request = ! no;
+            cfg->entry_cfg.dns_request = ! no;
             continue;
           } else if (!strcasecmp(elt, "OnionTraffic")) {
-            onion_traffic = ! no;
+            cfg->entry_cfg.onion_traffic = ! no;
             continue;
           } else if (!strcasecmp(elt, "OnionTrafficOnly")) {
             /* Only connect to .onion addresses.  Equivalent to
@@ -6345,46 +6340,50 @@ port_parse_config(smartlist_t *out,
                        "DNSRequest, IPv4Traffic, and/or IPv6Traffic instead.",
                        portname, escaped(elt));
             } else {
-              ipv4_traffic = ipv6_traffic = dns_request = 0;
+              cfg->entry_cfg.ipv4_traffic = 0;
+              cfg->entry_cfg.ipv6_traffic = 0;
+              cfg->entry_cfg.dns_request = 0;
             }
             continue;
           }
         }
         if (!strcasecmp(elt, "CacheIPv4DNS")) {
           warn_client_dns_cache(elt, no); // since 0.2.9.2-alpha
-          cache_ipv4 = ! no;
+          cfg->entry_cfg.cache_ipv4_answers = ! no;
           continue;
         } else if (!strcasecmp(elt, "CacheIPv6DNS")) {
           warn_client_dns_cache(elt, no); // since 0.2.9.2-alpha
-          cache_ipv6 = ! no;
+          cfg->entry_cfg.cache_ipv6_answers = ! no;
           continue;
         } else if (!strcasecmp(elt, "CacheDNS")) {
           warn_client_dns_cache(elt, no); // since 0.2.9.2-alpha
-          cache_ipv4 = cache_ipv6 = ! no;
+          cfg->entry_cfg.cache_ipv4_answers = ! no;
+          cfg->entry_cfg.cache_ipv6_answers = ! no;
           continue;
         } else if (!strcasecmp(elt, "UseIPv4Cache")) {
           warn_client_dns_cache(elt, no); // since 0.2.9.2-alpha
-          use_cached_ipv4 = ! no;
+          cfg->entry_cfg.use_cached_ipv4_answers = ! no;
           continue;
         } else if (!strcasecmp(elt, "UseIPv6Cache")) {
           warn_client_dns_cache(elt, no); // since 0.2.9.2-alpha
-          use_cached_ipv6 = ! no;
+          cfg->entry_cfg.use_cached_ipv6_answers = ! no;
           continue;
         } else if (!strcasecmp(elt, "UseDNSCache")) {
           warn_client_dns_cache(elt, no); // since 0.2.9.2-alpha
-          use_cached_ipv4 = use_cached_ipv6 = ! no;
+          cfg->entry_cfg.use_cached_ipv4_answers = ! no;
+          cfg->entry_cfg.use_cached_ipv6_answers = ! no;
           continue;
         } else if (!strcasecmp(elt, "PreferIPv6Automap")) {
-          prefer_ipv6_automap = ! no;
+          cfg->entry_cfg.prefer_ipv6_virtaddr = ! no;
           continue;
         } else if (!strcasecmp(elt, "PreferSOCKSNoAuth")) {
-          prefer_no_auth = ! no;
+          cfg->entry_cfg.socks_prefer_no_auth = ! no;
           continue;
         } else if (!strcasecmp(elt, "KeepAliveIsolateSOCKSAuth")) {
-          socks_iso_keep_alive = ! no;
+          cfg->entry_cfg.socks_iso_keep_alive = ! no;
           continue;
         } else if (!strcasecmp(elt, "ExtendedErrors")) {
-          extended_errors = ! no;
+          cfg->entry_cfg.extended_socks5_codes = ! no;
           continue;
         }
 
@@ -6407,9 +6406,9 @@ port_parse_config(smartlist_t *out,
         }
 
         if (no) {
-          isolation &= ~isoflag;
+          cfg->entry_cfg.isolation_flags &= ~isoflag;
         } else {
-          isolation |= isoflag;
+          cfg->entry_cfg.isolation_flags |= isoflag;
         }
       } SMARTLIST_FOREACH_END(elt);
     }
@@ -6419,51 +6418,51 @@ port_parse_config(smartlist_t *out,
     else
       got_zero_port = 1;
 
-    if (dns_request == 0 && listener_type == CONN_TYPE_AP_DNS_LISTENER) {
+    if (cfg->entry_cfg.dns_request == 0 &&
+        listener_type == CONN_TYPE_AP_DNS_LISTENER) {
       log_warn(LD_CONFIG, "You have a %sPort entry with DNS disabled; that "
                "won't work.", portname);
       goto err;
     }
-
-    if (ipv4_traffic == 0 && ipv6_traffic == 0 && onion_traffic == 0
-        && listener_type != CONN_TYPE_AP_DNS_LISTENER) {
+    if (cfg->entry_cfg.ipv4_traffic == 0 &&
+        cfg->entry_cfg.ipv6_traffic == 0 &&
+        cfg->entry_cfg.onion_traffic == 0 &&
+        listener_type != CONN_TYPE_AP_DNS_LISTENER) {
       log_warn(LD_CONFIG, "You have a %sPort entry with all of IPv4 and "
                "IPv6 and .onion disabled; that won't work.", portname);
       goto err;
     }
-
-    if (dns_request == 1 && ipv4_traffic == 0 && ipv6_traffic == 0
-        && listener_type != CONN_TYPE_AP_DNS_LISTENER) {
+    if (cfg->entry_cfg.dns_request == 1 &&
+        cfg->entry_cfg.ipv4_traffic == 0 &&
+        cfg->entry_cfg.ipv6_traffic == 0 &&
+        listener_type != CONN_TYPE_AP_DNS_LISTENER) {
       log_warn(LD_CONFIG, "You have a %sPort entry with DNSRequest enabled, "
                "but IPv4 and IPv6 disabled; DNS-based sites won't work.",
                portname);
       goto err;
     }
-
     if ( has_used_unix_socket_only_option && ! unix_socket_path) {
       log_warn(LD_CONFIG, "You have a %sPort entry with GroupWritable, "
                "WorldWritable, or RelaxDirModeCheck, but it is not a "
                "unix socket.", portname);
       goto err;
     }
-
-    if (!(isolation & ISO_SOCKSAUTH) && socks_iso_keep_alive) {
+    if (!(cfg->entry_cfg.isolation_flags & ISO_SOCKSAUTH) &&
+        cfg->entry_cfg.socks_iso_keep_alive) {
       log_warn(LD_CONFIG, "You have a %sPort entry with both "
                "NoIsolateSOCKSAuth and KeepAliveIsolateSOCKSAuth set.",
                portname);
       goto err;
     }
-
-    if (unix_socket_path && (isolation & ISO_CLIENTADDR)) {
+    if (unix_socket_path &&
+        (cfg->entry_cfg.isolation_flags & ISO_CLIENTADDR)) {
       /* `IsolateClientAddr` is nonsensical in the context of AF_LOCAL.
        * just silently remove the isolation flag.
        */
-      isolation &= ~ISO_CLIENTADDR;
+      cfg->entry_cfg.isolation_flags &= ~ISO_CLIENTADDR;
     }
-
     if (out && port) {
       size_t namelen = unix_socket_path ? strlen(unix_socket_path) : 0;
-      port_cfg_t *cfg = port_cfg_new(namelen);
       if (unix_socket_path) {
         tor_addr_make_unspec(&cfg->addr);
         memcpy(cfg->unix_addr, unix_socket_path, namelen + 1);
@@ -6474,33 +6473,13 @@ port_parse_config(smartlist_t *out,
         cfg->port = port;
       }
       cfg->type = listener_type;
-      cfg->is_world_writable = world_writable;
-      cfg->is_group_writable = group_writable;
-      cfg->relax_dirmode_check = relax_dirmode_check;
-      cfg->entry_cfg.isolation_flags = isolation;
-      cfg->entry_cfg.session_group = sessiongroup;
-      cfg->server_cfg.no_advertise = no_advertise;
-      cfg->server_cfg.no_listen = no_listen;
-      cfg->server_cfg.all_addrs = all_addrs;
-      cfg->server_cfg.bind_ipv4_only = bind_ipv4_only;
-      cfg->server_cfg.bind_ipv6_only = bind_ipv6_only;
-      cfg->entry_cfg.ipv4_traffic = ipv4_traffic;
-      cfg->entry_cfg.ipv6_traffic = ipv6_traffic;
-      cfg->entry_cfg.prefer_ipv6 = prefer_ipv6;
-      cfg->entry_cfg.dns_request = dns_request;
-      cfg->entry_cfg.onion_traffic = onion_traffic;
-      cfg->entry_cfg.cache_ipv4_answers = cache_ipv4;
-      cfg->entry_cfg.cache_ipv6_answers = cache_ipv6;
-      cfg->entry_cfg.use_cached_ipv4_answers = use_cached_ipv4;
-      cfg->entry_cfg.use_cached_ipv6_answers = use_cached_ipv6;
-      cfg->entry_cfg.prefer_ipv6_virtaddr = prefer_ipv6_automap;
-      cfg->entry_cfg.socks_prefer_no_auth = prefer_no_auth;
-      if (! (isolation & ISO_SOCKSAUTH))
+      if (! (cfg->entry_cfg.isolation_flags & ISO_SOCKSAUTH))
         cfg->entry_cfg.socks_prefer_no_auth = 1;
-      cfg->entry_cfg.socks_iso_keep_alive = socks_iso_keep_alive;
-      cfg->entry_cfg.extended_socks5_codes = extended_errors;
-
       smartlist_add(out, cfg);
+      /* out owns cfg now, don't re-use or free it */
+      cfg = NULL;
+    } else {
+      tor_free(cfg);
     }
     SMARTLIST_FOREACH(elts, char *, cp, tor_free(cp));
     smartlist_clear(elts);
@@ -6526,10 +6505,22 @@ port_parse_config(smartlist_t *out,
 
   retval = 0;
  err:
+  /* There are two ways we can error out:
+   *  1. part way through the loop: cfg needs to be freed;
+   *  2. ending the loop normally: cfg is always NULL.
+   *     In this case, cfg has either been:
+   *     - added to out, then set to NULL, or
+   *     - freed and set to NULL (because out is NULL, or port is 0).
+   */
+  tor_free(cfg);
+
+  /* Free the other variables from the loop.
+   * elts is always non-NULL here, but it may or may not be empty. */
   SMARTLIST_FOREACH(elts, char *, cp, tor_free(cp));
   smartlist_free(elts);
   tor_free(unix_socket_path);
   tor_free(addrport);
+
   return retval;
 }