From fcf817f8976fb4ae2a255d54d1fc1eb1b11fdbfb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 15 Aug 2008 13:55:01 +0000 Subject: [PATCH] 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 --- ChangeLog | 9 +++++++++ src/or/connection.c | 41 +++++++-------------------------------- src/or/control.c | 30 ++++++++++++++--------------- src/or/cpuworker.c | 47 +++++++++++++++++---------------------------- src/or/or.h | 11 ++++------- 5 files changed, 53 insertions(+), 85 deletions(-) diff --git a/ChangeLog b/ChangeLog index b9e4a1d224..d42206c63b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -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 diff --git a/src/or/connection.c b/src/or/connection.c index 64b866748c..37362d2c2b 100644 --- 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 id 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; } diff --git a/src/or/control.c b/src/or/control.c index 31dd65c769..9c3f749d6f 100644 --- 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; diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index 77e15c748e..c6c68c07f5 100644 --- 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 tag 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; diff --git a/src/or/or.h b/src/or/or.h index ceecd9296b..80bcd1b745 100644 --- 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);