From 136ed3307140ff82ade78f21765e43c87902d240 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2006 14:57:46 +0000 Subject: [PATCH] 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 | 4 ++ doc/TODO | 4 +- src/or/connection_edge.c | 65 ++++++++++++++++++--------- src/or/or.h | 3 ++ src/or/relay.c | 95 +++++++++++++++++++++++++--------------- 5 files changed, 113 insertions(+), 58 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6dddeeb5e5..e779146968 100644 --- a/ChangeLog +++ b/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 diff --git a/doc/TODO b/doc/TODO index 018f7f990a..d38d4da015 100644 --- a/doc/TODO +++ b/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. diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 854e75566e..d3232a2475 100644 --- a/src/or/connection_edge.c +++ b/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); diff --git a/src/or/or.h b/src/or/or.h index 1ca35c46fb..f44f8a2a90 100644 --- a/src/or/or.h +++ b/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, diff --git a/src/or/relay.c b/src/or/relay.c index bfea129270..f70ed23170 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -446,6 +446,61 @@ relay_header_unpack(relay_header_t *dest, const char *src) dest->length = ntohs(get_uint16(src+9)); } +/** Make a relay cell out of relay_command and payload, and send + * it onto the open circuit circ. stream_id is the ID on + * circ for the stream that's sending the relay cell, or 0 if it's a + * control cell. cpath_layer 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 +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) +{ + cell_t cell; + relay_header_t rh; + int cell_direction; + /* XXXX NM Split this function into a separate versions per circuit type? */ + + tor_assert(circ); + + memset(&cell, 0, sizeof(cell_t)); + cell.command = CELL_RELAY; + if (cpath_layer) { + cell.circ_id = circ->n_circ_id; + cell_direction = CELL_DIRECTION_OUT; + } else if (! CIRCUIT_IS_ORIGIN(circ)) { + cell.circ_id = TO_OR_CIRCUIT(circ)->p_circ_id; + cell_direction = CELL_DIRECTION_IN; + } else { + return -1; + } + + memset(&rh, 0, sizeof(rh)); + rh.command = relay_command; + rh.stream_id = stream_id; + rh.length = payload_len; + relay_header_pack(cell.payload, &rh); + if (payload_len) { + tor_assert(payload_len <= RELAY_PAYLOAD_SIZE); + memcpy(cell.payload+RELAY_HEADER_SIZE, payload, payload_len); + } + + log_debug(LD_OR,"delivering %d cell %s.", relay_command, + cell_direction == CELL_DIRECTION_OUT ? "forward" : "backward"); + + if (circuit_package_relay_cell(&cell, circ, cell_direction, cpath_layer) + < 0) { + log_warn(LD_BUG,"circuit_package_relay_cell failed. Closing."); + circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); + return -1; + } + return 0; +} + /** Make a relay cell out of relay_command and payload, and * send it onto the open circuit circ. fromconn is the stream * that's sending the relay cell, or NULL if it's a control cell. @@ -460,9 +515,6 @@ 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) { - cell_t cell; - relay_header_t rh; - int cell_direction; /* XXXX NM Split this function into a separate versions per circuit type? */ if (fromconn && fromconn->_base.marked_for_close) { @@ -486,39 +538,9 @@ connection_edge_send_command(edge_connection_t *fromconn, circuit_t *circ, return -1; } - memset(&cell, 0, sizeof(cell_t)); - cell.command = CELL_RELAY; - if (cpath_layer) { - cell.circ_id = circ->n_circ_id; - cell_direction = CELL_DIRECTION_OUT; - } else if (! CIRCUIT_IS_ORIGIN(circ)) { - cell.circ_id = TO_OR_CIRCUIT(circ)->p_circ_id; - cell_direction = CELL_DIRECTION_IN; - } else { - return -1; - } - - memset(&rh, 0, sizeof(rh)); - rh.command = relay_command; - if (fromconn) - rh.stream_id = fromconn->stream_id; /* else it's 0 */ - rh.length = payload_len; - relay_header_pack(cell.payload, &rh); - if (payload_len) { - tor_assert(payload_len <= RELAY_PAYLOAD_SIZE); - memcpy(cell.payload+RELAY_HEADER_SIZE, payload, payload_len); - } - - log_debug(LD_OR,"delivering %d cell %s.", relay_command, - cell_direction == CELL_DIRECTION_OUT ? "forward" : "backward"); - - if (circuit_package_relay_cell(&cell, circ, cell_direction, cpath_layer) - < 0) { - log_warn(LD_BUG,"circuit_package_relay_cell failed. Closing."); - circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); - return -1; - } - return 0; + return relay_send_command_from_edge(fromconn ? fromconn->stream_id : 0, + circ, relay_command, payload, + payload_len, cpath_layer); } /** Translate reason, which came from a relay 'end' cell, @@ -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);