Switch global_identifier on connections to a 64-bit field and move it to connection_t. When procession onionskins, look up the connection by this field rather than by addr:port. This will keep us from dropping onionskins. How many dropped circuits are dropped because of this bug?

svn:r16558
2024-11-23 20:03:31 +01:00 · 2008-08-15 13:55:01 +00:00 · 2008-08-15 13:55:01 +00:00 · fcf817f897
commit fcf817f897
parent 1fcbd9f233
5 changed files with 53 additions and 85 deletions
--- a/9
+++ b/9
@ -7,6 +7,13 @@ Changes in version 0.2.1.5-alpha - 2008-08-??
      hidden services): associate keys, client lists, and authorization
      types with hidden services.

+  o Major bugfixes:
+    - When sending CREATED cells back for a given circuit, use a 64-bit
+      connection ID to find the right connection, rather than an addr:port
+      combination.  Now that we can have multiple OR connections between the
+      same ORs, it is no longer possible to use addr:port to uniquely
+      identify a connection.
+
  o Minor bugfixes:
    - Recover 3-7 bytes that were wasted per memory chunk.  Fixes bug
      794; bug spotted by rovv.  Bugfix on 0.2.0.1-alpha.
@ -17,6 +24,8 @@ Changes in version 0.2.1.5-alpha - 2008-08-??
    - Correctly detect the presence of the linux/netfilter_ipv4.h header
      when building against recent kernels.  Bugfix on 0.1.2.1-alpha.
    - Add a missing safe_str() call for a debug log message.
+    - Use 64 bits instead of 32 bits for connection identifiers used with
+      the controller protocol, to greatly reduce risk of identifier reuse.

  o Minor features
    - Rate-limit too-many-sockets messages: when they happen, they
--- a/src/or/connection.c
+++ b/src/or/connection.c
@ -166,7 +166,8 @@ conn_state_to_string(int type, int state)
 connection_t *
 connection_new(int type, int socket_family)
 {
-  static uint32_t n_connections_allocated = 1;
+  static uint64_t n_connections_allocated = 1;
+
  connection_t *conn;
  time_t now = time(NULL);
  size_t length;
@ -200,6 +201,7 @@ connection_new(int type, int socket_family)
  conn->magic = magic;
  conn->s = -1; /* give it a default of 'not used' */
  conn->conn_array_index = -1; /* also default to 'not used' */
+  conn->global_identifier = n_connections_allocated++;

  conn->type = type;
  conn->socket_family = socket_family;
@ -211,9 +213,6 @@ connection_new(int type, int socket_family)
    TO_EDGE_CONN(conn)->socks_request =
      tor_malloc_zero(sizeof(socks_request_t));
  }
-  if (CONN_IS_EDGE(conn)) {
-    TO_EDGE_CONN(conn)->global_identifier = n_connections_allocated++;
-  }
  if (type == CONN_TYPE_OR) {
    TO_OR_CONN(conn)->timestamp_last_added_nonpadding = now;
    TO_OR_CONN(conn)->next_circ_id = crypto_rand_int(1<<15);
@ -2437,28 +2436,6 @@ _connection_write_to_buf_impl(const char *string, size_t len,
  }
 }

-/** Return the conn to addr/port that has the most recent
- * timestamp_created, or NULL if no such conn exists. */
-or_connection_t *
-connection_or_exact_get_by_addr_port(uint32_t addr, uint16_t port)
-{
-  /* XXXX021 IP6 make this take a tor_addr_t, or deprecate it. */
-
-  or_connection_t *best=NULL;
-  smartlist_t *conns = get_connection_array();
-
-  SMARTLIST_FOREACH(conns, connection_t *, conn,
-  {
-    if (conn->type == CONN_TYPE_OR &&
-        tor_addr_eq_ipv4h(&conn->addr, addr) &&
-        conn->port == port &&
-        !conn->marked_for_close &&
-        (!best || best->_base.timestamp_created < conn->timestamp_created))
-      best = TO_OR_CONN(conn);
-  });
-  return best;
-}
-
 /** Return a connection with given type, address, port, and purpose;
 * or NULL if no such connection exists. */
 connection_t *
@ -2482,18 +2459,14 @@ connection_get_by_type_addr_port_purpose(int type,
 /** Return the stream with id <b>id</b> if it is not already marked for
 * close.
 */
-edge_connection_t *
-connection_get_by_global_id(uint32_t id)
+connection_t *
+connection_get_by_global_id(uint64_t id)
 {
  smartlist_t *conns = get_connection_array();
  SMARTLIST_FOREACH(conns, connection_t *, conn,
  {
-    if (CONN_IS_EDGE(conn) && TO_EDGE_CONN(conn)->global_identifier == id) {
-      if (!conn->marked_for_close)
-        return TO_EDGE_CONN(conn);
-      else
-        return NULL;
-    }
+    if (conn->global_identifier == id)
+      return conn;
  });
  return NULL;
 }
--- a/src/or/control.c
+++ b/src/or/control.c
@ -656,16 +656,16 @@ get_circ(const char *id)
 static edge_connection_t *
 get_stream(const char *id)
 {
-  uint32_t n_id;
+  uint64_t n_id;
  int ok;
-  edge_connection_t *conn;
-  n_id = (uint32_t) tor_parse_ulong(id, 10, 0, UINT32_MAX, &ok, NULL);
+  connection_t *conn;
+  n_id = tor_parse_uint64(id, 10, 0, UINT64_MAX, &ok, NULL);
  if (!ok)
    return NULL;
  conn = connection_get_by_global_id(n_id);
-  if (!conn || conn->_base.type != CONN_TYPE_AP)
+  if (!conn || conn->type != CONN_TYPE_AP || conn->marked_for_close)
    return NULL;
-  return conn;
+  return TO_EDGE_CONN(conn);
 }

 /** Helper for setconf and resetconf. Acts like setconf, except
@ -1648,8 +1648,7 @@ getinfo_helper_events(control_connection_t *control_conn,
    smartlist_t *conns = get_connection_array();
    smartlist_t *status = smartlist_create();
    char buf[256];
-    SMARTLIST_FOREACH(conns, connection_t *, base_conn,
-    {
+    SMARTLIST_FOREACH_BEGIN(conns, connection_t *, base_conn) {
      const char *state;
      edge_connection_t *conn;
      char *s;
@ -1691,12 +1690,12 @@ getinfo_helper_events(control_connection_t *control_conn,
      slen = strlen(buf)+strlen(state)+32;
      s = tor_malloc(slen+1);
      tor_snprintf(s, slen, "%lu %s %lu %s",
-                   (unsigned long) conn->global_identifier,state,
+                   (unsigned long) conn->_base.global_identifier,state,
                   origin_circ?
                         (unsigned long)origin_circ->global_identifier : 0ul,
                   buf);
      smartlist_add(status, s);
-    });
+    } SMARTLIST_FOREACH_END(base_conn);
    *answer = smartlist_join_strings(status, "\r\n", 0, NULL);
    SMARTLIST_FOREACH(status, char *, cp, tor_free(cp));
    smartlist_free(status);
@ -3136,8 +3135,8 @@ control_event_stream_status(edge_connection_t *conn, stream_status_event_t tp,
  if (circ && CIRCUIT_IS_ORIGIN(circ))
    origin_circ = TO_ORIGIN_CIRCUIT(circ);
  send_control_event_extended(EVENT_STREAM_STATUS, ALL_NAMES,
-                        "650 STREAM %lu %s %lu %s@%s%s%s\r\n",
-                        (unsigned long)conn->global_identifier, status,
+                        "650 STREAM "U64_FORMAT" %s %lu %s@%s%s%s\r\n",
+                        U64_PRINTF_ARG(conn->_base.global_identifier), status,
                        origin_circ?
                           (unsigned long)origin_circ->global_identifier : 0ul,
                        buf, reason_buf, addrport_buf, purpose);
@ -3242,7 +3241,7 @@ control_event_stream_bandwidth_used(void)
    smartlist_t *conns = get_connection_array();
    edge_connection_t *edge_conn;

-    SMARTLIST_FOREACH(conns, connection_t *, conn,
+    SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn)
    {
        if (conn->type != CONN_TYPE_AP)
          continue;
@ -3251,13 +3250,14 @@ control_event_stream_bandwidth_used(void)
          continue;

        send_control_event(EVENT_STREAM_BANDWIDTH_USED, ALL_NAMES,
-                            "650 STREAM_BW %lu %lu %lu\r\n",
-                            (unsigned long)edge_conn->global_identifier,
+                            "650 STREAM_BW "U64_FORMAT" %lu %lu\r\n",
+                            U64_PRINTF_ARG(edge_conn->_base.global_identifier),
                            (unsigned long)edge_conn->n_read,
                            (unsigned long)edge_conn->n_written);

        edge_conn->n_written = edge_conn->n_read = 0;
-    });
+    }
+    SMARTLIST_FOREACH_END(conn);
  }

  return 0;
--- a/src/or/cpuworker.c
+++ b/src/or/cpuworker.c
@ -23,7 +23,7 @@ const char cpuworker_c_id[] =
 #define MIN_CPUWORKERS 1

 /** The tag specifies which circuit this onionskin was from. */
-#define TAG_LEN 8
+#define TAG_LEN 10
 /** How many bytes are sent from the cpuworker back to tor? */
 #define LEN_ONION_RESPONSE \
  (1+TAG_LEN+ONIONSKIN_REPLY_LEN+CPATH_KEY_MATERIAL_LEN)
@ -60,33 +60,23 @@ connection_cpu_finished_flushing(connection_t *conn)
  return 0;
 }

-/** Pack addr,port,and circ_id; set *tag to the result. (See note on
+/** Pack global_id and circ_id; set *tag to the result. (See note on
 * cpuworker_main for wire format.) */
 static void
-tag_pack(char *tag, const tor_addr_t *addr, uint16_t port, circid_t circ_id)
+tag_pack(char *tag, uint64_t conn_id, circid_t circ_id)
 {
  /*XXXX RETHINK THIS WHOLE MESS !!!! !NM NM NM NM*/
-  *(uint32_t *)tag     = tor_addr_to_ipv4h(addr);
-  *(uint16_t *)(tag+4) = port;
-  *(uint16_t *)(tag+6) = circ_id;
+  *(uint64_t *)tag     = conn_id;
+  *(uint16_t *)(tag+8) = circ_id;
 }

 /** Unpack <b>tag</b> into addr, port, and circ_id.
 */
 static void
-tag_unpack(const char *tag, uint32_t *addr, uint16_t *port, circid_t *circ_id)
+tag_unpack(const char *tag, uint64_t *conn_id, circid_t *circ_id)
 {
-  struct in_addr in;
-  char addrbuf[INET_NTOA_BUF_LEN];
-
-  *addr    = *(const uint32_t *)tag;
-  *port    = *(const uint16_t *)(tag+4);
-  *circ_id = *(const uint16_t *)(tag+6);
-
-  in.s_addr = htonl(*addr);
-  tor_inet_ntoa(&in, addrbuf, sizeof(addrbuf));
-  log_debug(LD_OR,
-            "onion was from %s:%d, circ_id %d.", addrbuf, *port, *circ_id);
+  *conn_id = *(const uint64_t *)tag;
+  *circ_id = *(const uint16_t *)(tag+8);
 }

 /** Called when the onion key has changed and we need to spawn new
@ -136,10 +126,10 @@ connection_cpu_process_inbuf(connection_t *conn)
 {
  char success;
  char buf[LEN_ONION_RESPONSE];
-  uint32_t addr;
-  uint16_t port;
+  uint64_t conn_id;
  circid_t circ_id;
-  or_connection_t *p_conn;
+  connection_t *tmp_conn;
+  or_connection_t *p_conn = NULL;
  circuit_t *circ;

  tor_assert(conn);
@ -157,14 +147,13 @@ connection_cpu_process_inbuf(connection_t *conn)
    connection_fetch_from_buf(buf,LEN_ONION_RESPONSE-1,conn);

    /* parse out the circ it was talking about */
-    tag_unpack(buf, &addr, &port, &circ_id);
+    tag_unpack(buf, &conn_id, &circ_id);
    circ = NULL;
-    /* (Here we use connection_or_exact_get_by_addr_port rather than
-     * get_by_identity_digest: we want a specific port here in
-     * case there are multiple connections.) */
-    /* XXXX021 This is dumb. We don't want just any connection with a matching
-     * IP and port: we want the exact one that sent us this CREATE cell. */
-    p_conn = connection_or_exact_get_by_addr_port(addr,port);
+    tmp_conn = connection_get_by_global_id(conn_id);
+    if (tmp_conn && !tmp_conn->marked_for_close &&
+        tmp_conn->type == CONN_TYPE_OR)
+      p_conn = TO_OR_CONN(tmp_conn);
+
    if (p_conn)
      circ = circuit_get_by_circid_orconn(circ_id, p_conn);

@ -471,7 +460,7 @@ assign_onionskin_to_cpuworker(connection_t *cpuworker,
      tor_free(onionskin);
      return -1;
    }
-    tag_pack(tag, &circ->p_conn->_base.addr, circ->p_conn->_base.port,
+    tag_pack(tag, circ->p_conn->_base.global_identifier,
             circ->p_circ_id);

    cpuworker->state = CPUWORKER_STATE_BUSY_ONION;
--- a/src/or/or.h
+++ b/src/or/or.h
@ -904,6 +904,9 @@ typedef struct connection_t {
  /** Another connection that's connected to this one in lieu of a socket. */
  struct connection_t *linked_conn;

+  /** Unique identifier for this connection. */
+  uint64_t global_identifier;
+
  /* XXXX021 move this into a subtype. */
  struct evdns_server_port *dns_server_port;

@ -1014,10 +1017,6 @@ typedef struct edge_connection_t {
  /** The reason why this connection is closing; passed to the controller. */
  uint16_t end_reason;

-  /** Quasi-global identifier for this connection; used for control.c */
-  /* XXXX NM This can get re-used after 2**32 streams */
-  uint32_t global_identifier;
-
  /** Bytes read since last call to control_event_stream_bandwidth_used() */
  uint32_t n_read;

@ -2858,9 +2857,7 @@ connection_write_to_buf_zlib(const char *string, size_t len,
  _connection_write_to_buf_impl(string, len, TO_CONN(conn), done ? -1 : 1);
 }

-or_connection_t *connection_or_exact_get_by_addr_port(uint32_t addr,
-                                                   uint16_t port);
-edge_connection_t *connection_get_by_global_id(uint32_t id);
+connection_t *connection_get_by_global_id(uint64_t id);

 connection_t *connection_get_by_type(int type);
 connection_t *connection_get_by_type_purpose(int type, int purpose);