Comment the heck out of the stream/circuit attaching process.

1 files changed, 76 insertions, 33 deletions

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 1ee0c0f5cd..6b68a19836 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -830,7 +830,8 @@ connection_ap_rescan_and_attach_pending(void)
 #endif
 
 /** Tell any AP streams that are listed as waiting for a new circuit to try
- * again, either attaching to an available circ or launching a new one.
+ * again.  If there is an available circuit for a stream, attach it. Otherwise,
+ * launch a new circuit.
  *
  * If <b>retry</b> is false, only check the list if it contains at least one
  * streams that we have not yet tried to attach to a circuit.
@@ -845,8 +846,9 @@ connection_ap_attach_pending(int retry)
   if (untried_pending_connections == 0 && !retry)
     return;
 
-  /* Don't allow modifications to pending_entry_connections while we are
-   * iterating over it. */
+  /* Don't allow any modifications to list while we are iterating over
+   * it.  We'll put streams back on this list if we can't attach them
+   * immediately. */
   smartlist_t *pending = pending_entry_connections;
   pending_entry_connections = smartlist_new();
 
@@ -873,6 +875,7 @@ connection_ap_attach_pending(int retry)
       continue;
     }
 
+    /* Okay, we're through the sanity checks. Try to handle this stream. */
     if (connection_ap_handshake_attach_circuit(entry_conn) < 0) {
       if (!conn->marked_for_close)
         connection_mark_unattached_ap(entry_conn,
@@ -882,12 +885,17 @@ connection_ap_attach_pending(int retry)
     if (! conn->marked_for_close &&
         conn->type == CONN_TYPE_AP &&
         conn->state == AP_CONN_STATE_CIRCUIT_WAIT) {
+      /* Is it still waiting for a circuit? If so, we didn't attach it,
+       * so it's still pending.  Put it back on the list.
+       */
       if (!smartlist_contains(pending_entry_connections, entry_conn)) {
         smartlist_add(pending_entry_connections, entry_conn);
         continue;
       }
     }
 
+    /* If we got here, then we either closed the connection, or
+     * we attached it. */
     UNMARK();
   } SMARTLIST_FOREACH_END(entry_conn);
 
@@ -1186,6 +1194,8 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
 
   /* Remember the original address so we can tell the user about what
    * they actually said, not just what it turned into. */
+  /* XXX yes, this is the same as out->orig_address above. One is
+   * in the output, and one is in the connection. */
   if (! conn->original_dest_address) {
     /* Is the 'if' necessary here? XXXX */
     conn->original_dest_address = tor_strdup(conn->socks_request->address);
@@ -1193,7 +1203,7 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
 
   /* First, apply MapAddress and MAPADDRESS mappings. We need to do
    * these only for non-reverse lookups, since they don't exist for those.
-   * We need to do this before we consider automapping, since we might
+   * We also need to do this before we consider automapping, since we might
    * e.g. resolve irc.oftc.net into irconionaddress.onion, at which point
    * we'd need to automap it. */
   if (socks->command != SOCKS_COMMAND_RESOLVE_PTR) {
@@ -1205,9 +1215,12 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
     }
   }
 
-  /* Now, handle automapping.  Automapping happens when we're asked to
-   * resolve a hostname, and AutomapHostsOnResolve is set, and
-   * the hostname has a suffix listed in AutomapHostsSuffixes.
+  /* Now see if we need to create or return an existing Hostname->IP
+   * automapping.  Automapping happens when we're asked to resolve a
+   * hostname, and AutomapHostsOnResolve is set, and the hostname has a
+   * suffix listed in AutomapHostsSuffixes.  It's a handy feature
+   * that lets you have Tor assign e.g. IPv6 addresses for .onion
+   * names, and return them safely from DNSPort.
    */
   if (socks->command == SOCKS_COMMAND_RESOLVE &&
       tor_addr_parse(&addr_tmp, socks->address)<0 &&
@@ -1247,7 +1260,8 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
   }
 
   /* Now handle reverse lookups, if they're in the cache.  This doesn't
-   * happen too often, since client-side DNS caching is off by default. */
+   * happen too often, since client-side DNS caching is off by default,
+   * and very deprecated. */
   if (socks->command == SOCKS_COMMAND_RESOLVE_PTR) {
     unsigned rewrite_flags = 0;
     if (conn->entry_cfg.use_cached_ipv4_answers)
@@ -1292,11 +1306,12 @@ connection_ap_handshake_rewrite(entry_connection_t *conn,
     }
   }
 
-  /* If we didn't automap it before, then this is still the address
-   * that came straight from the user, mapped according to any
-   * MapAddress/MAPADDRESS commands.  Now other mappings, including
-   * previously registered Automap entries, TrackHostExits entries,
-   * and client-side DNS cache entries (not recommended).
+  /* If we didn't automap it before, then this is still the address that
+   * came straight from the user, mapped according to any
+   * MapAddress/MAPADDRESS commands.  Now apply other mappings,
+   * including previously registered Automap entries (IP back to
+   * hostname), TrackHostExits entries, and client-side DNS cache
+   * entries (if they're turned on).
    */
   if (socks->command != SOCKS_COMMAND_RESOLVE_PTR &&
       !out->automap) {
@@ -1361,11 +1376,14 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   time_t now = time(NULL);
   rewrite_result_t rr;
 
+  /* First we'll do the rewrite part.  Let's see if we get a reasonable
+   * answer.
+   */
   memset(&rr, 0, sizeof(rr));
   connection_ap_handshake_rewrite(conn,&rr);
 
   if (rr.should_close) {
-    /* connection_ap_handshake_rewrite told us to close the connection,
+    /* connection_ap_handshake_rewrite told us to close the connection:
      * either because it sent back an answer, or because it sent back an
      * error */
     connection_mark_unattached_ap(conn, rr.end_reason);
@@ -1379,8 +1397,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   const int automap = rr.automap;
   const addressmap_entry_source_t exit_source = rr.exit_source;
 
-  /* Parse the address provided by SOCKS.  Modify it in-place if it
-   * specifies a hidden-service (.onion) or particular exit node (.exit).
+  /* Now, we parse the address to see if it's an .onion or .exit or
+   * other special address.
    */
   const hostname_type_t addresstype = parse_extended_hostname(socks->address);
 
@@ -1394,8 +1412,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   }
 
   /* If this is a .exit hostname, strip off the .name.exit part, and
-   * see whether we're going to connect there, and otherwise handle it.
-   * (The ".exit" part got stripped off by "parse_extended_hostname").
+   * see whether we're willing to connect there, and and otherwise handle the
+   * .exit address.
    *
    * We'll set chosen_exit_name and/or close the connection as appropriate.
    */
@@ -1407,7 +1425,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     const node_t *node = NULL;
 
     /* If this .exit was added by an AUTOMAP, then it came straight from
-     * a user.  Make sure that options->AllowDotExit permits that. */
+     * a user.  Make sure that options->AllowDotExit permits that! */
     if (exit_source == ADDRMAPSRC_AUTOMAP && !options->AllowDotExit) {
       /* Whoops; this one is stale.  It must have gotten added earlier,
        * when AllowDotExit was on. */
@@ -1436,7 +1454,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     }
 
     tor_assert(!automap);
-    /* Now, find the character before the .(name) part. */
+
+    /* Now, find the character before the .(name) part.
+     * (The ".exit" part got stripped off by "parse_extended_hostname").
+     *
+     * We're going to put the exit name into conn->chosen_exit_name, and
+     * look up a node correspondingly. */
     char *s = strrchr(socks->address,'.');
     if (s) {
       /* The address was of the form "(stuff).(name).exit */
@@ -1492,10 +1515,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
        implies no. */
   }
 
-  /* Now, handle everything that isn't a .onion address. */
+  /* Now, we handle everything that isn't a .onion address. */
   if (addresstype != ONION_HOSTNAME) {
     /* Not a hidden-service request.  It's either a hostname or an IP,
-     * possibly with a .exit that we stripped off. */
+     * possibly with a .exit that we stripped off.  We're going to check
+     * if we're allowed to connect/resolve there, and then launch the
+     * appropriate request. */
 
     /* Check for funny characters in the address. */
     if (address_is_invalid_destination(socks->address, 1)) {
@@ -1542,7 +1567,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     }
 
     /* Then check if we have a hostname or IP address, and whether DNS or
-     * the IP address family are permitted */
+     * the IP address family are permitted.  Reject if not. */
     tor_addr_t dummy_addr;
     int socks_family = tor_addr_parse(&dummy_addr, socks->address);
     /* family will be -1 for a non-onion hostname that's not an IP */
@@ -1564,8 +1589,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
                safe_str_client(socks->address));
       connection_mark_unattached_ap(conn, END_STREAM_REASON_ENTRYPOLICY);
       return -1;
+    } else {
+      tor_assert_nonfatal_unreached_once();
     }
-    /* No else, we've covered all possible returned value. */
 
     /* See if this is a hostname lookup that we can answer immediately.
      * (For example, an attempt to look up the IP address for an IP address.)
@@ -1585,7 +1611,10 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       }
       tor_assert(!automap);
       rep_hist_note_used_resolve(now); /* help predict this next time */
-    } else if (socks->command == SOCKS_COMMAND_CONNECT) {
+    }
+
+    /* Now see if this is a connect request that we can reject immediately */
+    if (socks->command == SOCKS_COMMAND_CONNECT) {
       /* Special handling for attempts to connect */
       tor_assert(!automap);
       /* Don't allow connections to port 0. */
@@ -1639,7 +1668,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       } /* end "if we should check for internal addresses" */
 
       /* Okay.  We're still doing a CONNECT, and it wasn't a private
-       * address.  Do special handling for literal IP addresses */
+       * address.  Here we do special handling for literal IP addresses,
+       * to see if we should reject this preemptively, and to set up
+       * fields in conn->entry_cfg to tell the exit what AF we want. */
       {
         tor_addr_t addr;
         /* XXX Duplicate call to tor_addr_parse. */
@@ -1682,11 +1713,15 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
         }
       }
 
+      /* we never allow IPv6 answers on socks4. (TODO: Is this smart?) */
       if (socks->socks_version == 4)
         conn->entry_cfg.ipv6_traffic = 0;
 
       /* Still handling CONNECT. Now, check for exit enclaves.  (Which we
-       * don't do on BEGINDIR, or there is a chosen exit.)
+       * don't do on BEGINDIR, or when there is a chosen exit.)
+       *
+       * TODO: Should we remove this?  Exit enclaves are nutty and don't
+       * work very well
        */
       if (!conn->use_begindir && !conn->chosen_exit_name && !circ) {
         /* see if we can find a suitable enclave exit */
@@ -1710,7 +1745,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
         if (consider_plaintext_ports(conn, socks->port) < 0)
           return -1;
 
-      /* Remember the port so that we do predicted requests there. */
+      /* Remember the port so that we will predict that more requests
+         there will happen in the future. */
       if (!conn->use_begindir) {
         /* help predict this next time */
         rep_hist_note_used_port(now, socks->port);
@@ -1735,6 +1771,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     if (circ) {
       rv = connection_ap_handshake_attach_chosen_circuit(conn, circ, cpath);
     } else {
+      /* We'll try to attach it at the next event loop, or whenever
+       * we call connection_ap_attach_pending() */
       connection_ap_mark_as_pending_circuit(conn);
       rv = 0;
     }
@@ -1811,8 +1849,8 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     log_info(LD_REND,"Got a hidden service request for ID '%s'",
              safe_str_client(rend_data->onion_address));
 
-    /* Lookup the given onion address. If invalid, stop right now else we
-     * might have it in the cache or not, it will be tested later on. */
+    /* Lookup the given onion address. If invalid, stop right now.
+     * Otherwise, we might have it in the cache or not. */
     unsigned int refetch_desc = 0;
     rend_cache_entry_t *entry = NULL;
     const int rend_cache_lookup_result =
@@ -1826,6 +1864,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
         connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
         return -1;
       case ENOENT:
+        /* We didn't have this; we should look it up. */
         refetch_desc = 1;
         break;
       default:
@@ -1835,8 +1874,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       }
     }
 
-    /* Help predict this next time. We're not sure if it will need
-     * a stable circuit yet, but we know we'll need *something*. */
+    /* Help predict that we'll want to do hidden service circuits in the
+     * future. We're not sure if it will need a stable circuit yet, but
+     * we know we'll need *something*. */
     rep_hist_note_used_internal(now, 0, 1);
 
     /* Now we have a descriptor but is it usable or not? If not, refetch.
@@ -1851,9 +1891,12 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       return 0;
     }
 
-    /* We have the descriptor so launch a connection to the HS. */
+    /* We have the descriptor!  So launch a connection to the HS. */
     base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
     log_info(LD_REND, "Descriptor is here. Great.");
+
+    /* We'll try to attach it at the next event loop, or whenever
+     * we call connection_ap_attach_pending() */
     connection_ap_mark_as_pending_circuit(conn);
     return 0;
   }