torproject · dgoulet-tor · Nov 23, 2020
diff --git a/changes/ticket40106 b/changes/ticket40106
@@ -0,0 +1,8 @@
+  o Minor bugfixes (config, bridge):
+    - Really fix the case where torrc has a missing Bridge line while configured
+      with a ClientTransportPlugin. Previously, we failed to also look at the
+      managed proxy list and thus it would fail for the "exec" case. Fixes bug
+      40106; bugfix on 0.4.5.1-alpha.
+    - Our code was supporting a *TransportPlugin line to have multiple
+      transport name but our documentation never detailed it. Remove that
+      capability in favor of using multiple *TransportPlugin line instead.
diff --git a/src/app/config/config.c b/src/app/config/config.c
@@ -2198,7 +2198,8 @@ options_act,(const or_options_t *old_options))
    * validation time. */
   SMARTLIST_FOREACH_BEGIN(bridge_list_get(), const bridge_info_t *, bi) {
     const char *bi_transport_name = bridget_get_transport_name(bi);
-    if (bi_transport_name && !transport_get_by_name(bi_transport_name)) {
+    if (bi_transport_name && (!transport_get_by_name(bi_transport_name) &&
+                          !managed_proxy_has_transport(bi_transport_name))) {
       log_warn(LD_CONFIG, "Bridge line with transport %s is missing a "
                           "ClientTransportPlugin line", bi_transport_name);
       return -1;
@@ -5266,8 +5267,7 @@ pt_parse_transport_line(const or_options_t *options,
 
   smartlist_t *items = NULL;
   int r;
-  const char *transports = NULL;
-  smartlist_t *transport_list = NULL;
+  const char *transport_name = NULL;
   char *type = NULL;
   char *addrport = NULL;
   tor_addr_t addr;
@@ -5279,7 +5279,6 @@ pt_parse_transport_line(const or_options_t *options,
   char **proxy_argv = NULL;
   char **tmp = NULL;
   int proxy_argc, i;
-  int is_useless_proxy = 1;
 
   int line_length;
 
@@ -5296,25 +5295,20 @@ pt_parse_transport_line(const or_options_t *options,
     goto err;
   }
 
-  /* Get the first line element, split it to commas into
-     transport_list (in case it's multiple transports) and validate
-     the transport names. */
-  transports = smartlist_get(items, 0);
-  transport_list = smartlist_new();
-  smartlist_split_string(transport_list, transports, ",",
-                         SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
-  SMARTLIST_FOREACH_BEGIN(transport_list, const char *, transport_name) {
-    /* validate transport names */
-    if (!string_is_C_identifier(transport_name)) {
-      log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).",
-               transport_name);
-      goto err;
-    }
-
-    /* see if we actually need the transports provided by this proxy */
-    if (!validate_only && transport_is_needed(transport_name))
-      is_useless_proxy = 0;
-  } SMARTLIST_FOREACH_END(transport_name);
+  /* Get transport name. */
+  transport_name = smartlist_get(items, 0);
+  if (!string_is_C_identifier(transport_name)) {
+    log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).",
+             transport_name);
+    goto err;
+  }
+  /* If we can't find a Bridge line for that same transport, it is an error
+   * and thus stop immediately. */
+  if (!validate_only && !server && !transport_is_needed(transport_name)) {
+    log_warn(LD_CONFIG, "ClientTransportPlugin line for %s is missing a "
+                        "corresponding Bridge line.", transport_name);
+    goto err;
+  }
 
   type = smartlist_get(items, 1);
   if (!strcmp(type, "exec")) {
@@ -5357,20 +5351,13 @@ pt_parse_transport_line(const or_options_t *options,
   if (is_managed) {
     /* managed */
 
-    if (!server && !validate_only && is_useless_proxy) {
-      log_info(LD_GENERAL,
-               "Pluggable transport proxy (%s) does not provide "
-               "any needed transports and will not be launched.",
-               line);
-    }
-
     /*
      * If we are not just validating, use the rest of the line as the
      * argv of the proxy to be launched. Also, make sure that we are
      * only launching proxies that contribute useful transports.
      */
 
-    if (!validate_only && (server || !is_useless_proxy)) {
+    if (!validate_only) {
       proxy_argc = line_length - 2;
       tor_assert(proxy_argc > 0);
       proxy_argv = tor_calloc((proxy_argc + 1), sizeof(char *));
@@ -5385,9 +5372,9 @@ pt_parse_transport_line(const or_options_t *options,
 
       /* kickstart the thing */
       if (server) {
-        pt_kickstart_server_proxy(transport_list, proxy_argv);
+        pt_kickstart_server_proxy(transport_name, proxy_argv);
       } else {
-        pt_kickstart_client_proxy(transport_list, proxy_argv);
+        pt_kickstart_client_proxy(transport_name, proxy_argv);
       }
     }
   } else {
@@ -5402,13 +5389,6 @@ pt_parse_transport_line(const or_options_t *options,
       goto err;
     }
 
-    if (smartlist_len(transport_list) != 1) {
-      log_warn(LD_CONFIG,
-               "You can't have an external proxy with more than "
-               "one transport.");
-      goto err;
-    }
-
     addrport = smartlist_get(items, 2);
 
     if (tor_addr_port_lookup(addrport, &addr, &port) < 0) {
@@ -5426,12 +5406,10 @@ pt_parse_transport_line(const or_options_t *options,
     if (!validate_only) {
       log_info(LD_DIR, "%s '%s' at %s.",
                server ? "Server transport" : "Transport",
-               transports, fmt_addrport(&addr, port));
+               transport_name, fmt_addrport(&addr, port));
 
       if (!server) {
-        transport_add_from_config(&addr, port,
-                                  smartlist_get(transport_list, 0),
-                                  socks_ver);
+        transport_add_from_config(&addr, port, transport_name, socks_ver);
       }
     }
   }
@@ -5445,10 +5423,6 @@ pt_parse_transport_line(const or_options_t *options,
  done:
   SMARTLIST_FOREACH(items, char*, s, tor_free(s));
   smartlist_free(items);
-  if (transport_list) {
-    SMARTLIST_FOREACH(transport_list, char*, s, tor_free(s));
-    smartlist_free(transport_list);
-  }
 
   return r;
 }

diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c
@@ -368,6 +368,26 @@ static int unconfigured_proxies_n = 0;
 /** Boolean: True iff we might need to restart some proxies. */
 static int check_if_restarts_needed = 0;
 
+/** Return true iff we have a managed_proxy_t in the global list is for the
+ * given transport name. */
+bool
+managed_proxy_has_transport(const char *transport_name)
+{
+  tor_assert(transport_name);
+
+  if (!managed_proxy_list) {
+    return false;
+  }
+
+  SMARTLIST_FOREACH_BEGIN(managed_proxy_list, const managed_proxy_t *, mp) {
+    if (!strcmp(mp->transport_name, transport_name)) {
+      return true;
+    }
+  } SMARTLIST_FOREACH_END(mp);
+
+  return false;
+}
+
 /** Return true if there are still unconfigured managed proxies, or proxies
  * that need restarting. */
 int
@@ -724,6 +744,7 @@ managed_proxy_destroy(managed_proxy_t *mp,
     process_terminate(mp->process);
   }
 
+  tor_free(mp->transport_name);
   tor_free(mp);
 }
 
@@ -1495,10 +1516,11 @@ create_managed_proxy_environment(const managed_proxy_t *mp)
  *
  * Requires that proxy_argv have at least one element. */
 STATIC managed_proxy_t *
-managed_proxy_create(const smartlist_t *with_transport_list,
-                     char **proxy_argv, int is_server)
+managed_proxy_create(const char *transport_name, char **proxy_argv,
+                     int is_server)
 {
   managed_proxy_t *mp = tor_malloc_zero(sizeof(managed_proxy_t));
+  mp->transport_name = tor_strdup(transport_name);
   mp->conf_state = PT_PROTO_INFANT;
   mp->is_server = is_server;
   mp->argv = proxy_argv;
@@ -1507,8 +1529,7 @@ managed_proxy_create(const smartlist_t *with_transport_list,
   mp->process = process_new(proxy_argv[0]);
 
   mp->transports_to_launch = smartlist_new();
-  SMARTLIST_FOREACH(with_transport_list, const char *, transport,
-                    add_transport_to_proxy(transport, mp));
+  add_transport_to_proxy(mp->transport_name, mp);
 
   /* register the managed proxy */
   if (!managed_proxy_list)
@@ -1521,30 +1542,32 @@ managed_proxy_create(const smartlist_t *with_transport_list,
   return mp;
 }
 
-/** Register proxy with <b>proxy_argv</b>, supporting transports in
- *  <b>transport_list</b>, to the managed proxy subsystem.
- *  If <b>is_server</b> is true, then the proxy is a server proxy.
+/** Register proxy with <b>proxy_argv</b>, supporting the transport name to
+ * the managed proxy subsystem.  If <b>is_server</b> is true, then the proxy
+ * is a server proxy.
  *
  * Takes ownership of proxy_argv.
  *
  * Requires that proxy_argv be a NULL-terminated array of command-line
  * elements, containing at least one element.
  **/
 MOCK_IMPL(void,
-pt_kickstart_proxy, (const smartlist_t *with_transport_list,
-                     char **proxy_argv, int is_server))
+pt_kickstart_proxy,(const char *transport_name, char **proxy_argv,
+                    int is_server))
 {
   managed_proxy_t *mp=NULL;
   transport_t *old_transport = NULL;
 
+  tor_assert(transport_name);
+
   if (!proxy_argv || !proxy_argv[0]) {
     return;
   }
 
   mp = get_managed_proxy_by_argv_and_type(proxy_argv, is_server);
 
   if (!mp) { /* we haven't seen this proxy before */
-    managed_proxy_create(with_transport_list, proxy_argv, is_server);
+    managed_proxy_create(transport_name, proxy_argv, is_server);
 
   } else { /* known proxy. add its transport to its transport list */
     if (mp->was_around_before_config_read) {
@@ -1558,18 +1581,15 @@ pt_kickstart_proxy, (const smartlist_t *with_transport_list,
         check_if_restarts_needed = 1;
       }
 
-      /* For each new transport, check if the managed proxy used to
+      /* For the transport, check if the managed proxy used to
          support it before the SIGHUP. If that was the case, make sure
          it doesn't get removed because we might reuse it. */
-      SMARTLIST_FOREACH_BEGIN(with_transport_list, const char *, transport) {
-        old_transport = transport_get_by_name(transport);
-        if (old_transport)
-          old_transport->marked_for_removal = 0;
-      } SMARTLIST_FOREACH_END(transport);
+      old_transport = transport_get_by_name(transport_name);
+      if (old_transport)
+        old_transport->marked_for_removal = 0;
     }
 
-    SMARTLIST_FOREACH(with_transport_list, const char *, transport,
-                      add_transport_to_proxy(transport, mp));
+    add_transport_to_proxy(transport_name, mp);
     free_execve_args(proxy_argv);
   }
 }

diff --git a/src/feature/client/transports.h b/src/feature/client/transports.h
@@ -41,15 +41,15 @@ void transport_free_(transport_t *transport);
 #define transport_free(tr) FREE_AND_NULL(transport_t, transport_free_, (tr))
 
 MOCK_DECL(transport_t*, transport_get_by_name, (const char *name));
+bool managed_proxy_has_transport(const char *transport_name);
 
 MOCK_DECL(void, pt_kickstart_proxy,
-          (const smartlist_t *transport_list, char **proxy_argv,
-           int is_server));
+          (const char *transport_name, char **proxy_argv, int is_server));
 
-#define pt_kickstart_client_proxy(tl, pa)  \
-  pt_kickstart_proxy(tl, pa, 0)
-#define pt_kickstart_server_proxy(tl, pa) \
-  pt_kickstart_proxy(tl, pa, 1)
+#define pt_kickstart_client_proxy(name, pa)  \
+  pt_kickstart_proxy(name, pa, 0)
+#define pt_kickstart_server_proxy(name, pa) \
+  pt_kickstart_proxy(name, pa, 1)
 
 void pt_configure_remaining_proxies(void);
 
@@ -87,6 +87,7 @@ struct process_t;
 
 /** Structure containing information of a managed proxy. */
 typedef struct {
+  char *transport_name; /* Transport name */
   enum pt_proto_state conf_state; /* the current configuration state */
   char **argv; /* the cli arguments of this proxy */
   int conf_protocol; /* the configuration protocol version used */
@@ -135,8 +136,9 @@ STATIC char *get_transport_options_for_server_proxy(const managed_proxy_t *mp);
 STATIC void managed_proxy_destroy(managed_proxy_t *mp,
                                   int also_terminate_process);
 
-STATIC managed_proxy_t *managed_proxy_create(const smartlist_t *transport_list,
-                                             char **proxy_argv, int is_server);
+STATIC managed_proxy_t *managed_proxy_create(const char *name,
+                                             char **proxy_argv,
+                                             int is_server);
 
 STATIC int configure_proxy(managed_proxy_t *mp);
 

diff --git a/src/test/test_config.c b/src/test/test_config.c
@@ -627,8 +627,8 @@ get_total_system_memory_mock(size_t *mem_out)
 
 /* Mocks needed for the transport plugin line test */
 
-static void pt_kickstart_proxy_mock(const smartlist_t *transport_list,
-                                    char **proxy_argv, int is_server);
+static void pt_kickstart_proxy_mock(const char *name, char **proxy_argv,
+                                    int is_server);
 static int transport_add_from_config_mock(const tor_addr_t *addr,
                                           uint16_t port, const char *name,
                                           int socks_ver);
@@ -640,12 +640,11 @@ static int transport_is_needed_mock_call_count = 0;
 static int transport_is_needed_mock_return = 0;
 
 static void
-pt_kickstart_proxy_mock(const smartlist_t *transport_list,
-                        char **proxy_argv, int is_server)
+pt_kickstart_proxy_mock(const char *name, char **proxy_argv, int is_server)
 {
-  (void) transport_list;
   (void) proxy_argv;
   (void) is_server;
+  (void) name;
   /* XXXX check that args are as expected. */
 
   ++pt_kickstart_proxy_mock_call_count;
@@ -759,13 +758,7 @@ test_config_parse_transport_plugin_line(void *arg)
       "transport_1 exec /usr/bin/fake-transport", 1, 0);
   tt_int_op(r, OP_EQ, 0);
   r = pt_parse_transport_line(options,
-   "transport_1 exec /usr/bin/fake-transport", 1, 1);
-  tt_int_op(r, OP_EQ, 0);
-  r = pt_parse_transport_line(options,
-      "transport_1,transport_2 exec /usr/bin/fake-transport", 1, 0);
-  tt_int_op(r, OP_EQ, 0);
-  r = pt_parse_transport_line(options,
-      "transport_1,transport_2 exec /usr/bin/fake-transport", 1, 1);
+      "transport_1 exec /usr/bin/fake-transport", 1, 1);
   tt_int_op(r, OP_EQ, 0);
   /* Bad transport identifiers */
   r = pt_parse_transport_line(options,
@@ -786,13 +779,6 @@ test_config_parse_transport_plugin_line(void *arg)
   r = pt_parse_transport_line(options,
       "transport_1 proxy 1.2.3.4:567", 1, 1);
   tt_int_op(r, OP_EQ, 0);
-  /* Multiple-transport error exit */
-  r = pt_parse_transport_line(options,
-      "transport_1,transport_2 socks5 1.2.3.4:567", 1, 0);
-  tt_int_op(r, OP_LT, 0);
-  r = pt_parse_transport_line(options,
-      "transport_1,transport_2 proxy 1.2.3.4:567", 1, 1);
-  tt_int_op(r, OP_LT, 0);
   /* No port error exit */
   r = pt_parse_transport_line(options,
       "transport_1 socks5 1.2.3.4", 1, 0);
@@ -862,7 +848,7 @@ test_config_parse_transport_plugin_line(void *arg)
   r = pt_parse_transport_line(options,
       "transport_1 exec /usr/bin/fake-transport", 0, 0);
   /* Should have succeeded */
-  tt_int_op(r, OP_EQ, 0);
+  tt_int_op(r, OP_EQ, -1);
   /* transport_is_needed() should have been called */
   tt_assert(transport_is_needed_mock_call_count ==
       old_transport_is_needed_mock_call_count + 1);