[tor-commits] [stegotorus/master] Stop half-closing downstream connections.

zwol at torproject.org zwol at torproject.org
Fri Jul 20 23:17:08 UTC 2012


commit 9d42a1ad9962307c53079a8e30574ade4998d950
Author: Zack Weinberg <zackw at 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);
   }





More information about the tor-commits mailing list