diff --git a/ChangeLog b/ChangeLog index ee9e3828e7..aa63ab9926 100644 --- a/ChangeLog +++ b/ChangeLog @@ -64,6 +64,8 @@ Changes in version 0.1.2.7-alpha - 2007-??-?? - Stop using the reserved ac_cv namespace in our configure script. - Call stat() slightly less often; use fstat() when possible. - Treat failure to parse resolv.conf as an error. + - Refactor the way we handle pending circuits when an OR connection + completes or fails, in an attempt to fix a rare crash bug. o Major features: - Weight directory requests by advertised bandwidth. Now we can diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 5a158f32e6..5ae6ceeeb5 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -388,8 +388,6 @@ circuit_handle_first_hop(origin_circuit_t *circ) return 0; } -extern smartlist_t *circuits_pending_or_conns; - /** Find any circuits that are waiting on or_conn to become * open and get them to send their create cells forward. * @@ -398,25 +396,24 @@ extern smartlist_t *circuits_pending_or_conns; void circuit_n_conn_done(or_connection_t *or_conn, int status) { - smartlist_t *changed_circs; + smartlist_t *pending_circs; int err_reason = 0; log_debug(LD_CIRC,"or_conn to %s, status=%d", or_conn->nickname ? or_conn->nickname : "NULL", status); - if (!circuits_pending_or_conns) - return; + pending_circs = smartlist_create(); + circuit_get_all_pending_on_or_conn(pending_circs, or_conn); - changed_circs = smartlist_create(); - - SMARTLIST_FOREACH(circuits_pending_or_conns, circuit_t *, circ, - { - if (circ->marked_for_close) - continue; - tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT); - if (!circ->n_conn && - !memcmp(or_conn->identity_digest, circ->n_conn_id_digest, - DIGEST_LEN)) { + SMARTLIST_FOREACH(pending_circs, circuit_t *, circ, + { + /* This check is redundant wrt get_all_pending_on_or_conn, but I'm + * leaving them in in case it's possible for the status of a circuit to + * change as we're going down the list. */ + if (circ->marked_for_close || circ->n_conn || + circ->state != CIRCUIT_STATE_OR_WAIT || + memcmp(or_conn->identity_digest, circ->n_conn_id_digest, DIGEST_LEN)) + continue; if (!status) { /* or_conn failed; close circ */ log_info(LD_CIRC,"or_conn failed. Closing circ."); circuit_mark_for_close(circ, END_CIRC_REASON_OR_CONN_CLOSED); @@ -445,18 +442,11 @@ circuit_n_conn_done(or_connection_t *or_conn, int status) continue; } tor_free(circ->onionskin); - /* We don't want to change circ's state here, since the act - * of doing that modifies the circuits_pending_or_conns list - * that we're looping through right now. So collect a list of - * circs to change their state when we're done. */ - smartlist_add(changed_circs, circ); + circuit_set_state(circ, CIRCUIT_STATE_OPEN); } - } - }); + }); - SMARTLIST_FOREACH(changed_circs, circuit_t *, circ, - circuit_set_state(circ, CIRCUIT_STATE_OPEN)); - smartlist_free(changed_circs); + smartlist_free(pending_circs); } /** Find a new circid that isn't currently in use on the circ->n_conn diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 2296ca765f..ce8ba5f73a 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -21,7 +21,7 @@ const char circuitlist_c_id[] = circuit_t *global_circuitlist=NULL; /** A list of all the circuits in CIRCUIT_STATE_OR_WAIT. */ -smartlist_t *circuits_pending_or_conns=NULL; +static smartlist_t *circuits_pending_or_conns=NULL; static void circuit_free(circuit_t *circ); static void circuit_free_cpath(crypt_path_t *cpath); @@ -165,15 +165,14 @@ circuit_set_state(circuit_t *circ, int state) tor_assert(circ); if (state == circ->state) return; + if (!circuits_pending_or_conns) + circuits_pending_or_conns = smartlist_create(); if (circ->state == CIRCUIT_STATE_OR_WAIT) { /* remove from waiting-circuit list. */ - if (circuits_pending_or_conns) - smartlist_remove(circuits_pending_or_conns, circ); + smartlist_remove(circuits_pending_or_conns, circ); } if (state == CIRCUIT_STATE_OR_WAIT) { /* add to waiting-circuit list. */ - if (!circuits_pending_or_conns) - circuits_pending_or_conns = smartlist_create(); smartlist_add(circuits_pending_or_conns, circ); } circ->state = state; @@ -194,6 +193,45 @@ circuit_add(circuit_t *circ) } } +/** Append to out the number of circuits in state OR_WAIT, waiting for + * the given connection. */ +void +circuit_get_all_pending_on_or_conn(smartlist_t *out, or_connection_t *or_conn) +{ + tor_assert(out); + tor_assert(or_conn); + + if (!circuits_pending_or_conns) + return; + + SMARTLIST_FOREACH(circuits_pending_or_conns, circuit_t *, circ, + { + if (circ->marked_for_close) + continue; + tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT); + if (!circ->n_conn && + !memcmp(or_conn->identity_digest, circ->n_conn_id_digest, + DIGEST_LEN)) { + smartlist_add(out, circ); + } + }); +} + +/** Return the number of circuits in state OR_WAIT, waiting for the given + * connection. */ +int +circuit_count_pending_on_or_conn(or_connection_t *or_conn) +{ + int cnt; + smartlist_t *sl = smartlist_create(); + circuit_get_all_pending_on_or_conn(sl, or_conn); + cnt = smartlist_len(sl); + smartlist_free(sl); + log_debug(LD_CIRC,"or_conn to %s, %d pending circs", + or_conn->nickname ? or_conn->nickname : "NULL", cnt); + return cnt; +} + /** Detach from the global circuit list, and deallocate, all * circuits that have been marked for close. */ diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 420d6b9fa3..b234e906a7 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -779,37 +779,6 @@ connection_or_send_destroy(uint16_t circ_id, or_connection_t *conn, int reason) return 0; } -/* XXXX012 This global is getting _too_ global. -NM */ -extern smartlist_t *circuits_pending_or_conns; - -/** Count number of pending circs on an or_conn */ -int -connection_or_count_pending_circs(or_connection_t *or_conn) -{ - int cnt = 0; - - if (!circuits_pending_or_conns) - return 0; - - tor_assert(or_conn); - - SMARTLIST_FOREACH(circuits_pending_or_conns, circuit_t *, circ, - { - if (circ->marked_for_close) - continue; - tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT); - if (!circ->n_conn && - !memcmp(or_conn->identity_digest, circ->n_conn_id_digest, - DIGEST_LEN)) { - cnt++; - } - }); - - log_debug(LD_CIRC,"or_conn to %s, %d pending circs", - or_conn->nickname ? or_conn->nickname : "NULL", cnt); - return cnt; -} - /** DOCDOC */ #define BUF_FULLNESS_THRESHOLD (128*1024) /** DOCDOC */ diff --git a/src/or/control.c b/src/or/control.c index b02bbd34e3..efff7019b6 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -3356,7 +3356,7 @@ control_event_or_conn_status(or_connection_t *conn,or_conn_status_event_t tp, log_warn(LD_BUG, "Unrecognized status code %d", (int)tp); return 0; } - ncircs = connection_or_count_pending_circs(conn); + ncircs = circuit_count_pending_on_or_conn(conn); ncircs += conn->n_circuits; if (ncircs && (tp == OR_CONN_EVENT_FAILED || tp == OR_CONN_EVENT_CLOSED)) { tor_snprintf(ncircs_buf, sizeof(ncircs_buf), "%sNCIRCS=%d", diff --git a/src/or/or.h b/src/or/or.h index d61d00ed39..abdd62fd2c 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1989,6 +1989,9 @@ void circuit_expire_all_dirty_circs(void); void _circuit_mark_for_close(circuit_t *circ, int reason, int line, const char *file); int circuit_get_cpath_len(origin_circuit_t *circ); +void circuit_get_all_pending_on_or_conn(smartlist_t *out, + or_connection_t *or_conn); +int circuit_count_pending_on_or_conn(or_connection_t *or_conn); #define circuit_mark_for_close(c, reason) \ _circuit_mark_for_close((c), (reason), __LINE__, _SHORT_FILE_) @@ -2250,7 +2253,6 @@ void connection_or_write_cell_to_buf(const cell_t *cell, or_connection_t *conn); int connection_or_send_destroy(uint16_t circ_id, or_connection_t *conn, int reason); -int connection_or_count_pending_circs(or_connection_t *or_conn); /********************************* control.c ***************************/