commit 4ae14e9e21d9b2848d089ba3ee150582e20744b9 Author: Zack Weinberg zackw@panix.com Date: Wed Feb 1 21:19:18 2012 -0800

Another subtle bug involving races near circuit teardown. --- src/network.cc | 3 ++- src/protocol/chop.cc | 32 +++++++++++++++++++------------- 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/network.cc b/src/network.cc index a11cd7d..a55e539 100644 --- a/src/network.cc +++ b/src/network.cc @@ -423,7 +423,8 @@ upstream_flush_cb(struct bufferevent *bev, void *arg) ckt->connected ? "" : " (not connected)", ckt->flushing ? "" : " (not flushing)");

- if (remain == 0 && ckt->flushing && ckt->connected) { + if (remain == 0 && ckt->flushing && ckt->connected + && (!ckt->flush_timer || !evtimer_pending(ckt->flush_timer, NULL))) { bufferevent_disable(bev, EV_WRITE); if (bufferevent_get_enabled(bev) || evbuffer_get_length(bufferevent_get_input(bev)) > 0) { diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index 5acc010..d7da97f 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -784,8 +784,6 @@ chop_push_to_upstream(circuit_t *c) static int chop_find_or_make_circuit(conn_t *conn, uint64_t circuit_id) { - log_assert(conn->cfg->mode == LSN_SIMPLE_SERVER); - chop_config_t *cfg = static_cast<chop_config_t *>(conn->cfg); chop_circuit_table::value_type in(circuit_id, 0); std::pair<chop_circuit_table::iterator, bool> out = cfg->circuits.insert(in); @@ -954,6 +952,11 @@ chop_config_t::circuit_create(size_t) ckt->recv_crypt = decryptor::create(s2c_key, 16); while (!ckt->circuit_id) rng_bytes((uint8_t *)&ckt->circuit_id, sizeof(uint64_t)); + + chop_circuit_table::value_type in(ckt->circuit_id, 0); + std::pair<chop_circuit_table::iterator, bool> out = circuits.insert(in); + log_assert(out.second); + out.first->second = ckt; } return ckt; } @@ -996,17 +999,20 @@ chop_circuit_t::~chop_circuit_t() free(p); }

- if (this->cfg->mode == LSN_SIMPLE_SERVER) { - /* The IDs for old circuits are preserved for a while (at present, - indefinitely; FIXME: purge them on a timer) against the - possibility that we'll get a junk connection for one of them - right after we close it (same deal as the TIME_WAIT state in TCP). */ - chop_config_t *cfg = static_cast<chop_config_t *>(this->cfg); - out = cfg->circuits.find(this->circuit_id); - log_assert(out != cfg->circuits.end()); - log_assert(out->second == this); - out->second = NULL; - } + /* The IDs for old circuits are preserved for a while (at present, + indefinitely; FIXME: purge them on a timer) against the + possibility that we'll get a junk connection for one of them + right after we close it (same deal as the TIME_WAIT state in + TCP). Note that we can hit this case for the *client* if the + cover protocol includes a mandatory reply to every client + message and the hidden channel closed s->c before c->s: the + circuit will get destroyed on the client side after the c->s FIN, + and the mandatory reply will be to a stale circuit. */ + chop_config_t *cfg = static_cast<chop_config_t *>(this->cfg); + out = cfg->circuits.find(this->circuit_id); + log_assert(out != cfg->circuits.end()); + log_assert(out->second == this); + out->second = NULL; }

void