[tor-commits] [tor/master] Improve docs for addr, address, and real_addr fields

[nickm at torproject.org]

commit 02cff32d7947cd230b1dab299b29dec5315ea4b9
Author: Nick Mathewson <nickm at torproject.org>
Date:   Mon Jul 13 12:13:41 2020 -0400

    Improve docs for addr, address, and real_addr fields
    
    These fields have a complicated history, some slightly complicated
    behavior, and some definitely inadequate documentation.  Before we
    go fixing them up, let's document how they work now.
---
 src/core/or/connection_st.h    | 52 +++++++++++++++++++++++++++++++++++++-----
 src/core/or/or_connection_st.h | 16 +++++++++----
 2 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/src/core/or/connection_st.h b/src/core/or/connection_st.h
index 55d94d9451..508328f75c 100644
--- a/src/core/or/connection_st.h
+++ b/src/core/or/connection_st.h
@@ -109,6 +109,39 @@ struct connection_t {
 
   int socket_family; /**< Address family of this connection's socket.  Usually
                       * AF_INET, but it can also be AF_UNIX, or AF_INET6 */
+  /**
+   * IP address on the internet of this connection's peer, usually.
+   *
+   * This address may come from several sources.  If this is an outbound
+   * connection, it is the address we are trying to connect to--either
+   * directly through `s`, or via a proxy.  (If we used a proxy, then
+   * `getpeername(s)` will not give this address.)
+   *
+   * For incoming connections, this field is the address we got from
+   * getpeername() or accept(), as updated by any proxy that we
+   * are using (for example, an ExtORPort proxy).
+   *
+   * For listeners, this is the address we are trying to bind to.
+   *
+   * If this connection is using a unix socket, then this address is a null
+   * address, and the real address is in the `address` field.
+   *
+   * If this connection represents a request made somewhere other than via
+   * TCP (for example, a UDP dns request, or a controller resolve request),
+   * then this address is the address that originated the request.
+   *
+   * TECHNICAL DEBT:
+   *
+   * There are a few places in the code that modify this address,
+   * or use it in other ways that we don't currently like.  Please don't add
+   * any more!
+   *
+   * The misuses of this field include:
+   *    * Setting it to the canonical address of a relay on an OR connection.
+   *    * Setting it on linked connections, possibly.
+   *    * Updating it based on the Forwarded-For header-- Forwarded-For is
+   *      set by a proxy, but not a local trusted troxy.
+   **/
   tor_addr_t addr; /**< IP that socket "s" is directly connected to;
                     * may be the IP address for a proxy or pluggable transport,
                     * see "address" for the address of the final destination.
@@ -122,12 +155,19 @@ struct connection_t {
                               * marked.) */
   const char *marked_for_close_file; /**< For debugging: in which file were
                                       * we marked for close? */
-  char *address; /**< FQDN (or IP) and port of the final destination for this
-                  * connection; this is always the remote address, it is
-                  * passed to a proxy or pluggable transport if one in use.
-                  * See "addr" and "port" for the address that socket "s" is
-                  * directly connected to.
-                  * strdup into this, because free_connection() frees it. */
+  /**
+   * String address of the peer of this connection.
+   *
+   * TECHNICAL DEBT:
+   *
+   * This field serves many purposes, and they're not all pretty.  In addition
+   * to describing the peer we're connected to, it can also hold:
+   *
+   *    * An address we're trying to resolve (as an exit).
+   *    * A unix address we're trying to bind to (as a listener).
+   *    * A canonical address for an OR connection.
+   **/
+  char *address;
   /** Another connection that's connected to this one in lieu of a socket. */
   struct connection_t *linked_conn;
 
diff --git a/src/core/or/or_connection_st.h b/src/core/or/or_connection_st.h
index 92956c2847..2507f90803 100644
--- a/src/core/or/or_connection_st.h
+++ b/src/core/or/or_connection_st.h
@@ -49,10 +49,18 @@ struct or_connection_t {
   /* Channel using this connection */
   channel_tls_t *chan;
 
-  tor_addr_t real_addr; /**< The actual address that this connection came from
-                       * or went to.  The <b>addr</b> field is prone to
-                       * getting overridden by the address from the router
-                       * descriptor matching <b>identity_digest</b>. */
+  /**
+   * The actual address (as modified by any proxies) that this connection
+   * came from or went to.  (See connection_t.addr for caveats.)
+   *
+   * TECHNICAL DEBT:
+   *
+   * This field shouldn't really exist.  We need it because our code
+   * overwrites conenction_t.addr with the "canonical address" of the OR we
+   * are talking to, taken from the descriptor of the authenticated OR.
+   * That's a bad choice.
+   **/
+  tor_addr_t real_addr;
 
   /** Should this connection be used for extending circuits to the server
    * matching the <b>identity_digest</b> field?  Set to true if we're pretty