From 66aff2d8f35217cc802bd46eeeaf49326d7de4b0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Sep 2017 14:39:40 -0400 Subject: [PATCH 1/5] Remove or_circuit_t.is_first_hop; use channel_is_client() instead The is_first_hop field should have been called used_create_fast, but everywhere that we wanted to check it, we should have been checking channel_is_client() instead. --- src/or/circuitbuild.c | 4 ++-- src/or/circuituse.c | 6 +++--- src/or/connection_edge.c | 9 +++++---- src/or/or.h | 3 --- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 65cd7bd5dc..e614c3f287 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1611,12 +1611,12 @@ onionskin_answer(or_circuit_t *circ, memcpy(circ->rend_circ_nonce, rend_circ_nonce, DIGEST_LEN); - circ->is_first_hop = (created_cell->cell_type == CELL_CREATED_FAST); + int used_create_fast = (created_cell->cell_type == CELL_CREATED_FAST); append_cell_to_circuit_queue(TO_CIRCUIT(circ), circ->p_chan, &cell, CELL_DIRECTION_IN, 0); log_debug(LD_CIRC,"Finished sending '%s' cell.", - circ->is_first_hop ? "created_fast" : "created"); + used_create_fast ? "created_fast" : "created"); /* Ignore the local bit when ExtendAllowPrivateAddresses is set: * it violates the assumption that private addresses are local. diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 570b05e572..6a45979871 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1514,7 +1514,7 @@ circuit_expire_old_circuits_clientside(void) #define IDLE_ONE_HOP_CIRC_TIMEOUT 60 /** Find each non-origin circuit that has been unused for too long, - * has no streams on it, used a create_fast, and ends here: mark it + * has no streams on it, came from a client, and ends here: mark it * for close. */ void @@ -1530,9 +1530,9 @@ circuit_expire_old_circuits_serverside(time_t now) /* If the circuit has been idle for too long, and there are no streams * on it, and it ends here, and it used a create_fast, mark it for close. */ - if (or_circ->is_first_hop && !circ->n_chan && + if (or_circ->p_chan && channel_is_client(or_circ->p_chan) && + !circ->n_chan && !or_circ->n_streams && !or_circ->resolving_streams && - or_circ->p_chan && channel_when_last_xmit(or_circ->p_chan) <= cutoff) { log_info(LD_CIRC, "Closing circ_id %u (empty %d secs ago)", (unsigned)or_circ->p_circ_id, diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index a9fdeee0ee..82dad60fc5 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -3413,7 +3413,8 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ) port = bcell.port; if (or_circ && or_circ->p_chan) { - if ((or_circ->is_first_hop || + const int client_chan = channel_is_client(or_circ->p_chan); + if ((client_chan || (!connection_or_digest_is_known_relay( or_circ->p_chan->identity_digest) && should_refuse_unknown_exits(options)))) { @@ -3423,10 +3424,10 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ) log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Attempt by %s to open a stream %s. Closing.", safe_str(channel_get_canonical_remote_descr(or_circ->p_chan)), - or_circ->is_first_hop ? "on first hop of circuit" : - "from unknown relay"); + client_chan ? "on first hop of circuit" : + "from unknown relay"); relay_send_end_cell_from_edge(rh.stream_id, circ, - or_circ->is_first_hop ? + client_chan ? END_STREAM_REASON_TORPROTOCOL : END_STREAM_REASON_MISC, NULL); diff --git a/src/or/or.h b/src/or/or.h index b3cd83f245..2712ac3e59 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3459,9 +3459,6 @@ typedef struct or_circuit_t { /* We have already received an INTRODUCE1 cell on this circuit. */ unsigned int already_received_introduce1 : 1; - /** True iff this circuit was made with a CREATE_FAST cell. */ - unsigned int is_first_hop : 1; - /** If set, this circuit carries HS traffic. Consider it in any HS * statistics. */ unsigned int circuit_carries_hs_traffic_stats : 1; From d1e0e486e9eafa7ba44e3ed49ce1b92fec9c7201 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Sep 2017 14:55:58 -0400 Subject: [PATCH 2/5] Stop clearing the is_client flag on channel directly --- src/or/channel.c | 14 ++++++++++++++ src/or/channel.h | 1 + src/or/command.c | 2 +- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/or/channel.c b/src/or/channel.c index 9f8a03683f..ea113903af 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -4096,6 +4096,20 @@ channel_mark_client(channel_t *chan) chan->is_client = 1; } +/** + * Clear the client flag + * + * Mark a channel as being _not_ from a client + */ + +void +channel_clear_client(channel_t *chan) +{ + tor_assert(chan); + + chan->is_client = 0; +} + /** * Get the canonical flag for a channel * diff --git a/src/or/channel.h b/src/or/channel.h index 2d0ec39924..a5a87de136 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -671,6 +671,7 @@ int channel_is_local(channel_t *chan); int channel_is_incoming(channel_t *chan); int channel_is_outgoing(channel_t *chan); void channel_mark_client(channel_t *chan); +void channel_clear_client(channel_t *chan); int channel_matches_extend_info(channel_t *chan, extend_info_t *extend_info); int channel_matches_target_addr_for_extend(channel_t *chan, const tor_addr_t *target); diff --git a/src/or/command.c b/src/or/command.c index 2c82984901..46d3b6291c 100644 --- a/src/or/command.c +++ b/src/or/command.c @@ -331,7 +331,7 @@ command_process_create_cell(cell_t *cell, channel_t *chan) // Needed for chutney: Sometimes relays aren't in the consensus yet, and // get marked as clients. This resets their channels once they appear. // Probably useful for normal operation wrt relay flapping, too. - chan->is_client = 0; + channel_clear_client(chan); } else { channel_mark_client(chan); } From ceb49c1c5f2556c7fcd5b1ff34d8988344fa2ba7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Sep 2017 15:37:09 -0400 Subject: [PATCH 3/5] Use channel_is_client() accessor in channelpadding.c. Also, allow channel_is_client() to take a const channel. --- src/or/channel.c | 2 +- src/or/channel.h | 2 +- src/or/channelpadding.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index ea113903af..fa704e0426 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -4075,7 +4075,7 @@ channel_mark_bad_for_new_circs(channel_t *chan) */ int -channel_is_client(channel_t *chan) +channel_is_client(const channel_t *chan) { tor_assert(chan); diff --git a/src/or/channel.h b/src/or/channel.h index a5a87de136..e913e9749e 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -666,7 +666,7 @@ int channel_is_bad_for_new_circs(channel_t *chan); void channel_mark_bad_for_new_circs(channel_t *chan); int channel_is_canonical(channel_t *chan); int channel_is_canonical_is_reliable(channel_t *chan); -int channel_is_client(channel_t *chan); +int channel_is_client(const channel_t *chan); int channel_is_local(channel_t *chan); int channel_is_incoming(channel_t *chan); int channel_is_outgoing(channel_t *chan); diff --git a/src/or/channelpadding.c b/src/or/channelpadding.c index bed2489837..2122602ffd 100644 --- a/src/or/channelpadding.c +++ b/src/or/channelpadding.c @@ -66,7 +66,7 @@ static int consensus_nf_pad_relays; * its a client, use that. Then finally verify in the consensus). */ #define CHANNEL_IS_CLIENT(chan, options) \ - (!public_server_mode((options)) || (chan)->is_client || \ + (!public_server_mode((options)) || channel_is_client(chan) || \ !connection_or_digest_is_known_relay((chan)->identity_digest)) /** From 6a75a6fd9a7cd9c60904388f6836e7e659ef60e6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Sep 2017 16:18:40 -0400 Subject: [PATCH 4/5] changes file for bug22805 --- changes/bug22805 | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 changes/bug22805 diff --git a/changes/bug22805 b/changes/bug22805 new file mode 100644 index 0000000000..6d95ad5a33 --- /dev/null +++ b/changes/bug22805 @@ -0,0 +1,9 @@ + o Minor features (relay): + - When choosing which circuits can be expired as unused, consider + circuits from clients even if those clients used regular CREATE + cells to make them. Part of ticket 22805. + + o Code simplification and refactoring: + - Remove various ways of testing circuits and connections for + "clientness"; instead, favor channel_is_client(). + Part of ticket 22805. From 95a7e7e9254bc70db9e1d967d0db3deb658a2be3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 15 Sep 2017 09:43:13 -0400 Subject: [PATCH 5/5] Stop using CREATE_FAST as a signifier of clienthood. Relays send it, and we may as well let them. Part of our fix for 22805. --- changes/bug22805 | 3 ++- src/or/command.c | 10 ---------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/changes/bug22805 b/changes/bug22805 index 6d95ad5a33..2b0369da30 100644 --- a/changes/bug22805 +++ b/changes/bug22805 @@ -1,7 +1,8 @@ o Minor features (relay): - When choosing which circuits can be expired as unused, consider circuits from clients even if those clients used regular CREATE - cells to make them. Part of ticket 22805. + cells to make them; and do not consider circuits from relays even if + they were made with CREATE_FAST. Part of ticket 22805. o Code simplification and refactoring: - Remove various ways of testing circuits and connections for diff --git a/src/or/command.c b/src/or/command.c index 46d3b6291c..42b42b21bb 100644 --- a/src/or/command.c +++ b/src/or/command.c @@ -353,16 +353,6 @@ command_process_create_cell(cell_t *cell, channel_t *chan) int len; created_cell_t created_cell; - /* If the client used CREATE_FAST, it's probably a tor client or bridge - * relay, and we must not use it for EXTEND requests (in most cases, we - * won't have an authenticated peer ID for the extend). - * Public relays on 0.2.9 and later will use CREATE_FAST if they have no - * ntor onion key for this relay, but that should be a rare occurrence. - * Clients on 0.3.1 and later avoid using CREATE_FAST as much as they can, - * even during bootstrap, so the CREATE_FAST check is most accurate for - * earlier tor client versions. */ - channel_mark_client(chan); - memset(&created_cell, 0, sizeof(created_cell)); len = onion_skin_server_handshake(ONION_HANDSHAKE_TYPE_FAST, create_cell->onionskin,