Fix various issues pointed out by Nick and Andrea.

- Document the key=value format.
- Constify equal_sign_pos.
- Pass some strings that are about to be logged to escape().
- Update documentation and fix some bugs in tor_escape_str_for_socks_arg().
- Use string_is_key_value() in parse_bridge_line().
- Parenthesize a forgotten #define
- Add some more comments.
- Add some more unit test cases.

4 files changed, 39 insertions, 20 deletions

diff --git a/src/common/util.c b/src/common/util.c
index b2f12bfb6a..9aba7d6c5d 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -865,30 +865,30 @@ tor_digest_is_zero(const char *digest)
   return tor_memeq(digest, ZERO_DIGEST, DIGEST_LEN);
 }
 
-/** Return true if <b>string</b> is a valid '<key>=<value>' string.
+/** Return true if <b>string</b> is a valid '<key>=[<value>]' string.
  *  <value> is optional, to indicate the empty string. */
 int
 string_is_key_value(const char *string)
 {
   /* position of equal sign in string */
-  char *equal_sign_pos = NULL;
+  const char *equal_sign_pos = NULL;
 
   tor_assert(string);
 
-  if (strlen(string) < 2) { /* "x=a" is shortest args string */
-    log_warn(LD_GENERAL, "'%s' is too short to be a k=v value.", string);
+  if (strlen(string) < 2) { /* "x=" is shortest args string */
+    log_warn(LD_GENERAL, "'%s' is too short to be a k=v value.", escaped(string));
     return 0;
   }
 
   equal_sign_pos = strchr(string, '=');
   if (!equal_sign_pos) {
-    log_warn(LD_GENERAL, "'%s' is not a k=v value.", string);
+    log_warn(LD_GENERAL, "'%s' is not a k=v value.", escaped(string));
     return 0;
   }
 
   /* validate that the '=' is not in the beginning of the string. */
   if (equal_sign_pos == string) {
-    log_warn(LD_GENERAL, "'%s' is not a valid k=v value.", string);
+    log_warn(LD_GENERAL, "'%s' is not a valid k=v value.", escaped(string));
     return 0;
   }
 
@@ -1279,9 +1279,10 @@ wrap_string(smartlist_t *out, const char *string, size_t width,
   }
 }
 
-/** Escape every character of <b>string</b> that belongs to the set of
- *  characters <b>set</b>. Use <b>escape_char</b> as the character to
- *  use for escaping. */
+/** Escape every ";" or "\" character of <b>string</b>. Use
+ *  <b>escape_char</b> as the character to use for escaping.
+ *  The returned string is allocated on the heap and it's the
+ *  responsibility of the caller to free it. */
 char *
 tor_escape_str_for_socks_arg(const char *string)
 {
@@ -1294,8 +1295,8 @@ tor_escape_str_for_socks_arg(const char *string)
 
   length = strlen(string);
 
-  if (!length)
-    return NULL;
+  if (!length) /* If we were given the empty string, return the same. */
+    return tor_strdup("");
   /* (new_length > SIZE_MAX) => ((length * 2) + 1 > SIZE_MAX) =>
      (length*2 > SIZE_MAX - 1) => (length > (SIZE_MAX - 1)/2) */
   if (length > (SIZE_MAX - 1)/2) /* check for overflow */
@@ -1304,7 +1305,7 @@ tor_escape_str_for_socks_arg(const char *string)
   /* this should be enough even if all characters must be escaped */
   new_length = (length * 2) + 1;
 
-  new_string = new_cp = tor_malloc_zero(new_length);
+  new_string = new_cp = tor_malloc(new_length);
 
   while (*string) {
     if (strchr(chars_to_escape, *string))
@@ -1313,6 +1314,8 @@ tor_escape_str_for_socks_arg(const char *string)
     *new_cp++ = *string++;
   }
 
+  *new_cp = '\0'; /* NUL-terminate the new string */
+
   return new_string;
 }
 
diff --git a/src/or/config.c b/src/or/config.c
index d057dd8ae3..a09dda996b 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -2875,14 +2875,14 @@ options_validate(or_options_t *old_options, or_options_t *options,
     size_t len;
 
     len = strlen(options->Socks5ProxyUsername);
-    if (len < 1 || len > 255)
+    if (len < 1 || len > MAX_SOCKS5_AUTH_FIELD_SIZE)
       REJECT("Socks5ProxyUsername must be between 1 and 255 characters.");
 
     if (!options->Socks5ProxyPassword)
       REJECT("Socks5ProxyPassword must be included with Socks5ProxyUsername.");
 
     len = strlen(options->Socks5ProxyPassword);
-    if (len < 1 || len > 255)
+    if (len < 1 || len > MAX_SOCKS5_AUTH_FIELD_SIZE)
       REJECT("Socks5ProxyPassword must be between 1 and 255 characters.");
   } else if (options->Socks5ProxyPassword)
     REJECT("Socks5ProxyPassword must be included with Socks5ProxyUsername.");
@@ -4120,11 +4120,12 @@ parse_bridge_line(const char *line, int validate_only)
       field = smartlist_get(items, 0);
       smartlist_del_keeporder(items, 0);
 
-      /* If '=', it's a k=v value pair. */
-      if (strchr(field, '=')) {
+      /* If it's a key=value pair, then it's a SOCKS argument for the
+         transport proxy... */
+      if (string_is_key_value(field)) {
         socks_args = smartlist_new();
         smartlist_add(socks_args, field);
-      } else { /* If no '=', it's the fingerprint. */
+      } else { /* ...otherwise, it's the bridge fingerprint. */
         fingerprint = field;
       }
 
diff --git a/src/or/connection.c b/src/or/connection.c
index 6bac59b20c..b0fbe520b2 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -1585,7 +1585,7 @@ get_proxy_type(void)
 /* One byte for the version, one for the command, two for the
    port, and four for the addr... and, one more for the
    username NUL: */
-#define SOCKS4_STANDARD_BUFFER_SIZE 1 + 1 + 2 + 4 + 1
+#define SOCKS4_STANDARD_BUFFER_SIZE (1 + 1 + 2 + 4 + 1)
 
 /** Write a proxy request of <b>type</b> (socks4, socks5, https) to conn
  * for conn->addr:conn->port, authenticating with the auth details given
@@ -1688,6 +1688,9 @@ connection_proxy_connect(connection_t *conn, int type)
       memcpy(buf + 2, &portn, 2); /* port */
       memcpy(buf + 4, &ip4addr, 4); /* addr */
 
+      /* Next packet field is the userid. If we have pluggable
+         transport SOCKS arguments, we have to embed them
+         there. Otherwise, we use an empty userid.  */
       if (socks_args_string) { /* place the SOCKS args string: */
         tor_assert(strlen(socks_args_string) > 0);
         tor_assert(buf_size >=
@@ -1951,6 +1954,8 @@ connection_read_proxy_handshake(connection_t *conn)
           break;
         }
 
+        /* Username and password lengths should have been checked
+           above and during torrc parsing. */
         tor_assert(usize <= MAX_SOCKS5_AUTH_FIELD_SIZE &&
                    psize <= MAX_SOCKS5_AUTH_FIELD_SIZE);
         reqsize = 3 + usize + psize;
diff --git a/src/test/test_util.c b/src/test/test_util.c
index b41f23571b..a307a79c80 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -813,9 +813,11 @@ test_util_escape_string_socks(void)
   test_streq(escaped_string, "First rule: Do not use \\;");
   tor_free(escaped_string);
 
-  /** Ilegal: Empty string. */
+  /** Empty string. */
   escaped_string = tor_escape_str_for_socks_arg("");
-  test_assert(!escaped_string);
+  test_assert(escaped_string);
+  test_streq(escaped_string, "");
+  tor_free(escaped_string);
 
   /** Escape all characters. */
   escaped_string = tor_escape_str_for_socks_arg(";\\;\\");
@@ -823,6 +825,11 @@ test_util_escape_string_socks(void)
   test_streq(escaped_string, "\\;\\\\\\;\\\\");
   tor_free(escaped_string);
 
+  escaped_string = tor_escape_str_for_socks_arg(";");
+  test_assert(escaped_string);
+  test_streq(escaped_string, "\\;");
+  tor_free(escaped_string);
+
  done:
   tor_free(escaped_string);
 }
@@ -834,7 +841,10 @@ test_util_string_is_key_value(void *ptr)
   test_assert(string_is_key_value("key=value"));
   test_assert(string_is_key_value("k=v"));
   test_assert(string_is_key_value("key="));
+  test_assert(string_is_key_value("x="));
+  test_assert(string_is_key_value("xx="));
   test_assert(!string_is_key_value("=value"));
+  test_assert(!string_is_key_value("=x"));
   test_assert(!string_is_key_value("="));
 
   /* ??? */