Expire socks connections if they spend too long waiting for the

handshake to finish. Previously we would let them sit around for
days, if the connecting application didn't close them either.

Also take this opportunity to refactor a duplicate bit of circuituse.c.

And change the semantics of SocksTimeout slightly, but I think it'll
be ok.


svn:r9350

3 files changed, 26 insertions, 23 deletions

diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index 7c2178fd7..4114d1295 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -532,7 +532,7 @@ circuit_detach_stream(circuit_t *circ, edge_connection_t *conn)
 }
 
 /** Find each circuit that has been unused for too long, or dirty
- * for too long and has no streax=ms on it: mark it for close.
+ * for too long and has no streams on it: mark it for close.
  */
 static void
 circuit_expire_old_circuits(time_t now)
@@ -1180,21 +1180,12 @@ connection_ap_handshake_attach_circuit(edge_connection_t *conn)
 {
   int retval;
   int conn_age;
-  int severity;
 
   tor_assert(conn);
   tor_assert(conn->_base.state == AP_CONN_STATE_CIRCUIT_WAIT);
   tor_assert(conn->socks_request);
 
   conn_age = time(NULL) - conn->_base.timestamp_created;
-  severity = (!conn->_base.addr && !conn->_base.port) ? LOG_INFO : LOG_NOTICE;
-  if (conn_age > get_options()->SocksTimeout) {
-    log_fn(severity, LD_APP,
-           "Tried for %d seconds to get a connection to %s:%d. Giving up.",
-           conn_age, safe_str(conn->socks_request->address),
-           conn->socks_request->port);
-    return -1;
-  }
 
   if (!connection_edge_is_rendezvous_stream(conn)) { /* we're a general conn */
     origin_circuit_t *circ=NULL;
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index d96aa5d23..244261dff 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -353,6 +353,7 @@ connection_ap_expire_beginning(void)
   or_options_t *options = get_options();
   int severity;
   int cutoff;
+  int seconds_idle;
 
   get_connection_array(&carray, &n);
 
@@ -363,19 +364,28 @@ connection_ap_expire_beginning(void)
     /* if it's an internal bridge connection, don't yell its status. */
     severity = (!conn->_base.addr && !conn->_base.port)
       ? LOG_INFO : LOG_NOTICE;
-    if (conn->_base.state == AP_CONN_STATE_CONTROLLER_WAIT) {
-      if (now - conn->_base.timestamp_lastread >= options->SocksTimeout) {
-        log_fn(severity, LD_APP, "Closing unattached stream.");
+    seconds_idle = now - conn->_base.timestamp_lastread;
+
+    if (AP_CONN_STATE_IS_UNATTACHED(conn->_base.state)) {
+      if (seconds_idle >= options->SocksTimeout) {
+        if (conn->_base.state == AP_CONN_STATE_CIRCUIT_WAIT) {
+          log_fn(severity, LD_APP,
+            "Tried for %d seconds to get a connection to %s:%d. Giving up.",
+            seconds_idle, safe_str(conn->socks_request->address),
+            conn->socks_request->port);
+        } else {
+          log_fn(severity, LD_APP, "Closing unattached stream.");
+        }
         connection_mark_unattached_ap(conn, END_STREAM_REASON_TIMEOUT);
       }
       continue;
     }
 
-    if (conn->_base.state != AP_CONN_STATE_RESOLVE_WAIT &&
-        conn->_base.state != AP_CONN_STATE_CONNECT_WAIT)
+    if (conn->_base.state == AP_CONN_STATE_OPEN)
       continue;
+
     cutoff = compute_socks_timeout(conn);
-    if (now - conn->_base.timestamp_lastread < cutoff)
+    if (seconds_idle < cutoff)
       continue;
     circ = circuit_get_by_edge_conn(conn);
     if (!circ) { /* it's vanished? */
@@ -385,11 +395,11 @@ connection_ap_expire_beginning(void)
       continue;
     }
     if (circ->purpose == CIRCUIT_PURPOSE_C_REND_JOINED) {
-      if (now - conn->_base.timestamp_lastread > options->SocksTimeout) {
+      if (seconds_idle > options->SocksTimeout) {
         log_fn(severity, LD_REND,
                "Rend stream is %d seconds late. Giving up on address"
                " '%s.onion'.",
-               (int)(now - conn->_base.timestamp_lastread),
+               seconds_idle,
                safe_str(conn->socks_request->address));
         connection_edge_end(conn, END_STREAM_REASON_TIMEOUT,
                             conn->cpath_layer);
@@ -403,8 +413,7 @@ connection_ap_expire_beginning(void)
     log_fn(cutoff < 15 ? LOG_INFO : severity, LD_APP,
            "We tried for %d seconds to connect to '%s' using exit '%s'."
            " Retrying on a new circuit.",
-           (int)(now - conn->_base.timestamp_lastread),
-           safe_str(conn->socks_request->address),
+           seconds_idle, safe_str(conn->socks_request->address),
            nickname ? nickname : "*unnamed*");
     /* send an end down the circuit */
     connection_edge_end(conn, END_STREAM_REASON_TIMEOUT, conn->cpath_layer);
diff --git a/src/or/or.h b/src/or/or.h
index 77d94f1b4..d63f4603a 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -294,7 +294,7 @@ typedef enum {
 #define AP_CONN_STATE_CIRCUIT_WAIT 8
 /** State for a SOCKS connection: sent BEGIN, waiting for CONNECTED. */
 #define AP_CONN_STATE_CONNECT_WAIT 9
-/** State for a SOCKS connection: send RESOLVE, waiting for RESOLVED. */
+/** State for a SOCKS connection: sent RESOLVE, waiting for RESOLVED. */
 #define AP_CONN_STATE_RESOLVE_WAIT 10
 /** State for a SOCKS connection: ready to send and receive. */
 #define AP_CONN_STATE_OPEN 11
@@ -303,6 +303,9 @@ typedef enum {
 #define AP_CONN_STATE_NATD_WAIT 12
 #define _AP_CONN_STATE_MAX 12
 
+#define AP_CONN_STATE_IS_UNATTACHED(s) \
+  ((s) <= AP_CONN_STATE_CIRCUIT_WAIT || (s) == AP_CONN_STATE_NATD_WAIT)
+
 #define _DIR_CONN_STATE_MIN 1
 /** State for connection to directory server: waiting for connect(). */
 #define DIR_CONN_STATE_CONNECTING 1
@@ -501,8 +504,8 @@ typedef enum {
 #define END_STREAM_REASON_CANT_FETCH_ORIG_DEST 260
 #define END_STREAM_REASON_INVALID_NATD_DEST 261
 
-/* OR this with the argument to control_event_stream_status to indicate that
- * the reason came from an END cell. */
+/** Bitwise-or this with the argument to control_event_stream_status
+ * to indicate that the reason came from an END cell. */
 #define END_STREAM_REASON_FLAG_REMOTE     512
 
 #define RESOLVED_TYPE_HOSTNAME 0