Fix a number of issues with the #5040 code.

- Don't leak if a transport proxy sends us a TRANSPORT command more
  than once.

- Don't use smartlist_string_isin() in geoip_get_transport_history().
  (pointed out by Nick)

- Use the 'join' argument of smartlist_join_strings() instead of
  trying to write the separator on our own.
  (pointed out by Nick)

- Document 'ext_or_transport' a bit better.
  (pointed out by Nick)

- Be a bit more consistent with the types of the values of 'transport_counts'.
  (pointed out by Nick)

3 files changed, 14 insertions, 10 deletions

diff --git a/src/or/ext_orport.c b/src/or/ext_orport.c
index f83002c18..8fd9b77c8 100644
--- a/src/or/ext_orport.c
+++ b/src/or/ext_orport.c
@@ -460,6 +460,12 @@ connection_ext_or_handle_cmd_transport(or_connection_t *conn,
     return -1;
   }
 
+  /* If ext_or_transport is already occupied (because the PT sent two
+   *  TRANSPORT commands), deallocate the old name and keep the new
+   *  one */
+  if (conn->ext_or_transport)
+    tor_free(conn->ext_or_transport);
+
   conn->ext_or_transport = transport_str;
   return 0;
 }
diff --git a/src/or/geoip.c b/src/or/geoip.c
index 7244c5617..737512f62 100644
--- a/src/or/geoip.c
+++ b/src/or/geoip.c
@@ -807,7 +807,6 @@ geoip_get_transport_history(void)
   const char *transport_name = NULL;
   smartlist_t *string_chunks = smartlist_new();
   char *the_string = NULL;
-  int i = 0;
 
   /* If we haven't seen any clients yet, return NULL. */
   if (HT_EMPTY(&client_history))
@@ -841,7 +840,7 @@ geoip_get_transport_history(void)
     strmap_set(transport_counts, transport_name, ptr);
 
     /* If it's the first time we see this transport, note it. */
-    if (!smartlist_contains_string(transports_used, transport_name))
+    if (val == 1)
       smartlist_add(transports_used, tor_strdup(transport_name));
 
     log_debug(LD_GENERAL, "Client from '%s' with transport '%s'. "
@@ -857,20 +856,18 @@ geoip_get_transport_history(void)
   /* Loop through all seen transports. */
   SMARTLIST_FOREACH_BEGIN(transports_used, const char *, transport_name) {
     void *transport_count_ptr = strmap_get(transport_counts, transport_name);
-    unsigned int transport_count = (uintptr_t) transport_count_ptr;
-    i++; /* counter so that we don't add a comma if it's the last transport. */
+    unsigned int transport_count = (intptr_t) transport_count_ptr;
 
     log_debug(LD_GENERAL, "We got %u clients with transport '%s'.",
               transport_count, transport_name);
 
-    smartlist_add_asprintf(string_chunks, "%s=%u%s",
+    smartlist_add_asprintf(string_chunks, "%s=%u",
                            transport_name,
                            round_to_next_multiple_of(transport_count,
-                                                     granularity),
-                           i != smartlist_len(transports_used) ? "," : "");
+                                                     granularity));
   } SMARTLIST_FOREACH_END(transport_name);
 
-  the_string = smartlist_join_strings(string_chunks, "", 0, NULL);
+  the_string = smartlist_join_strings(string_chunks, ",", 0, NULL);
 
   log_debug(LD_GENERAL, "Final bridge-ip-transports string: '%s'", the_string);
 
diff --git a/src/or/or.h b/src/or/or.h
index 7916c476a..9b519a78f 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -1452,8 +1452,9 @@ typedef struct or_connection_t {
   char *ext_or_conn_id;
   /** Client hash of the Extended ORPort authentication scheme */
   char *ext_or_auth_correct_client_hash;
-  /** Name of the pluggable transport that is obfuscating this
-      connection. If no pluggable transports are used, it's NULL. */
+  /** String carrying the name of the pluggable transport
+   *  (e.g. "obfs2") that is obfuscating this connection. If no
+   *  pluggable transports are used, it's NULL. */
   char *ext_or_transport;
 
   char *nickname; /**< Nickname of OR on other side (if any). */