commit 9d42a1ad9962307c53079a8e30574ade4998d950 Author: Zack Weinberg <zackw@cmu.edu> Date: Sun Jul 8 20:14:11 2012 +0200 Stop half-closing downstream connections. Up till now, when we were done writing to a downstream socket, we would use shutdown() to send a TCP FIN immediately, even if there might be data still to be _read_ from that socket. This turns out to be unreliable: on the far side, the FIN may cause libevent to signal read EOF before we are completely done reading data from the network, causing block loss. (This is arguably a bug in libevent, but shutdown() on socket bufferevents is not officially supported at present, so we need to fix it on our side.) Also, we have reason to believe none of our intended cover protocols normally leave sockets half-closed for any significant length of time, so this might be a 'tell.' And it has been observed to confuse middleboxes to the point where they won't pass our traffic at all. Instead, on the receive side, take note of a half-closed connection if the cover protocol provides an in-band indication of that fact (e.g. 'Connection: close' in HTTP) (i.e. conn_t::expect_close() now does something) but do not call shutdown() on the transmit side. This exposes a race condition in connection-to-circuit association which could cause a spurious fatal assertion, and allows us to simplify the 'should we open new downstream connections?' logic substantially. We still half-close our *upstream* sockets when we're done writing to them. That should probably change too, but that may require changes to tltester. --- src/network.cc | 16 ++++++++++------ src/protocol/chop.cc | 20 ++++++++------------ src/steg/nosteg_rr.cc | 7 ++++--- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/network.cc b/src/network.cc index 945fcbf..e4f2657 100644 --- a/src/network.cc +++ b/src/network.cc @@ -447,11 +447,7 @@ downstream_flush_cb(struct bufferevent *bev, void *arg) if (remain == 0 && ((conn->pending_write_eof && conn->connected) || (!conn->circuit() && conn->ever_received))) { - if (!conn->write_eof) { - log_debug(conn, "sending EOF downstream"); - shutdown(bufferevent_getfd(bev), SHUT_WR); - conn->write_eof = true; - } + conn->write_eof = true; if (conn->read_eof && conn->write_eof) conn->close(); } @@ -501,7 +497,15 @@ downstream_connect_cb(struct bufferevent *bev, short what, void *arg) connection, and replace this callback with the regular event_cb */ if (what & BEV_EVENT_CONNECTED) { circuit_t *ckt = conn->circuit(); - log_assert(ckt); + if (!ckt) { + // This can happen if the 3-way handshake for a new connection + // began while the circuit was still active but ended after the + // circuit was closed. Just drop the connection. + log_debug(conn, "successful connection for stale circuit"); + conn->close(); + return; + } + log_assert(ckt->up_peer); log_assert(conn->buffer == bev); diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index 9e3a605..bb4a95c 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -445,9 +445,11 @@ chop_circuit_t::send() struct evbuffer *xmit_pending = bufferevent_get_input(up_buffer); size_t avail = evbuffer_get_length(xmit_pending); size_t avail0 = avail; + bool no_target_connection = false; if (downstreams.empty()) { log_debug(this, "no downstream connections"); + no_target_connection = true; } else { // Send at least one block, even if there is no real data to send. do { @@ -458,6 +460,7 @@ chop_circuit_t::send() // this is not an error; it can happen e.g. when the server has // something to send immediately and the client hasn't spoken yet log_debug(this, "no target connection available"); + no_target_connection = true; break; } @@ -472,16 +475,10 @@ chop_circuit_t::send() dead_cycles++; log_debug(this, "%u dead cycles", dead_cycles); - // If there was real data or an EOF to send, and we didn't make - // any progress on it, or if there are no downstream connections - // at all, and we're the client, try opening new connections. If - // we're the server, we have to just twiddle our thumbs and hope - // the client does that. Note that due to the sliding window of - // receive blocks, there is a hard upper limit of 64 outstanding - // connections (that is, half the receive window). - if (downstreams.empty() || - (downstreams.size() <= 64 && - (avail0 > 0 || (upstream_eof && !sent_fin)))) { + // If we're the client and we had no target connection, try + // reopening new connections. If we're the server, we have to + // just twiddle our thumbs and hope the client does that. + if (no_target_connection) { if (config->mode != LSN_SIMPLE_SERVER) circuit_reopen_downstreams(this); else @@ -1160,8 +1157,7 @@ chop_conn_t::recv_eof() void chop_conn_t::expect_close() { - // We currently don't need to do anything here. - // FIXME: figure out if this hook is _ever_ useful, and if not, remove it. + read_eof = true; } void diff --git a/src/steg/nosteg_rr.cc b/src/steg/nosteg_rr.cc index 6719584..70aef90 100644 --- a/src/steg/nosteg_rr.cc +++ b/src/steg/nosteg_rr.cc @@ -104,9 +104,10 @@ nosteg_rr_steg_t::receive(struct evbuffer *dest) return -1; } - if (config->cfg->mode != LSN_SIMPLE_SERVER) { - conn->expect_close(); - } else if (!did_transmit) { + // Note: we can't call expect_close here because we don't know whether + // there's more data coming. + + if (config->cfg->mode == LSN_SIMPLE_SERVER && !did_transmit) { can_transmit = true; conn->transmit_soon(100); }