r9289@31-35-219: nickm | 2006-10-20 09:43:22 -0400

 Fix longstanding bug in connection_exit_begin_conn():  Since connection_edge_end() exits when the connection is unattached, we were never sending RELAY_END cells back for failed RELAY_BEGIN attempts. Fix this.  This might make clients that were otherwise timing out either fail faster or retry faster, which is good news for us.


svn:r8770

ChangeLog

@@ -48,6 +48,10 @@ Changes in version 0.1.2.3-alpha - 2006-10-??
     - Detect the size of the routers file correctly even if it is corrupted
       (on systems without mmap) or not page-aligned (on systems with mmap).
       This bug was harmless.
+    - Implement the protocol correctly by always sending a RELAY_END cell
+      when an attempt to open a stream fails.  This should make clients
+      able to find a good exit faster in some cases, since unhandleable
+      requests will now get an error rather than timing out.
 
 
 Changes in version 0.1.2.2-alpha - 2006-10-07

doc/TODO

@@ -57,9 +57,9 @@ N . Have (and document) a BEGIN_DIR relay cell that means "Connect to your
     - Use for something, so we can be sure it works.
     - Test and debug
 
-N - Send back RELAY_END cells on malformed RELAY_BEGIN.
+  o Send back RELAY_END cells on malformed RELAY_BEGIN.
 
-N - Change the circuit end reason display a little for reasons from
+  o Change the circuit end reason display a little for reasons from
     destroyed/truncated circuits.  We want to indicate both that we're
     closing because somebody told us to, and why they told us they wanted to
     close.

src/or/connection_edge.c

@@ -1838,36 +1838,51 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
   relay_header_t rh;
   char *address=NULL;
   uint16_t port;
+  char end_payload[1];
 
   assert_circuit_ok(circ);
 
-  /* XXX currently we don't send an end cell back if we drop the
-   * begin because it's malformed.
-   */
+  relay_header_unpack(&rh, cell->payload);
+
+  /* Note: we have to use relay_send_command_from_edge here, not
+   * connection_edge_end or connection_edge_send_command, since those require
+   * that we have a stream connected to a circuit, and we don't connect to a
+   * circuit unitl we have a pending/sucessful resolve. */
 
   if (!server_mode(get_options()) &&
       circ->purpose != CIRCUIT_PURPOSE_S_REND_JOINED) {
     log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
            "Relay begin cell at non-server. Dropping.");
+    end_payload[0] = END_STREAM_REASON_EXITPOLICY;
+    relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                 end_payload, 1, NULL);
     return 0;
   }
 
-  relay_header_unpack(&rh, cell->payload);
   if (rh.command == RELAY_COMMAND_BEGIN) {
     if (!memchr(cell->payload+RELAY_HEADER_SIZE, 0, rh.length)) {
       log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
              "Relay begin cell has no \\0. Dropping.");
+      end_payload[0] = END_STREAM_REASON_TORPROTOCOL;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       return 0;
     }
     if (parse_addr_port(LOG_PROTOCOL_WARN, cell->payload+RELAY_HEADER_SIZE,
                         &address,NULL,&port)<0) {
       log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
              "Unable to parse addr:port in relay begin cell. Dropping.");
+      end_payload[0] = END_STREAM_REASON_TORPROTOCOL;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       return 0;
     }
     if (port==0) {
       log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
              "Missing port in relay begin cell. Dropping.");
+      end_payload[0] = END_STREAM_REASON_TORPROTOCOL;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       tor_free(address);
       return 0;
     }
@@ -1876,6 +1891,9 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
       log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
              "Non-printing characters in address %s in relay "
              "begin cell. Dropping.", escaped(address));
+      end_payload[0] = END_STREAM_REASON_TORPROTOCOL;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       tor_free(address);
       return 0;
     }
@@ -1886,15 +1904,27 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
        */
       log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
              "Attempt to open a stream on first hop of circuit. Dropping.");
+      end_payload[0] = END_STREAM_REASON_TORPROTOCOL;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       tor_free(address);
       return 0;
     }
   } else if (rh.command == RELAY_COMMAND_BEGIN_DIR) {
     or_options_t *options = get_options();
+    port = options->DirPort; /* not actually used to open a connection */
+    if (!port || circ->purpose != CIRCUIT_PURPOSE_OR) {
+      end_payload[0] = END_STREAM_REASON_NOTDIRECTORY;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
+      return 0;
+    }
     address = tor_strdup("127.0.0.1");
-    port = options->DirPort; /* not actually used. */
   } else {
     log_warn(LD_BUG, "Got an unexpected command %d", (int)rh.command);
+    end_payload[0] = END_STREAM_REASON_INTERNAL;
+    relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                 end_payload, 1, NULL);
     return 0;
   }
 
@@ -1908,15 +1938,6 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
   n_stream->package_window = STREAMWINDOW_START;
   n_stream->deliver_window = STREAMWINDOW_START;
 
-  if (rh.command == RELAY_COMMAND_BEGIN_DIR &&
-      (!get_options()->DirPort || circ->purpose != CIRCUIT_PURPOSE_OR)) {
-    connection_edge_end(n_stream, END_STREAM_REASON_NOTDIRECTORY,
-                        n_stream->cpath_layer);
-    connection_free(TO_CONN(n_stream));
-    tor_free(address);
-    return 0;
-  }
-
   if (circ->purpose == CIRCUIT_PURPOSE_S_REND_JOINED) {
     origin_circuit_t *origin_circ = TO_ORIGIN_CIRCUIT(circ);
     log_debug(LD_REND,"begin is for rendezvous. configuring stream.");
@@ -1929,8 +1950,9 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
     if (rend_service_set_connection_addr_port(n_stream, origin_circ) < 0) {
       log_info(LD_REND,"Didn't find rendezvous service (port %d)",
                n_stream->_base.port);
-      connection_edge_end(n_stream, END_STREAM_REASON_EXITPOLICY,
-                          n_stream->cpath_layer);
+      end_payload[0] = END_STREAM_REASON_EXITPOLICY;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       connection_free(TO_CONN(n_stream));
       /* knock the whole thing down, somebody screwed up */
       circuit_mark_for_close(circ, END_CIRC_REASON_CONNECTFAILED);
@@ -1957,8 +1979,9 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
   /* default to failed, change in dns_resolve if it turns out not to fail */
 
   if (we_are_hibernating()) {
-    connection_edge_end(n_stream, END_STREAM_REASON_HIBERNATING,
-                        n_stream->cpath_layer);
+    end_payload[0] = END_STREAM_REASON_HIBERNATING;
+    relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                 end_payload, 1, NULL);
     connection_free(TO_CONN(n_stream));
     return 0;
   }
@@ -1985,7 +2008,9 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
       connection_exit_connect(n_stream);
       return 0;
     case -1: /* resolve failed */
-      /*  XXXX send back indication of failure for connect case? -NM*/
+      end_payload[0] = END_STREAM_REASON_RESOLVEFAILED;
+      relay_send_command_from_edge(rh.stream_id, circ, RELAY_COMMAND_END,
+                                   end_payload, 1, NULL);
       /* n_stream got freed. don't touch it. */
       break;
     case 0: /* resolve added to pending list */
@@ -2157,7 +2182,7 @@ connection_exit_connect_dir(edge_connection_t *exit_conn)
 
   if ((err = tor_socketpair(AF_UNIX, SOCK_STREAM, 0, fd)) < 0) {
     log_warn(LD_NET,
-             "Couldn't construct socketpair (%s). Network down? Delaying.",
+             "Couldn't construct socketpair (%s). Out of sockets?",
              tor_socket_strerror(-err));
     connection_edge_end(exit_conn, END_STREAM_REASON_RESOURCELIMIT,
                         exit_conn->cpath_layer);

src/or/or.h

@@ -2332,6 +2332,9 @@ int circuit_receive_relay_cell(cell_t *cell, circuit_t *circ,
 
 void relay_header_pack(char *dest, const relay_header_t *src);
 void relay_header_unpack(relay_header_t *dest, const char *src);
+int relay_send_command_from_edge(uint16_t stream_id, circuit_t *circ,
+                                int relay_command, const char *payload,
+                                size_t payload_len, crypt_path_t *cpath_layer);
 int connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ,
                                  int relay_command, const char *payload,
                                  size_t payload_len,

src/or/relay.c

@@ -446,17 +446,17 @@ relay_header_unpack(relay_header_t *dest, const char *src)
   dest->length = ntohs(get_uint16(src+9));
 }
 
-/** Make a relay cell out of <b>relay_command</b> and <b>payload</b>, and
- * send it onto the open circuit <b>circ</b>. <b>fromconn</b> is the stream
- * that's sending the relay cell, or NULL if it's a control cell.
- * <b>cpath_layer</b> is NULL for OR->OP cells, or the destination hop
- * for OP->OR cells.
+/** Make a relay cell out of <b>relay_command</b> and <b>payload</b>, and send
+ * it onto the open circuit <b>circ</b>. <b>stream_id</b> is the ID on
+ * <b>circ</b> for the stream that's sending the relay cell, or 0 if it's a
+ * control cell.  <b>cpath_layer</b> is NULL for OR->OP cells, or the
+ * destination hop for OP->OR cells.
  *
- * If you can't send the cell, mark the circuit for close and
- * return -1. Else return 0.
+ * If you can't send the cell, mark the circuit for close and return -1. Else
+ * return 0.
  */
 int
-connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ,
+relay_send_command_from_edge(uint16_t stream_id, circuit_t *circ,
                              int relay_command, const char *payload,
                              size_t payload_len, crypt_path_t *cpath_layer)
 {
@@ -465,26 +465,7 @@ connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ,
   int cell_direction;
   /* XXXX NM Split this function into a separate versions per circuit type? */
 
-  if (fromconn && fromconn->_base.marked_for_close) {
-    log_warn(LD_BUG,
-             "Bug: called on conn that's already marked for close at %s:%d.",
-             fromconn->_base.marked_for_close_file,
-             fromconn->_base.marked_for_close);
-    return 0;
-  }
-
-  if (!circ) {
-    tor_assert(fromconn);
-    if (fromconn->_base.type == CONN_TYPE_AP) {
-      log_info(LD_APP,"no circ. Closing conn.");
-      connection_mark_unattached_ap(fromconn, END_STREAM_REASON_INTERNAL);
-    } else {
-      log_info(LD_EXIT,"no circ. Closing conn.");
-      fromconn->_base.edge_has_sent_end = 1; /* no circ to send to */
-      connection_mark_for_close(TO_CONN(fromconn));
-    }
-    return -1;
-  }
+  tor_assert(circ);
 
   memset(&cell, 0, sizeof(cell_t));
   cell.command = CELL_RELAY;
@@ -500,8 +481,7 @@ connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ,
 
   memset(&rh, 0, sizeof(rh));
   rh.command = relay_command;
-  if (fromconn)
-    rh.stream_id = fromconn->stream_id; /* else it's 0 */
+  rh.stream_id = stream_id;
   rh.length = payload_len;
   relay_header_pack(cell.payload, &rh);
   if (payload_len) {
@@ -521,6 +501,48 @@ connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ,
   return 0;
 }
 
+/** Make a relay cell out of <b>relay_command</b> and <b>payload</b>, and
+ * send it onto the open circuit <b>circ</b>. <b>fromconn</b> is the stream
+ * that's sending the relay cell, or NULL if it's a control cell.
+ * <b>cpath_layer</b> is NULL for OR->OP cells, or the destination hop
+ * for OP->OR cells.
+ *
+ * If you can't send the cell, mark the circuit for close and
+ * return -1. Else return 0.
+ */
+int
+connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ,
+                             int relay_command, const char *payload,
+                             size_t payload_len, crypt_path_t *cpath_layer)
+{
+  /* XXXX NM Split this function into a separate versions per circuit type? */
+
+  if (fromconn && fromconn->_base.marked_for_close) {
+    log_warn(LD_BUG,
+             "Bug: called on conn that's already marked for close at %s:%d.",
+             fromconn->_base.marked_for_close_file,
+             fromconn->_base.marked_for_close);
+    return 0;
+  }
+
+  if (!circ) {
+    tor_assert(fromconn);
+    if (fromconn->_base.type == CONN_TYPE_AP) {
+      log_info(LD_APP,"no circ. Closing conn.");
+      connection_mark_unattached_ap(fromconn, END_STREAM_REASON_INTERNAL);
+    } else {
+      log_info(LD_EXIT,"no circ. Closing conn.");
+      fromconn->_base.edge_has_sent_end = 1; /* no circ to send to */
+      connection_mark_for_close(TO_CONN(fromconn));
+    }
+    return -1;
+  }
+
+  return relay_send_command_from_edge(fromconn ? fromconn->stream_id : 0,
+                                      circ, relay_command, payload,
+                                      payload_len, cpath_layer);
+}
+
 /** Translate <b>reason</b>, which came from a relay 'end' cell,
  * into a static const string describing why the stream is closing.
  * <b>reason</b> is -1 if no reason was provided.
@@ -545,6 +567,7 @@ connection_edge_end_reason_str(int reason)
     case END_STREAM_REASON_RESOURCELIMIT:  return "server out of resources";
     case END_STREAM_REASON_CONNRESET:      return "connection reset";
     case END_STREAM_REASON_TORPROTOCOL:    return "Tor protocol error";
+    case END_STREAM_REASON_NOTDIRECTORY:   return "not a directory";
     default:
       log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
              "Reason for ending (%d) not recognized.",reason);