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.

2 files changed, 12 insertions, 6 deletions

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;