commit 20c0581a7935369fecb6c62b7cf5c7c244cdb533 Author: Nick Mathewson <nickm@torproject.org> Date: Thu Jul 7 10:40:23 2011 -0400 Launch sufficient circuits to satisfy pending isolated streams Our old "do we need to launch a circuit for stream S" logic was, more or less, that if we had a pending circuit that could handle S, we didn't need to launch a new one. But now that we have streams isolated from one another, we need something stronger here: It's possible that some pending C can handle either S1 or S2, but not both. This patch reuses the existing isolation logic for a simple solution: when we decide during circuit launching that some pending C would satisfy stream S1, we "hypothetically" mark C as though S1 had been connected to it. Now if S2 is incompatible with S1, it won't be something that can attach to C, and so we'll launch a new stream. When the circuit becomes OPEN for the first time (with no streams attached to it), we reset the circuit's isolation status. I'm not too sure about this part: I wanted some way to be sure that, if all streams that would have used a circuit die before the circuit is done, the circuit can still get used. But I worry that this approach could also lead to us launching too many circuits. Careful thought needed here. --- src/or/circuitlist.c | 18 ++++++++++++++++-- src/or/circuituse.c | 13 +++++++++++-- src/or/connection_edge.c | 35 +++++++++++++++++++++++++++++++++++ src/or/connection_edge.h | 1 + src/or/or.h | 24 +++++++++++++++++++----- 5 files changed, 82 insertions(+), 9 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 9f68880..6f17697 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -210,9 +210,23 @@ circuit_set_state(circuit_t *circ, uint8_t state) /* add to waiting-circuit list. */ smartlist_add(circuits_pending_or_conns, circ); } - if (state == CIRCUIT_STATE_OPEN) - tor_assert(!circ->n_conn_onionskin); + circ->state = state; + + if (state == CIRCUIT_STATE_OPEN) { + tor_assert(!circ->n_conn_onionskin); + if (CIRCUIT_IS_ORIGIN(circ)) { + origin_circuit_t *origin_circ = TO_ORIGIN_CIRCUIT(circ); + if (origin_circ->isolation_values_set && + !origin_circ->isolation_any_streams_attached) { + /* If we have any isolation information set on this circuit, + * but we never attached any streams to it, then all of the + * isolation information was hypothetical. Clear it. + */ + circuit_clear_isolation(origin_circ); + } + } + } } /** Add <b>circ</b> to the global list of circuits. This is called only from diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 19a6234..93098e5 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1457,12 +1457,20 @@ circuit_get_open_circ_or_launch(edge_connection_t *conn, rend_client_rendcirc_has_opened(circ); } } - } - if (!circ) + } /* endif (!circ) */ + if (circ) { + /* Mark the circuit with the isolation fields for this connection. + * When the circuit arrives, we'll clear these flags: this is + * just some internal bookkeeping to make sure that we have + * launched enough circuits. + */ + connection_edge_update_circuit_isolation(conn, circ, 0); + } else { log_info(LD_APP, "No safe circuit (purpose %d) ready for edge " "connection; delaying.", desired_circuit_purpose); + } *circp = circ; return 0; } @@ -1509,6 +1517,7 @@ link_apconn_to_circ(edge_connection_t *apconn, origin_circuit_t *circ, apconn->cpath_layer = circ->cpath->prev; } + circ->isolation_any_streams_attached = 1; connection_edge_update_circuit_isolation(apconn, circ, 0); } diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index ce555ed..20f01b1 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -3414,3 +3414,38 @@ connection_edge_update_circuit_isolation(const edge_connection_t *conn, } } +/** + * Clear the isolation settings on <b>circ</b>. + * + * This only works on an open circuit that has never had a stream attached to + * it, and whose isolation settings are hypothetical. (We set hypothetical + * isolation settings on circuits as we're launching them, so that we + * know whether they can handle more streams or whether we need to launch + * even more circuits. We clear the flags once the circuits are open, + * in case the streams that made us launch the circuits have closed + * since we began launching the circuits.) + */ +void +circuit_clear_isolation(origin_circuit_t *circ) +{ + if (circ->isolation_any_streams_attached) { + log_warn(LD_BUG, "Tried to clear the isolation status of a dirty circuit"); + return; + } + if (TO_CIRCUIT(circ)->state != CIRCUIT_STATE_OPEN) { + log_warn(LD_BUG, "Tried to clear the isolation status of a non-open " + "circuit"); + return; + } + + circ->isolation_values_set = 0; + circ->isolation_flags_mixed = 0; + circ->client_proto_type = 0; + circ->client_proto_socksver = 0; + circ->dest_port = 0; + tor_addr_make_unspec(&circ->client_addr); + tor_free(circ->dest_address); + circ->session_group = -1; + circ->nym_epoch = 0; +} + diff --git a/src/or/connection_edge.h b/src/or/connection_edge.h index ddcf8cc..8bd308d 100644 --- a/src/or/connection_edge.h +++ b/src/or/connection_edge.h @@ -110,6 +110,7 @@ int connection_edge_compatible_with_circuit(const edge_connection_t *conn, int connection_edge_update_circuit_isolation(const edge_connection_t *conn, origin_circuit_t *circ, int dry_run); +void circuit_clear_isolation(origin_circuit_t *circ); #endif diff --git a/src/or/or.h b/src/or/or.h index 9cf508c..97418f5 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2461,9 +2461,19 @@ typedef struct origin_circuit_t { /* XXXX NM This can get re-used after 2**32 circuits. */ uint32_t global_identifier; - /** True if we have attached at least one stream to this circuit, thereby - * setting the isolation paramaters for this circuit. */ + /** True if we have associated one stream to this circuit, thereby setting + * the isolation paramaters for this circuit. Note that this doesn't + * necessarily mean that we've <em>attached</em> any streams to the circuit: + * we may only have marked up this circuit during the launch process. + */ unsigned int isolation_values_set : 1; + /** True iff any stream has <em>ever</em> been attached to this circuit. + * + * In a better world we could use timestamp_dirty for this, but + * timestamp_dirty is far too overloaded at the moment. + */ + unsigned int isolation_any_streams_attached : 1; + /** A bitfield of ISO_* flags for every isolation field such that this * circuit has had streams with more than one value for that field * attached to it. */ @@ -2471,11 +2481,15 @@ typedef struct origin_circuit_t { /** @name Isolation parameters * - * If any streams have been attached to this circuit (isolation_values_set - * == 1), and all streams attached to the circuit have had the same value - * for some field ((isolation_flags_mixed & ISO_FOO) == 0), then these + * If any streams have been associated with this circ (isolation_values_set + * == 1), and all streams associated with the circuit have had the same + * value for some field ((isolation_flags_mixed & ISO_FOO) == 0), then these * elements hold the value for that field. * + * Note again that "associated" is not the same as "attached": we + * preliminarily associate streams with a circuit while the circuit is being + * launched, so that we can tell whether we need to launch more circuits. + * * @{ */ uint8_t client_proto_type;