[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor/master] revert the or_connection and dir_connection flags



commit f590a303db344a18f4e77be803a615e259892d2d
Author: Roger Dingledine <arma@xxxxxxxxxxxxxx>
Date:   Thu Mar 24 18:53:33 2016 -0400

    revert the or_connection and dir_connection flags
    
    They incorrectly summarized what the function was planning to do,
    leading to wrong behavior like making an http request to an orport,
    or making a begindir request to a dirport.
    
    This change backs out some of the changes made in commit e72cbf7a, and
    most of the changes made in commit ba6509e9.
    
    This patch resolves bug 18625. There more changes I want to make
    after this one, for code clarity.
---
 src/or/directory.c | 59 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/src/or/directory.c b/src/or/directory.c
index d057dac..c14c11d 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -1062,28 +1062,31 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
   dir_connection_t *conn;
   const or_options_t *options = get_options();
   int socket_error = 0;
+  /* Should the connection be to a relay's OR port (and inside that we will
+   * send our directory request)? */
   const int use_begindir = directory_command_should_use_begindir(options,
                                      &or_addr_port->addr, or_addr_port->port,
                                      router_purpose, indirection);
-  /* Is it an anonymous connection? Be careful, it could be either an OR or
-   * directory connection. */
+  /* Will the connection go via a three-hop Tor circuit? Note that this
+   * is separate from whether it will use_begindir. */
   const int anonymized_connection = dirind_is_anon(indirection);
-  /* Is it a connection to our DirPort? */
-  const int dir_connection = (indirection == DIRIND_ANON_DIRPORT ||
-                              indirection == DIRIND_DIRECT_CONN);
-  /* It's an OR connection if we should use BEGIN_DIR or if it's an
-   * anonymized connection but obviously not a directory connection. */
-  const int or_connection = (use_begindir ||
-                             (anonymized_connection && !dir_connection));
 
+  /* What is the address we want to make the directory request to? If
+   * we're making a begindir request this is the ORPort of the relay
+   * we're contacting; if not a begindir request, this is its DirPort.
+   * Note that if anonymized_connection is true, we won't be initiating
+   * a connection directly to this address. */
   tor_addr_t addr;
-  tor_addr_copy(&addr, &(or_connection ? or_addr_port : dir_addr_port)->addr);
-  uint16_t port = (or_connection ? or_addr_port : dir_addr_port)->port;
+  tor_addr_copy(&addr, &(use_begindir ? or_addr_port : dir_addr_port)->addr);
+  uint16_t port = (use_begindir ? or_addr_port : dir_addr_port)->port;
+  /* XXX dir_port is redundant and we can get rid of it. Just use port. */
   uint16_t dir_port = dir_addr_port->port;
 
   log_debug(LD_DIR, "anonymized %d, use_begindir %d.",
             anonymized_connection, use_begindir);
 
+  /* XXX This clause is redundant with the "addr, port is viable" check below.
+   * We should remove it. */
   if (!dir_port && !use_begindir) {
     char ipaddr[TOR_ADDR_BUF_LEN];
     tor_addr_to_str(ipaddr, &addr, TOR_ADDR_BUF_LEN, 0);
@@ -1104,28 +1107,20 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
 
   /* ensure that we don't make direct connections when a SOCKS server is
    * configured. */
-  if (dir_connection && !options->HTTPProxy &&
+  if (!anonymized_connection && !use_begindir && !options->HTTPProxy &&
       (options->Socks4Proxy || options->Socks5Proxy)) {
     log_warn(LD_DIR, "Cannot connect to a directory server through a "
                      "SOCKS proxy!");
     return;
   }
 
-  if (or_connection && (!or_addr_port->port
-                        || tor_addr_is_null(&or_addr_port->addr))) {
+  /* Make sure that the destination addr and port we picked is viable. */
+  if (!port || tor_addr_is_null(&addr)) {
     static int logged_backtrace = 0;
-    log_warn(LD_DIR, "Cannot make an outgoing OR connection without an OR "
-             "port.");
-    if (!logged_backtrace) {
-      log_backtrace(LOG_INFO, LD_BUG, "Address came from");
-      logged_backtrace = 1;
-    }
-    return;
-  } else if (dir_connection && (!dir_addr_port->port
-                                || tor_addr_is_null(&dir_addr_port->addr))) {
-    static int logged_backtrace = 0;
-    log_warn(LD_DIR, "Cannot make an outgoing Dir connection without a Dir "
-             "port.");
+    log_warn(LD_DIR,
+             "Cannot make an outgoing %sconnection without %sPort.",
+             use_begindir ? "begindir " : "",
+             use_begindir ? "an OR" : "a Dir");
     if (!logged_backtrace) {
       log_backtrace(LOG_INFO, LD_BUG, "Address came from");
       logged_backtrace = 1;
@@ -1161,7 +1156,7 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
   if (rend_query)
     conn->rend_data = rend_data_dup(rend_query);
 
-  if (dir_connection && !anonymized_connection) {
+  if (!anonymized_connection && !use_begindir) {
     /* then we want to connect to dirport directly */
 
     if (options->HTTPProxy) {
@@ -1190,8 +1185,12 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
         /* writable indicates finish, readable indicates broken link,
            error indicates broken link in windowsland. */
     }
-  } else { /* we want to connect via a tor connection */
+  } else {
+    /* We will use a Tor circuit (maybe 1-hop, maybe 3-hop, maybe with
+     * begindir, maybe not with begindir) */
+
     entry_connection_t *linked_conn;
+
     /* Anonymized tunneled connections can never share a circuit.
      * One-hop directory connections can share circuits with each other
      * but nothing else. */
@@ -1213,6 +1212,8 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
                               conn->base_.address, conn->base_.port,
                               digest,
                               SESSION_GROUP_DIRCONN, iso_flags,
+                    /* XXX dirconn_direct is misleading below. we should use
+                     * !anonymized_connection, since that's what we mean. */
                               use_begindir, conn->dirconn_direct);
     if (!linked_conn) {
       log_warn(LD_NET,"Making tunnel to dirserver failed.");
@@ -1225,6 +1226,8 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
       connection_mark_for_close(TO_CONN(conn));
       return;
     }
+    /* XXX the below line is suspicious and uncommented. does it close all
+     * consensus fetches if we've already bootstrapped? investigate. */
     if (connection_dir_close_consensus_conn_if_extra(conn)) {
       return;
     }



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits