commit 3d092ffbdd023a2be0307cb8a52bd48e2c85b257 Author: Andrea Shepard <andrea@torproject.org> Date: Mon Oct 1 13:06:10 2012 -0700 Handle closing circuits correctly with circuitmux_t --- src/or/circuitlist.c | 17 +++++--- src/or/circuitmux.c | 102 ++++++++++++++++++++++++++++---------------------- 2 files changed, 68 insertions(+), 51 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index a4dcec6..a87f989 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -131,11 +131,12 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction, if (old_chan) { /* * If we're changing channels or ID and had an old channel and a non - * zero old ID (i.e., we should have been attached), detach the circuit. - * ID changes require this because circuitmux hashes on (channel_id, - * circuit_id). + * zero old ID and weren't marked for close (i.e., we should have been + * attached), detach the circuit. ID changes require this because + * circuitmux hashes on (channel_id, circuit_id). */ - if (id != 0 && (old_chan != chan || old_id != id)) { + if (id != 0 && (old_chan != chan || old_id != id) && + !(circ->marked_for_close)) { tor_assert(old_chan->cmux); circuitmux_detach_circuit(old_chan->cmux, circ); } @@ -180,9 +181,11 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction, /* * Attach to the circuitmux if we're changing channels or IDs and - * have a new channel and ID to use. + * have a new channel and ID to use and the circuit is not marked for + * close. */ - if (chan && id != 0 && (old_chan != chan || old_id != id)) { + if (chan && id != 0 && (old_chan != chan || old_id != id) && + !(circ->marked_for_close)) { tor_assert(chan->cmux); circuitmux_attach_circuit(chan->cmux, circ, direction); attached = 1; @@ -1398,6 +1401,7 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line, if (circ->n_chan) { circuit_clear_cell_queue(circ, circ->n_chan); channel_send_destroy(circ->n_circ_id, circ->n_chan, reason); + circuitmux_detach_circuit(circ->n_chan->cmux, circ); } if (! CIRCUIT_IS_ORIGIN(circ)) { @@ -1425,6 +1429,7 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line, if (or_circ->p_chan) { circuit_clear_cell_queue(circ, or_circ->p_chan); channel_send_destroy(or_circ->p_circ_id, or_circ->p_chan, reason); + circuitmux_detach_circuit(or_circ->p_chan->cmux, circ); } } else { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 2bf511f..48336d8 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -130,6 +130,8 @@ struct chanid_circid_muxinfo_t { * Internal-use #defines */ +#define CMUX_PARANOIA + #ifdef CMUX_PARANOIA #define circuitmux_assert_okay_paranoid(cmux) \ circuitmux_assert_okay(cmux) @@ -1476,53 +1478,63 @@ circuitmux_assert_okay_pass_one(circuitmux_t *cmux) chan = channel_find_by_global_id(chan_id); tor_assert(chan); circ = circuit_get_by_circid_channel(circ_id, chan); - tor_assert(circ); - - /* Clear the circ_is_active bit to start */ - circ_is_active = 0; - - /* Assert that we know which direction this is going */ - tor_assert((*i)->muxinfo.direction == CELL_DIRECTION_OUT || - (*i)->muxinfo.direction == CELL_DIRECTION_IN); - - if ((*i)->muxinfo.direction == CELL_DIRECTION_OUT) { - /* We should be n_mux on this circuit */ - tor_assert(cmux == circ->n_mux); - tor_assert(chan == circ->n_chan); - /* Get next and prev for next test */ - next_p = &(circ->next_active_on_n_chan); - prev_p = &(circ->prev_active_on_n_chan); - } else { - /* This should be an or_circuit_t and we should be p_mux */ - or_circ = TO_OR_CIRCUIT(circ); - tor_assert(cmux == or_circ->p_mux); - tor_assert(chan == or_circ->p_chan); - /* Get next and prev for next test */ - next_p = &(or_circ->next_active_on_p_chan); - prev_p = &(or_circ->prev_active_on_p_chan); - } + if (circ) { + /* Clear the circ_is_active bit to start */ + circ_is_active = 0; + + /* Assert that we know which direction this is going */ + tor_assert((*i)->muxinfo.direction == CELL_DIRECTION_OUT || + (*i)->muxinfo.direction == CELL_DIRECTION_IN); + + if ((*i)->muxinfo.direction == CELL_DIRECTION_OUT) { + /* We should be n_mux on this circuit */ + tor_assert(cmux == circ->n_mux); + tor_assert(chan == circ->n_chan); + /* Get next and prev for next test */ + next_p = &(circ->next_active_on_n_chan); + prev_p = &(circ->prev_active_on_n_chan); + } else { + /* This should be an or_circuit_t and we should be p_mux */ + or_circ = TO_OR_CIRCUIT(circ); + tor_assert(cmux == or_circ->p_mux); + tor_assert(chan == or_circ->p_chan); + /* Get next and prev for next test */ + next_p = &(or_circ->next_active_on_p_chan); + prev_p = &(or_circ->prev_active_on_p_chan); + } - /* - * Should this circuit be active? I.e., does the mux know about > 0 - * cells on it? - */ - circ_is_active = ((*i)->muxinfo.cell_count > 0); - - /* It should be in the linked list iff it's active */ - if (circ_is_active) { - /* Either we have a next link or we are the tail */ - tor_assert(*next_p || (circ == cmux->active_circuits_tail)); - /* Either we have a prev link or we are the head */ - tor_assert(*prev_p || (circ == cmux->active_circuits_head)); - /* Increment the active circuits counter */ - ++n_active_circuits; + /* + * Should this circuit be active? I.e., does the mux know about > 0 + * cells on it? + */ + circ_is_active = ((*i)->muxinfo.cell_count > 0); + + /* It should be in the linked list iff it's active */ + if (circ_is_active) { + /* Either we have a next link or we are the tail */ + tor_assert(*next_p || (circ == cmux->active_circuits_tail)); + /* Either we have a prev link or we are the head */ + tor_assert(*prev_p || (circ == cmux->active_circuits_head)); + /* Increment the active circuits counter */ + ++n_active_circuits; + } else { + /* Shouldn't be in list, so no next or prev link */ + tor_assert(!(*next_p)); + tor_assert(!(*prev_p)); + /* And can't be head or tail */ + tor_assert(circ != cmux->active_circuits_head); + tor_assert(circ != cmux->active_circuits_tail); + } } else { - /* Shouldn't be in list, so no next or prev link */ - tor_assert(!(*next_p)); - tor_assert(!(*prev_p)); - /* And can't be head or tail */ - tor_assert(circ != cmux->active_circuits_head); - tor_assert(circ != cmux->active_circuits_tail); + /* + * circuit_get_by_circid_channel() doesn't want us to have a circuit + * that's marked for close, but we can test for that case with + * circuit_id_in_use_on_channel(). Assert if it really, really isn't + * there. + */ + tor_assert(circuit_id_in_use_on_channel(circ_id, chan)); + /* It definitely isn't active */ + circ_is_active = 0; } /* Increment the circuits counter */