From 7b6b2d5fb8c760a506a052aa0fba7ba6a8fe4e77 Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Tue, 6 Dec 2011 03:46:02 -0800 Subject: [PATCH 1/3] Refactor stream attachment in circuit_has_opened Put the 'try attaching streams, clear isolation state if possible, retry attaching streams' loop in its own separate function, where it belongs. --- src/or/circuituse.c | 53 ++++++++++++++++++++++++++++++--------------- src/or/circuituse.h | 1 + 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/or/circuituse.c b/src/or/circuituse.c index bdff75cd5b..ef4ac6faa3 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1004,7 +1004,6 @@ circuit_testing_failed(origin_circuit_t *circ, int at_last_hop) void circuit_has_opened(origin_circuit_t *circ) { - int can_try_clearing_isolation = 0, tried_clearing_isolation = 0; control_event_circuit_status(circ, CIRC_EVENT_BUILT, 0); /* Remember that this circuit has finished building. Now if we start @@ -1012,20 +1011,18 @@ circuit_has_opened(origin_circuit_t *circ) * to consider its build time. */ circ->has_opened = 1; - again: - switch (TO_CIRCUIT(circ)->purpose) { case CIRCUIT_PURPOSE_C_ESTABLISH_REND: rend_client_rendcirc_has_opened(circ); - /* XXXX We'd like to set something like "can_try_clearing_isolation" - * here, so that we can change the isolation of this circuit (and maybe - * its purpose too) if it turns out that we no longer have any streams - * that want to use it. But connection_ap_attach_pending() doesn't - * actually attach streams to a C_ESTABLISH_REND circuit-- streams - * don't get attached until the circuit is in C_REND_JOINED... so - * we can't clear isolation now. - */ + /* Start building an intro circ if we don't have one yet. */ connection_ap_attach_pending(); + /* This isn't a call to circuit_try_attaching_streams because a + * circuit in _C_ESTABLISH_REND state isn't connected to its + * hidden service yet, thus we can't attach streams to it yet, + * thus circuit_try_attaching_streams would always clear the + * circuit's isolation state. circuit_try_attaching_streams is + * called later, when the rend circ enters _C_REND_JOINED + * state. */ break; case CIRCUIT_PURPOSE_C_INTRODUCING: rend_client_introcirc_has_opened(circ); @@ -1033,8 +1030,7 @@ circuit_has_opened(origin_circuit_t *circ) case CIRCUIT_PURPOSE_C_GENERAL: /* Tell any AP connections that have been waiting for a new * circuit that one is ready. */ - can_try_clearing_isolation = 1; - connection_ap_attach_pending(); + circuit_try_attaching_streams(circ); break; case CIRCUIT_PURPOSE_S_ESTABLISH_INTRO: /* at Bob, waiting for introductions */ @@ -1051,12 +1047,15 @@ circuit_has_opened(origin_circuit_t *circ) * This won't happen in normal operation, but might happen if the * controller did it. Just let it slide. */ } +} +/** If the stream-isolation state of circ can be cleared, clear + * it. Return non-zero iff circ's isolation state was cleared. */ +static int +circuit_try_clearing_isolation_state(origin_circuit_t *circ) +{ if (/* The circuit may have become non-open if it was cannibalized.*/ circ->_base.state == CIRCUIT_STATE_OPEN && - /* Only if the purpose is clearable, and only if we haven't tried - * to clear isolation yet, do we try. */ - can_try_clearing_isolation && !tried_clearing_isolation && /* If !isolation_values_set, there is nothing to clear. */ circ->isolation_values_set && /* It's not legal to clear a circuit's isolation info if it's ever had @@ -1066,8 +1065,26 @@ circuit_has_opened(origin_circuit_t *circ) * we didn't manage to attach any streams to it, then we can * and should clear it and try again. */ circuit_clear_isolation(circ); - tried_clearing_isolation = 1; - goto again; + return 1; + } else { + return 0; + } +} + +/** Called when a circuit becomes ready for streams to be attached to + * it. */ +void +circuit_try_attaching_streams(origin_circuit_t *circ) +{ + /* Attach streams to this circuit if we can. */ + connection_ap_attach_pending(); + + /* The call to circuit_try_clearing_isolation_state here will do + * nothing and return 0 if we didn't attach any streams to circ + * above. */ + if (circuit_try_clearing_isolation_state(circ)) { + /* Maybe *now* we can attach some streams to this circuit. */ + connection_ap_attach_pending(); } } diff --git a/src/or/circuituse.h b/src/or/circuituse.h index 9867fd8205..bc11fe5d91 100644 --- a/src/or/circuituse.h +++ b/src/or/circuituse.h @@ -29,6 +29,7 @@ void reset_bandwidth_test(void); int circuit_enough_testing_circs(void); void circuit_has_opened(origin_circuit_t *circ); +void circuit_try_attaching_streams(origin_circuit_t *circ); void circuit_build_failed(origin_circuit_t *circ); /** Flag to set when a circuit should have only a single hop. */ From 832bfc3c462f0b8dc1668b5b580f656a2b457a96 Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Tue, 6 Dec 2011 05:02:58 -0800 Subject: [PATCH 2/3] Clear stream-isolation state on rend circs if needed to attach streams Fixes bug 4655; bugfix on 0.2.3.3-alpha. --- changes/bug4655 | 10 ++++++++++ src/or/rendclient.c | 4 +++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changes/bug4655 diff --git a/changes/bug4655 b/changes/bug4655 new file mode 100644 index 0000000000..b91d87123c --- /dev/null +++ b/changes/bug4655 @@ -0,0 +1,10 @@ + o Minor bugfixes: + + - If we can't attach streams to a rendezvous circuit when we + finish connecting to a hidden service, clear the rendezvous + circuit's stream-isolation state and try to attach streams + again. Previously, we cleared rendezvous circuits' isolation + state either too early (if they were freshly built) or not at + all (if they had been built earlier and were cannibalized). + Bugfix on 0.2.3.3-alpha; fixes bug 4655. + diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 5429b6c7e3..c4744731df 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -892,10 +892,12 @@ rend_client_receive_rendezvous(origin_circuit_t *circ, const uint8_t *request, onion_append_to_cpath(&circ->cpath, hop); circ->build_state->pending_final_cpath = NULL; /* prevent double-free */ + /* XXXX023 This is a pretty brute-force approach. It'd be better to * attach only the connections that are waiting on this circuit, rather * than trying to attach them all. See comments bug 743. */ - connection_ap_attach_pending(); + circuit_try_attaching_streams(circ); + memset(keys, 0, sizeof(keys)); return 0; err: From 59b5379424295567dc68a50535cf544873ac28d9 Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Tue, 6 Dec 2011 19:24:55 -0800 Subject: [PATCH 3/3] Remove comment complaining that we try to attach all streams to circs It's inefficient, but the more efficient solution (only try to attach streams aiming for this HS) would require far more complexity for a gain that should be tiny. --- src/or/rendclient.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index c4744731df..8aae8c5cb5 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -893,9 +893,6 @@ rend_client_receive_rendezvous(origin_circuit_t *circ, const uint8_t *request, onion_append_to_cpath(&circ->cpath, hop); circ->build_state->pending_final_cpath = NULL; /* prevent double-free */ - /* XXXX023 This is a pretty brute-force approach. It'd be better to - * attach only the connections that are waiting on this circuit, rather - * than trying to attach them all. See comments bug 743. */ circuit_try_attaching_streams(circ); memset(keys, 0, sizeof(keys));