Remove duplicate code between parse_{c,s}method in transport.c

- Merged common code in function parse_{c,s}method to a single function
- Removed duplicate code in transport.c
- Fixes #6236

changes/ticket6236

@@ -0,0 +1,4 @@
+  o Minor bugfixes (Duplicate code):
+    - Remove duplicate code in parse_{c,s}method_line and bootstrap
+      their functionalities into a single function. Fixes
+      bug 6236; bugfix on 0.2.3.6-alpha.

src/or/transports.c

@@ -1025,48 +1025,71 @@ parse_method_error(const char *line, int is_server)
            line+strlen(error)+1);
 }
 
-/** Parses an SMETHOD <b>line</b> and if well-formed it registers the
- *  new transport in <b>mp</b>. */
-STATIC int
-parse_smethod_line(const char *line, managed_proxy_t *mp)
+/** A helper for parse_{c,s}method_line(), bootstraps its
+ *  functionalities. If <b>is_smethod</b> is true then the
+ *  the line to parse is a SMETHOD line otherwise it is a
+ *  CMETHOD line*/
+static int
+parse_method_line_helper(const char *line,
+                         managed_proxy_t *mp,
+                         int is_smethod)
 {
+  int item_index = 0;
   int r;
-  smartlist_t *items = NULL;
 
-  char *method_name=NULL;
+  char *transport_name=NULL;
   char *args_string=NULL;
   char *addrport=NULL;
-  tor_addr_t tor_addr;
+  int socks_ver=PROXY_NONE;
   char *address=NULL;
   uint16_t port = 0;
 
+  const char *method_str = is_smethod ? PROTO_SMETHOD : PROTO_CMETHOD;
+  const int min_args_count = is_smethod ? 3 : 4;
+
+  tor_addr_t tor_addr;
   transport_t *transport=NULL;
+  smartlist_t *items= smartlist_new();
 
-  items = smartlist_new();
   smartlist_split_string(items, line, NULL,
                          SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1);
-  if (smartlist_len(items) < 3) {
-    log_warn(LD_CONFIG, "Server managed proxy sent us a SMETHOD line "
-             "with too few arguments.");
+  if (smartlist_len(items) < min_args_count) {
+    log_warn(LD_CONFIG, "Managed proxy sent us a %s line "
+             "with too few arguments.", method_str);
     goto err;
   }
 
-  /* Example of legit SMETHOD line:
-     SMETHOD obfs2 0.0.0.0:25612 ARGS:secret=supersekrit,key=superkey */
-
-  tor_assert(!strcmp(smartlist_get(items,0),PROTO_SMETHOD));
+  tor_assert(!strcmp(smartlist_get(items, item_index),method_str));
+  ++item_index;
 
-  method_name = smartlist_get(items,1);
-  if (!string_is_C_identifier(method_name)) {
+  transport_name = smartlist_get(items,item_index);
+  ++item_index;
+  if (!string_is_C_identifier(transport_name)) {
     log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).",
-             method_name);
+             transport_name);
     goto err;
   }
 
-  addrport = smartlist_get(items, 2);
+  /** Check for the proxy method sent to us in CMETHOD line. */
+  if (!is_smethod) {
+    const char *socks_ver_str = smartlist_get(items,item_index);
+    ++item_index;
+
+    if (!strcmp(socks_ver_str,"socks4")) {
+      socks_ver = PROXY_SOCKS4;
+    } else if (!strcmp(socks_ver_str,"socks5")) {
+      socks_ver = PROXY_SOCKS5;
+    } else {
+      log_warn(LD_CONFIG, "Client managed proxy sent us a proxy protocol "
+               "we don't recognize. (%s)", socks_ver_str);
+      goto err;
+    }
+  }
+
+  addrport = smartlist_get(items, item_index);
+  ++item_index;
   if (tor_addr_port_split(LOG_WARN, addrport, &address, &port)<0) {
-    log_warn(LD_CONFIG, "Error parsing transport "
-             "address '%s'", addrport);
+    log_warn(LD_CONFIG, "Error parsing transport address '%s'", addrport);
     goto err;
   }
 
@@ -1081,10 +1104,11 @@ parse_smethod_line(const char *line, managed_proxy_t *mp)
     goto err;
   }
 
-  if (smartlist_len(items) > 3) {
+  /** Check for options in the SMETHOD line. */
+  if (is_smethod && smartlist_len(items) > min_args_count) {
     /* Seems like there are also some [options] in the SMETHOD line.
        Let's see if we can parse them. */
-    char *options_string = smartlist_get(items, 3);
+    char *options_string = smartlist_get(items, item_index);
     log_debug(LD_CONFIG, "Got options_string: %s", options_string);
     if (!strcmpstart(options_string, "ARGS:")) {
       args_string = options_string+strlen("ARGS:");
@@ -1092,15 +1116,20 @@ parse_smethod_line(const char *line, managed_proxy_t *mp)
     }
   }
 
-  transport = transport_new(&tor_addr, port, method_name,
-                            PROXY_NONE, args_string);
+  transport = transport_new(&tor_addr, port, transport_name,
+                            socks_ver, args_string);
 
   smartlist_add(mp->transports, transport);
 
-  /* For now, notify the user so that they know where the server
-     transport is listening. */
-  log_info(LD_CONFIG, "Server transport %s at %s:%d.",
-           method_name, address, (int)port);
+  /** Logs info about line parsing success for client or server */
+  if (is_smethod) {
+    log_info(LD_CONFIG, "Server transport %s at %s:%d.",
+             transport_name, address, (int)port);
+  } else {
+    log_info(LD_CONFIG, "Transport %s at %s:%d with SOCKS %d. "
+             "Attached to managed proxy.",
+             transport_name, address, (int)port, socks_ver);
+  }
 
   r=0;
   goto done;
@@ -1115,93 +1144,24 @@ parse_smethod_line(const char *line, managed_proxy_t *mp)
   return r;
 }
 
+/** Parses an SMETHOD <b>line</b> and if well-formed it registers the
+ *  new transport in <b>mp</b>. */
+STATIC int
+parse_smethod_line(const char *line, managed_proxy_t *mp)
+{
+  /* Example of legit SMETHOD line:
+     SMETHOD obfs2 0.0.0.0:25612 ARGS:secret=supersekrit,key=superkey */
+  return parse_method_line_helper(line, mp, 1);
+}
+
 /** Parses a CMETHOD <b>line</b>, and if well-formed it registers
  *  the new transport in <b>mp</b>. */
 STATIC int
 parse_cmethod_line(const char *line, managed_proxy_t *mp)
 {
-  int r;
-  smartlist_t *items = NULL;
-
-  char *method_name=NULL;
-
-  char *socks_ver_str=NULL;
-  int socks_ver=PROXY_NONE;
-
-  char *addrport=NULL;
-  tor_addr_t tor_addr;
-  char *address=NULL;
-  uint16_t port = 0;
-
-  transport_t *transport=NULL;
-
-  items = smartlist_new();
-  smartlist_split_string(items, line, NULL,
-                         SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1);
-  if (smartlist_len(items) < 4) {
-    log_warn(LD_CONFIG, "Client managed proxy sent us a CMETHOD line "
-             "with too few arguments.");
-    goto err;
-  }
-
-  tor_assert(!strcmp(smartlist_get(items,0),PROTO_CMETHOD));
-
-  method_name = smartlist_get(items,1);
-  if (!string_is_C_identifier(method_name)) {
-    log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).",
-             method_name);
-    goto err;
-  }
-
-  socks_ver_str = smartlist_get(items,2);
-
-  if (!strcmp(socks_ver_str,"socks4")) {
-    socks_ver = PROXY_SOCKS4;
-  } else if (!strcmp(socks_ver_str,"socks5")) {
-    socks_ver = PROXY_SOCKS5;
-  } else {
-    log_warn(LD_CONFIG, "Client managed proxy sent us a proxy protocol "
-             "we don't recognize. (%s)", socks_ver_str);
-    goto err;
-  }
-
-  addrport = smartlist_get(items, 3);
-  if (tor_addr_port_split(LOG_WARN, addrport, &address, &port)<0) {
-    log_warn(LD_CONFIG, "Error parsing transport "
-             "address '%s'", addrport);
-    goto err;
-  }
-
-  if (!port) {
-    log_warn(LD_CONFIG,
-             "Transport address '%s' has no port.", addrport);
-    goto err;
-  }
-
-  if (tor_addr_parse(&tor_addr, address) < 0) {
-    log_warn(LD_CONFIG, "Error parsing transport address '%s'", address);
-    goto err;
-  }
-
-  transport = transport_new(&tor_addr, port, method_name, socks_ver, NULL);
-
-  smartlist_add(mp->transports, transport);
-
-  log_info(LD_CONFIG, "Transport %s at %s:%d with SOCKS %d. "
-           "Attached to managed proxy.",
-           method_name, address, (int)port, socks_ver);
-
-  r=0;
-  goto done;
-
- err:
-  r = -1;
-
- done:
-  SMARTLIST_FOREACH(items, char*, s, tor_free(s));
-  smartlist_free(items);
-  tor_free(address);
-  return r;
+  /* Example of legit CMETHOD line:
+     CMETHOD obfs2 socks5 127.0.0.1:35713 */
+  return parse_method_line_helper(line, mp, 0);
 }
 
 /** Parses an PROXY-ERROR <b>line</b> and warns the user accordingly. */